Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch custom Bzip2 cmake module to standard #2718

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

zklaus
Copy link

@zklaus zklaus commented Jul 5, 2023

This replaces the custom Cmake modules FindBz2.cmake and FindBzip2.cmake with the standard FindBZip2, included in Cmake. This entails a few changed variable names, but I tried to contain them in the Cmake parts of the build system, maintaining compatibility with the Autotools version.

It is noteworthy that what appears to be the current incarnation of the original source of those custom modules at https://github.com/sidch/CMake/tree/main/Modules does not contain them anymore.

It is not completely clear to me why there are two different detection methods in the current sources, so I may be missing something, but it does lead to problems with the Bzip2 plugin installation (see #2717).

I also backported this change to 4.9.2, using it in the Conda-forge build, where it successfully installs the Bzip2 plugin.

Closes #2717.

@CLAassistant
Copy link

CLAassistant commented Jul 5, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ WardF
❌ Klaus Zimmermann


Klaus Zimmermann seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@zklaus zklaus force-pushed the cmake-bzip2-handling branch from 3669c38 to a123e04 Compare July 5, 2023 09:38
@zklaus zklaus marked this pull request as ready for review July 5, 2023 11:28
@DennisHeimbigner
Copy link
Collaborator

DennisHeimbigner commented Jul 12, 2023

If memory serves, I found certain platforms had bz2 and not bzip2 and other platforms vice-versa.
Have you tried this under Windows using visual studio?

@zklaus
Copy link
Author

zklaus commented Jul 12, 2023

I have used this patch in the Conda-forge builds since conda-forge/libnetcdf-feedstock#178 and there it works on Windows, see here and here.

Allow me to highlight that the approach currently in main does not work for any platform because the top-level CMakeLists.txt file calls only FindBz2 (via FIND_PACKAGE(Bz2)), but the plugin installation uses the variables that would be set by FindBzip2.

@DennisHeimbigner
Copy link
Collaborator

Its coming back to me again. Under Ubuntu-22, I found that apt
has not libbzip2-dev package. It only has libbz2-dev package.
That was why I tried to test for both bz2 and bzip2.

@zklaus
Copy link
Author

zklaus commented Jul 13, 2023

Ah, that makes sense. The problem still needs to be addressed, though. Do you know where you got the FindBzip2.cmake and FindBz2.cmake from?

Regardless, the good news is that the built-in module takes care of this by looking for both bzip2 and bz2 and setting everything up accordingly, see https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/FindBZip2.cmake#L65.

@DennisHeimbigner
Copy link
Collaborator

The only clue I see is this line: # Author: Siddhartha Chaudhuri, 2009

@DennisHeimbigner
Copy link
Collaborator

This change fails for me under Ubuntu 22. It says:

-- Could NOT find BZip2 (missing: BZIP2_LIBRARIES BZIP2_INCLUDE_DIR)

@WardF
Copy link
Member

WardF commented Jul 13, 2023

I'm seeing the same behavior as Dennis, but will investigate further.

@WardF
Copy link
Member

WardF commented Jul 13, 2023

This is working for me on MacOS so I'll see if I can figure out what's happening on Ubuntu 22.04.

@DennisHeimbigner
Copy link
Collaborator

what bzip2 package is being installed under MACOS? bz2 or bzip2? My guess us bzip2.

@WardF
Copy link
Member

WardF commented Jul 13, 2023 via email

@zklaus
Copy link
Author

zklaus commented Jul 17, 2023

The only clue I see is this line: # Author: Siddhartha Chaudhuri, 2009

Yeah, I did see that and it is what lead me to https://github.com/sidch/CMake, which is a repository with a relatively large collection of Cmake modules by (presumably) the same author. I thought I might find the two modules used here there as well, but they are not present. The history of that repository stretches back only to August 2012 with an initial commit that already adds a lot of modules, so what I think happened is that the author has maintained the collection for some time, in 2009 including the Bzip2 modules, and removing them sometime between 2009 and the move of the collection to git in 2012, probably in favor of the upstream one.

This change fails for me under Ubuntu 22. It says:

-- Could NOT find BZip2 (missing: BZIP2_LIBRARIES BZIP2_INCLUDE_DIR)

Is that Ubuntu 22.04 Jammy or Ubuntu 22.10 Kinetic?

With libbz2-dev installed, both should have bzlib.h and libbz.so (see filelists for Jammy and Kinetic), which should be enough to find the library.
Perhaps you could share your build/CMakeCache.txt and build/CMakeFiles/CMake*.log files?

what bzip2 package is being installed under MACOS? bz2 or bzip2? My guess us bzip2.

Just to check I am not missing something: There aren't really two separate packages, right? It's all the same library, just that some packagers choose different names for their respective packages, right? As such, the question would maybe rather be "Which package manager are you using?" with Homebrew and Macports being the two most likely candidates for Macos.

As binary on Linux, I have only seen libbz2.so. This is true on Ubuntu (package name libbz2-dev), RHEL (bzip2-devel), and Conda-forge (bzip2).

IF(Bz2_FOUND)
SET(TLL_LIBS ${TLL_LIBS} ${Bz2_LIBRARIES})
IF(BZIP2_FOUND)
SET(TLL_LIBS ${TLL_LIBS} ${BZIP2_LIBRARIES})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FindBzip2 module defines an imported library, so might be better to use that, as it will take care of the header locations too:

Suggested change
SET(TLL_LIBS ${TLL_LIBS} ${BZIP2_LIBRARIES})
SET(TLL_LIBS ${TLL_LIBS} BZip2::BZip2)

(and below on the buildplugin line)

@WardF
Copy link
Member

WardF commented Oct 24, 2023

Re-checking these PR's in an attempt to get caught up on the backlog.

CMakeLists.txt Outdated Show resolved Hide resolved
Co-authored-by: Peter Hill <[email protected]>
@WardF
Copy link
Member

WardF commented Jan 29, 2024

@K20shores Can you take a look at the conflict and confirm where this should go instead, keeping in line with the changes you've made?

@WardF WardF self-assigned this Jan 29, 2024
@WardF WardF added this to the 4.9.3 milestone Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bzip2/Bz2 confusion in Cmake build
5 participants