Switch custom Bzip2 cmake module to standard #2718

zklaus · 2023-07-05T08:52:05Z

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 · 2023-07-05T08:52:10Z

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.}

DennisHeimbigner · 2023-07-12T02:33:24Z

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 · 2023-07-12T08:07:12Z

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 · 2023-07-13T02:47:41Z

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 · 2023-07-13T08:29:15Z

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 · 2023-07-13T19:01:26Z

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

DennisHeimbigner · 2023-07-13T19:16:41Z

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

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

WardF · 2023-07-13T20:53:11Z

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

WardF · 2023-07-13T21:03:46Z

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

DennisHeimbigner · 2023-07-13T21:06:32Z

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

WardF · 2023-07-13T21:19:20Z

Dennis, I'll have to dig to see what's installed as part of the development packages re: 'bz2 vs. bzip2', but the library itself is libbz2, so I would suspect bz2.

zklaus · 2023-07-17T09:13:44Z

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).

ZedThree · 2023-10-23T09:06:46Z

liblib/CMakeLists.txt

-IF(Bz2_FOUND)
-SET(TLL_LIBS ${TLL_LIBS} ${Bz2_LIBRARIES})
+IF(BZIP2_FOUND)
+SET(TLL_LIBS ${TLL_LIBS} ${BZIP2_LIBRARIES})


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)

IMO it doesn't belong into TLL_LIBS at all: It is only used in the bzip2 "MODULE".

WardF · 2023-10-24T20:47:45Z

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

CMakeLists.txt

Co-authored-by: Peter Hill <[email protected]>

WardF · 2024-01-29T19:10:58Z

@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?

dg0yt

After hours of trying to update the vcpkg port from 4.9.2 to 4.9.3, a little bit attention for upstream.
netcdf-c, you need to test the installed {cmake,nc,pkg}-config as part of your CI. Add an install step, and build a minimal test project from the installed location. (Sorry if you already do that and I missed it.)

dg0yt · 2025-08-10T15:47:18Z

CMakeLists.txt

  FIND_PACKAGE(Bz2)
+  IF(NOT Bz2_FOUND)
+    FIND_PACKAGE(BZip2)
+  ENDIF(NOT Bz2_FOUND)


"Bz2" exists neither in this repo (after this PR) nor in CMake nor in Bzip2 (as CMake config).

Suggested change

FIND_PACKAGE(Bz2)

IF(NOT Bz2_FOUND)

FIND_PACKAGE(BZip2)

ENDIF(NOT Bz2_FOUND)

FIND_PACKAGE(BZip2)

dg0yt · 2025-08-10T15:47:48Z

CMakeLists.txt

+
 ENDIF()


Cruft.

Suggested change

ENDIF()

ENDIF()

dg0yt · 2025-08-10T15:50:22Z

CMakeLists.txt

 IF(Bz2_FOUND)
  set_std_filter(Bz2)
+ELSEIF(BZIP2_FOUND)
+  set_std_filter(BZIP2)


I don't know what set_std_filter does but it doesn't look like something I would change when I touch the dependency lookup. And there shouldn't be Bz2_FOUND after the other suggestion. Last not least the result variable has a different spelling.

Suggested change

IF(Bz2_FOUND)

set_std_filter(Bz2)

ELSEIF(BZIP2_FOUND)

set_std_filter(BZIP2)

IF(BZip2_FOUND)

set_std_filter(Bz2)

dg0yt · 2025-08-10T15:51:52Z

CMakeLists.txt

+  SET(HAVE_LOCAL_BZIP2 ON)
+  SET(HAVE_BZIP2 ON)
+  set(STD_FILTERS "${STD_FILTERS} bzip2")


Less intrusive:

Suggested change

SET(HAVE_LOCAL_BZIP2 ON)

SET(HAVE_BZIP2 ON)

set(STD_FILTERS "${STD_FILTERS} bzip2")

SET(HAVE_LOCAL_BZ2 ON)

SET(HAVE_BZ2 ON)

set(STD_FILTERS "${STD_FILTERS} bz2")

dg0yt · 2025-08-10T15:52:46Z

CMakeLists.txt

+is_enabled(HAVE_BZIP2 HAS_BZIP2)
+is_enabled(HAVE_BZIP2 HAS_BZ2)


Suggested change

is_enabled(HAVE_BZIP2 HAS_BZIP2)

is_enabled(HAVE_BZIP2 HAS_BZ2)

is_enabled(HAVE_BZ2 HAS_BZ2)

dg0yt · 2025-08-10T15:53:45Z

liblib/CMakeLists.txt

@@ -92,8 +92,8 @@ ENDIF()
 IF(Zstd_FOUND)
 SET(TLL_LIBS ${TLL_LIBS} ${Zstd_LIBRARIES})
 ENDIF()
-IF(Bz2_FOUND)
-SET(TLL_LIBS ${TLL_LIBS} ${Bz2_LIBRARIES})
+IF(BZIP2_FOUND)


Suggested change

IF(BZIP2_FOUND)

IF(BZip2_FOUND)

dg0yt · 2025-08-10T15:54:46Z

liblib/CMakeLists.txt

