[bug] hdf5/1.12.1 (any version) : Include the module_file in cmake_build_modules for targets #10836
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I detected other pull requests that are modifying hdf5/all recipe:
This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
SpaceIm
reviewed
May 18, 2022
SpaceIm
reviewed
May 18, 2022
This comment has been minimized.
This comment has been minimized.
paulharris
added a commit
to paulharris/conan-center-index
that referenced
this pull request
May 18, 2022
7fd1397 to
e793fa1
Compare
SpaceIm
previously approved these changes
May 18, 2022
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit e793fa1hdf5/1.12.1hdf5/1.10.5hdf5/1.12.0hdf5/1.8.21hdf5/1.10.6 |
|
Don't merge yet, |
Convert it to a draft! GH will not allow to be merged. |
|
Seems to be ok now. Note that its "parallel" feature didn't work prior to this patch, and I wasn't able to fix it. |
4 tasks
|
@uilianries can we please merge this? the "parallel" problem mentioned before is a separate problem that should not block this merge. Note the parallel problem existed before this pull, and isn't a flag set by default. |
This comment has been minimized.
This comment has been minimized.
SpaceIm
reviewed
May 24, 2022
|
@paulharris There is a rule which must be followed to merge any PR: https://github.com/conan-io/conan-center-index/blob/master/docs/review_process.md Your PR must pass to be merged. No exceptions. |
Ah sorry I didn't see the failure for some reason. I thought it built ok. |
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit e4a6875hdf5/1.12.1 |
|
All the linux variants built with shared/unshared, until it got to GCC 11 (shared). |
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 187af8bhdf5/1.12.1 |
Try to force CI rebuild again
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 0e595e0hdf5/1.10.5 |
Hide version 1.10.5, is not compiling on CI.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit e6c7681hdf5/1.12.0hdf5/1.12.1hdf5/1.8.21hdf5/1.10.6 |
|
Please, @jcar87 . Can you have a look at this PR and the underlying issue? Thanks! |
Closed
|
Thanks @paulharris for opening a PR to fix up the handling of the CMake module files. I have looked into a the spurious build/test failures and I have a theory about the issues linking ZLIB. However from what I can see, this PR as it is is correct and should not fail, and the current spurious failures are caused by an HTTP timeout retrieving the sources, rather than related to the recipe or Conan Center CI. ZLIB linking errorsThe logic to link to ZLIB in the upstream build scripts is a bit fragile: https://github.com/HDFGroup/hdf5/blob/develop/CMakeFilters.cmake#L63-L67 According to the above, ZLIB will be linked correctly if the following 3 variables area defined: Up until this PR, CMake's own If there is a situation where The linker flag will be missing, due to the I believe what happened on CI is that on some of the docker images, Digging further, the So I think it's plausible that we were seing the confluence of 3 different things:
I believe there should be no issues now by linking directly to HDF5 target namesI have found it a bit hard to follow or understand the purpose of this PR with regards to the generated CMake files. My current understanding (and I could be wrong) is that these files set up alias targets so that we have both upper case and lowercase named targets The HDF5 build itself can also export targets in a generated config, however they follow a slightly different naming convention for the targets, but are also using the My question would be: if both CMake and the upstream project use a different convention (where all targets and the namespace are lower case), is it not a risk that Conan Center introduces a third convention that does not seem to exist out in the wild? I can see that lower-case seems to be convention when using targets for hdf5: https://github.com/search?p=1&q=hdf5%3A%3Ahdf5&type=Code - I would advocate for aligning ourselves with the existing conventions, no matter how recent they are, and discontinue the use of target names that use the uppercase Warnings from hooksThis warning: I believe this hook checks if the package fines CMake modules that consumers can |
|
Thanks @jcar87 for the extensive reply, It would be a lot quicker if someone else who knows more can pick this up and carry it over the line. On your points: cmake_find_package_multiI used cmake_find_package_multi based on someone's recommendation. I assume it needs to switch to cmake_find_package instead? Then there would be no _RELEASE appended. CONAN_PKGre CONAN_PKG::zlib vs ZLIB::ZLIB ... I did try ZLIB::ZLIB but it didn't work - probably because there is no find_package(ZLIB) call in the patched cmake. I tried various things and in the end just followed what past contributors had done: Which was presumably following the pattern set by @SpaceIm back in 2020: I would use a different pattern if I had known what should be done, but then all the other patches should also be cleaned up, and I don't have the experience or time to do all of that. Downstream testsMight be a good time to make the patch and then feed it back to HDF5 upstream via a PR. Target namesYes I agree, it would be better if the recipe were consistent with anything that already exists! Going forwardIt is unclear to me how to proceed with this recipe...
I suppose it depends on the conan v2 schedule, because it sounds like there will be a LOT of changes to move all the recipes to the new CMakeDeps patterns. So perhaps it just needs to be fixed a little further, and then leave notes to the future upgrader. |
You're doing a great job! This one was a series of unfortunate events, I'm afraid.
The The reason the recipe was working before was that the calls to
I see! I was not aware of this one, it's generally a great thing to follow existing patterns for consistency :)
I have checked that the minimum version of CMake required by the HDF5 CMakeLists already has support for the
I have seen other recipes in Conan center where this happens - that is, using CMake package or target names that do not exist out in the wild. This is a consequence of how packages and targets have evolved in CMake, and the current lack of consistent convention.
I think the recipe as it currently is in this PR does the right thing. Is it still the case that it's not passing CI properly?
Down the line, both :) I'd be happy to help!
|
4 tasks
|
I'm confused, the CI now builds this?
Did the CI change? Upstream HDF5 changes to simplify the zlib + szipI might be able to squeeze in a patch for upstream, |
|
Failure in build 37 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
|
I think I can close this now, a newer version of HDF5 is building with a slightly updated recipe, and it seems to be working fine for me. @SSE4 note that your suggested change was NOT part of the new (and current) HDF5 recipe, but it seems to be "working well enough" for now. You may want to apply the changes discussed in this PR yourself? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Specify library name and version: hdf5/1.12.1
The recipe created a cmake module but did not include it in the "cmake_build_modules" set.