Skip to content

Build hdf5 using CMake instead of Autogen. - #239

Closed
Algiane wants to merge 1 commit into
GEOS-DEV:masterfrom
Algiane:feature/NDEBUG-flag-in-Debug-build
Closed

Algiane wants to merge 1 commit into
GEOS-DEV:masterfrom
Algiane:feature/NDEBUG-flag-in-Debug-build

Conversation

@Algiane

@Algiane Algiane commented Sep 15, 2023 •

Copy link
Copy Markdown
Contributor

It 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:

Closes #211, closes GEOS-DEV/GEOS#2631, closes HDFGroup/hdf5#3526.

It avoids the spurious detection of the NDEBUG compilation flag by
CMake when importing the hdf5 package, see:
  - Geos issue #2631
  - hdf5 issue #3526
@untereiner

Copy link
Copy Markdown

Should also close #211

@untereiner untereiner left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

just a comment, not sure if it matters but other external projects have
-D CMAKE_BUILD_TYPE:STRING=${CMAKE_BUILD_TYPE}

@Algiane

Algiane commented Sep 18, 2023

Copy link
Copy Markdown
Contributor Author

Hi @untereiner ,

Thanks for your feedbacks: I have tried to preserve exactly the options that where used to build hdf5 using autogen. As it was always built in production mode, I have chosen to not transfer the CMAKE_BUILD_TYPE info to hdf5 and to always build it in Release cmake build type.

In practice I don't really have an opinion on what is better: do we want to build all TPLs in Debug mode sometimes or only some of them?

@tbeltzun

Copy link
Copy Markdown
Contributor

@TotoGaz is the dockerhub login failure induced by this PR being initiated from a fork ?

In that case won't this prevent opensource contributions for external developers ?

@tbeltzun

Copy link
Copy Markdown
Contributor

In practice I don't really have an opinion on what is better: do we want to build all TPLs in Debug mode sometimes or only some of them?

I think it would be better to propagate CMAKE_BUILD_TYPE explicitly (locally and on P3 I build the TPLs in both Debug and RelWithDebInfo, never with Release :)).

@Algiane

Algiane commented Sep 18, 2023 •

Copy link
Copy Markdown
Contributor Author

Ok, So I will transfer the global CMAKE_BUILD_TYPE toward hdf5.

Just for your info: now that I have commit rights in the repo, I redid this PR from a branch rather than a fork (it seems that the ci doesn't work if launched by a fork due to missing identifiers for connecting to DockerHub).

New PR is available here and point to this conversation.

@Algiane Algiane added duplicate This issue or pull request already exists and removed flag: ready for review labels Sep 18, 2023
@Algiane Algiane closed this Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate This issue or pull request already exists

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Export of the NDEBUG flag in HDF5_DEFINITIONS CMake variable Failing ci tests in Debug mode configure HDF5 with CMake intead of configure

3 participants