-IF(Bz2_FOUND)
-SET(TLL_LIBS ${TLL_LIBS} ${Bz2_LIBRARIES})
+IF(BZIP2_FOUND)
+SET(TLL_LIBS ${TLL_LIBS} ${BZIP2_LIBRARIES})


IMO it doesn't belong into TLL_LIBS at all: It is only used in the bzip2 "MODULE".

dg0yt · 2025-08-10T15:55:01Z

plugins/CMakeLists.txt

@@ -98,12 +98,12 @@ IF(HAVE_SZ)
  buildplugin(h5szip "h5szip" ${Szip_LIBRARIES})
 ENDIF()

-IF(HAVE_LOCAL_BZ2)
+IF(HAVE_LOCAL_BZIP2)


Suggested change

IF(HAVE_LOCAL_BZIP2)

IF(HAVE_LOCAL_BZ2)

dg0yt · 2025-08-10T15:55:23Z

plugins/CMakeLists.txt

 SET(h5bzip2_SOURCES H5Zbzip2.c blocksort.c huffman.c crctable.c randtable.c compress.c decompress.c bzlib.c bzlib.h bzlib_private.h)
 buildplugin(h5bzip2 "h5bzip2")
 ELSE()
 SET(h5bzip2_SOURCES H5Zbzip2.c)
-buildplugin(h5bzip2 "h5bzip2" ${Bzip2_LIBRARIES})
+buildplugin(h5bzip2 "h5bzip2" ${BZIP2_LIBRARIES})


Suggested change

buildplugin(h5bzip2 "h5bzip2" ${BZIP2_LIBRARIES})

buildplugin(h5bzip2 "h5bzip2" BZip2::BZip2)

dg0yt · 2025-08-10T15:55:39Z

plugins/CMakeLists.txt

@@ -117,7 +117,7 @@ MACRO(installplugin PLUG)
 ENDMACRO()

 install(DIRECTORY DESTINATION ${PLUGIN_INSTALL_DIR})
-IF(Bzip2_FOUND)
+IF(BZIP2_FOUND)


Suggested change

IF(BZIP2_FOUND)

IF(BZip2_FOUND)

DennisHeimbigner · 2025-08-10T19:18:55Z

This PR reminds me why I hate cmake :-)

dg0yt · 2025-08-10T21:24:41Z

This PR reminds me why I hate cmake :-)

This comment reminds me why I sometimes hate upstreaming.

Switch custom Bzip2 cmake module to standard

a123e04

zklaus force-pushed the cmake-bzip2-handling branch from 3669c38 to a123e04 Compare July 5, 2023 09:38

zklaus marked this pull request as ready for review July 5, 2023 11:28

zklaus requested review from WardF and DennisHeimbigner as code owners July 5, 2023 11:28

zklaus mentioned this pull request Jul 5, 2023

Improve requirements conda-forge/libnetcdf-feedstock#178

Merged

5 tasks

ZedThree reviewed Oct 23, 2023

View reviewed changes

Merge branch 'main' into cmake-bzip2-handling

f27fe60

ZedThree reviewed Jan 18, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Update CMakeLists.txt

2783fbd

Co-authored-by: Peter Hill <[email protected]>

WardF self-assigned this Jan 29, 2024

WardF added this to the 4.9.3 milestone Jan 29, 2024

dg0yt reviewed Aug 10, 2025

View reviewed changes

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

		is_enabled(HAVE_BZIP2 HAS_BZIP2)
		is_enabled(HAVE_BZIP2 HAS_BZ2)

	is_enabled(HAVE_BZIP2 HAS_BZIP2)
	is_enabled(HAVE_BZIP2 HAS_BZ2)
	is_enabled(HAVE_BZ2 HAS_BZ2)

	buildplugin(h5bzip2 "h5bzip2" ${BZIP2_LIBRARIES})
	buildplugin(h5bzip2 "h5bzip2" BZip2::BZip2)

Switch custom Bzip2 cmake module to standard #2718

Are you sure you want to change the base?

Switch custom Bzip2 cmake module to standard #2718

Uh oh!

Conversation

zklaus commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DennisHeimbigner commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zklaus commented Jul 12, 2023

Uh oh!

DennisHeimbigner commented Jul 13, 2023

Uh oh!

zklaus commented Jul 13, 2023

Uh oh!

DennisHeimbigner commented Jul 13, 2023

Uh oh!

DennisHeimbigner commented Jul 13, 2023

Uh oh!

WardF commented Jul 13, 2023

Uh oh!

WardF commented Jul 13, 2023

Uh oh!

DennisHeimbigner commented Jul 13, 2023

Uh oh!

WardF commented Jul 13, 2023 via email

Uh oh!

zklaus commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WardF commented Oct 24, 2023

Uh oh!

Uh oh!

WardF commented Jan 29, 2024

Uh oh!

dg0yt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DennisHeimbigner commented Aug 10, 2025

Uh oh!

dg0yt commented Aug 10, 2025

Uh oh!

Uh oh!

zklaus commented Jul 5, 2023 •

edited

Loading

CLAassistant commented Jul 5, 2023 •

edited

Loading

DennisHeimbigner commented Jul 12, 2023 •

edited

Loading

zklaus commented Jul 17, 2023 •

edited

Loading