Skip to content

Build hdf5 using CMake instead of Autogen - #248

Merged
CusiniM merged 9 commits into
masterfrom
revert-247-revert-243-feature/algiane/NDEBUG-flag-in-Debug-build
Nov 29, 2023
Merged

CusiniM merged 9 commits into
masterfrom
revert-247-revert-243-feature/algiane/NDEBUG-flag-in-Debug-build

Conversation

@TotoGaz

@TotoGaz TotoGaz commented Nov 1, 2023 •

Copy link
Copy Markdown
Contributor

This PR avoids the spurious detection of the NDEBUG compilation flag by CMake when importing the hdf5 package.

As a side-effect, it will allow to test the GEOS asserts in github-ci configs running in Debug mode.

See:

I initially proposed this PR from a fork of the TPLs but the ci fail due to missing authorization to connect to DockerHub.
Previous discussion is available in the initial PR.

Reverts #247 which was reverting #243

closes GEOS-DEV/GEOS#2631
Relates to GEOS-DEV/GEOS#2721

@TotoGaz

TotoGaz commented Nov 2, 2023 •

Copy link
Copy Markdown
Contributor Author

@Algiane @tbeltzun I'm sorry I had to revert your PR #243: our policy is that geos always points to the HEAD of the TPL, which was not the case since GEOS-DEV/GEOS#2721 is not merged. PR #243 should not have been merged.

As a result, anyone cloning both geos and the TPLs would not be able to build. Furthermore, it was preventing any other upgrade of the TPL.

I've reopened the current PR (#248), I've reopened the issues that were closed with the previous PR, and I updated GEOS-DEV/GEOS#2721 with the new TPL tag so everything should be ready on your side (no additional work I hope). Thank you for your understanding!

@Algiane

Algiane commented Nov 6, 2023

Copy link
Copy Markdown
Contributor

Hello @TotoGaz ,

Thanks for the revert and the feedback: I also realized that with this PR the compilation of Silo fails when hdf5 is built in Debug mode, at least on OSX (the hdf5 library is named libhdf5_debug and Silo doesn't find it).

I will try to fix this as soon as possible and to flag the PR as ready when it will be done (I need to free up some time to get back on Geos ;-) ).

Best

…IBRARY_SUFFIX to find the extension of hdf5 shared library.
…hat Silo's autogen script searches for libhdf5 library while CMake's build of hdf5 creates the libhdf5_debug library in Debug mode.
…id a compilation error with release 4.11 of Silo (fixed by release 4.11.1).
@Algiane Algiane removed the bug Something isn't working label Nov 13, 2023
@Algiane

Algiane commented Nov 14, 2023

Copy link
Copy Markdown
Contributor

In last commits, we:

  • switch to RelWithDebInfo compilation for hdf5 if user asks for Debug mode: it is a workaround to allow Silo's Autogen script to find the hdf5library (see this issue ). Note that before this PR, hdf5 was always build in Release mode.
    Once the implementation of Silo's build with CMake will be ready and released, it will be possible to link with the libhdf5_debug library;

  • solve compilation errors due to implicit declaration of function 'zfp_init_zfp' (and missing semi-colons) when building Silo's release 4.11 with Clang (see closed issue). For now, I have added a -w flag to compiler flags if Clang is detected (prior this PR, when building hdf5 using Autotools, this flag was automatically forwarded inside the HDF5_C_FLAGS variable). It allows to build Silo on all the architectures covered by the current ci and on OSX.
    Such a fix is not very satisfying (it probably doesn't work with Clang on windows and we may have issues with other compilers or on exotics architectures): as the compilation errors are solved in Silo's release 4.11.1. I will propose an upgrade to this latter and the removal of the conditional adding of -w flag in another PR (once the current one will be merged).

@TotoGaz TotoGaz left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

LGTM.
@Algiane you can approve it yourself! I cannot do it since I've reopened it, it's mine and github does not want me to approve my PR!

Comment thread CMakeLists.txt Outdated
Comment thread CMakeLists.txt Outdated
tbeltzun and others added 2 commits November 15, 2023 16:22
Co-authored-by: TotoGaz <49004943+TotoGaz@users.noreply.github.com>
Co-authored-by: TotoGaz <49004943+TotoGaz@users.noreply.github.com>
@Algiane

Algiane commented Nov 16, 2023

Copy link
Copy Markdown
Contributor

Hi @TotoGaz ,

Thanks, I will approve the current PR asap!

From the TPL side I think that everything is OK but there are still some issues when building Geos in Debug with Cuda on Ubuntu (we cannot remove the "ready to be merged" flag as Cuda jobs are not launched without this flag).

Best

@tbeltzun

tbeltzun commented Nov 16, 2023 •

Copy link
Copy Markdown
Contributor

Note that we could alternatively try to build silo with --enabled-shared instead of adding a -fPIC flag: this is reserved for a following PR.

@Algiane
Algiane requested review from Algiane and tbeltzun November 16, 2023 15:11
@Algiane

Algiane commented Nov 16, 2023

Copy link
Copy Markdown
Contributor

Hi @TotoGaz ,

I think that both PR are ready to be merged now :

  • the ci is fixed for GEOS PR 2721 that builds GEOS against the image constructed from the current branch;
  • the current branch builds the TPLs without errors for the ci and on OSX.

Thank you by advance for your help to merge this work.
Best,

@CusiniM
CusiniM merged commit 4cee7e3 into master Nov 29, 2023
@tbeltzun
tbeltzun deleted the revert-247-revert-243-feature/algiane/NDEBUG-flag-in-Debug-build branch December 1, 2023 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failing ci tests in Debug mode configure HDF5 with CMake intead of configure

4 participants