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

hdf5: use CMake #159165

Merged
merged 2 commits into from
Jan 10, 2024
Merged

hdf5: use CMake #159165

merged 2 commits into from
Jan 10, 2024

Conversation

jpfeuffer
Copy link
Contributor

closes #157329 (supersedes it)
closes #157078

tagging @SMillerDev

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@github-actions github-actions bot added autosquash Automatically squash pull request commits according to Homebrew style. CI-linux-self-hosted Build on Linux self-hosted runner labels Jan 6, 2024
Copy link
Contributor

github-actions bot commented Jan 6, 2024

Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request.

Formula/h/hdf5.rb Outdated Show resolved Hide resolved
Formula/h/hdf5.rb Outdated Show resolved Hide resolved
Formula/h/hdf5.rb Outdated Show resolved Hide resolved
Formula/h/hdf5.rb Outdated Show resolved Hide resolved
Formula/h/hdf5.rb Outdated Show resolved Hide resolved
@jpfeuffer
Copy link
Contributor Author

I'm not an expert but the dep failures seem unrelated.

@ZhongRuoyu ZhongRuoyu added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Jan 9, 2024
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Jan 9, 2024
@ZhongRuoyu
Copy link
Member

Yes, failures seem unrelated; see also: https://github.com/Homebrew/homebrew-core/pulls?q=is%3Apr+shmget

Co-authored-by: Julianus Pfeuffer <julianus.pfeuffer@pfizer.com>
Co-authored-by: Ruoyu Zhong <zhongruoyu@outlook.com>
@Porkepix
Copy link
Contributor

Porkepix commented Jan 9, 2024

Does it really need linux self-hosted runners? Slightly more than 1H15 on it seems pretty short and could probably fit within 6H of GitHub runners, right?

@ZhongRuoyu
Copy link
Member

Does it really need linux self-hosted runners? Slightly more than 1H15 on it seems pretty short and could probably fit within 6H of GitHub runners, right?

I think it did time out while testing dependents in #140211.

@Porkepix
Copy link
Contributor

Porkepix commented Jan 9, 2024

Mmmmh, maybe also because there were two formulas there vs. only one here.

@jpfeuffer
Copy link
Contributor Author

Speaking of which: we could add the changes for the hdf5-mpi formula as well. Could benefit from the Cmake- and pkg-config files for easier discoverability as well.
I didn't know about that formula and I'm personally not using it, so I'm not sure if it needs special treatment.

@SMillerDev
Copy link
Member

Let's merge this, and then please make a new PR for hdf5-mpi @jpfeuffer

@jpfeuffer jpfeuffer mentioned this pull request Jan 10, 2024
5 tasks
Copy link
Contributor

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label Jan 10, 2024
@BrewTestBot BrewTestBot enabled auto-merge January 10, 2024 10:09
@BrewTestBot BrewTestBot added this pull request to the merge queue Jan 10, 2024
Merged via the queue into Homebrew:master with commit a656e8d Jan 10, 2024
12 checks passed
@eli-schwartz
Copy link

This is causing universal CI failures for attempting to detect and compile against hdf5 in the Meson CI.

hdf5 cannot and must not ever be built with cmake. Using broken build files that upstream doesn't seriously support, just to make cmake's find_package() detect hdf5, is not a real solution. There's a reason that every linux distro I'm aware of builds hdf5 with autotools -- sometimes with comments about how the cmake support is utterly broken, sometimes without comment.

@jpfeuffer
Copy link
Contributor Author

jpfeuffer commented Jan 11, 2024

EDIT: Ignore. I thought you were saying the config files are broken.

Can't you just search in "MODULE" mode then? No one forces someone to use those config files.
And how about raising the problems at the HDF5 repository (potentially repeatedly) until they are fixed. IMHO ignoring the problem is also not a solution.
The config files work great for us and without config files you will never have easy access to compile flags and some more modern CMake features that depend on more detailed descriptions of the targets.

@jpfeuffer
Copy link
Contributor Author

jpfeuffer commented Jan 11, 2024

Ok it looks like you're saying the build itself is broken with CMake. That is hard for me to judge and I would be interested in some of the bug reports of the distros.
Your specific linked case "just" seems to have problems with the c++ standard version and a (probably) missing -lhdf5 in your linker command line.
But I think most importantly this should be posted on a HDF5 issue tracker. Ignoring it won't make it go away.
There is already a forum thread with some of the maintainers on HDF5 where people are discussing dropping maintenance of Autotools scripts.
https://forum.hdfgroup.org/t/can-we-retire-the-autotools/10362/31

@blawrence-ont
Copy link

It appears that this change has broken cmake's FindHDF5 module: https://github.com/Kitware/CMake/blob/master/Modules/FindHDF5.cmake

The module works by calling h5cc with -show to extract the libraries it needs to link to. Previous versions of h5cc handle -show correctly:

% h5cc -show 
clang -I/opt/homebrew/opt/libaec/include -L/opt/homebrew/Cellar/hdf5/1.14.3/lib /opt/homebrew/Cellar/hdf5/1.14.3/lib/libhdf5_hl.a /opt/homebrew/Cellar/hdf5/1.14.3/lib/libhdf5.a -L/opt/homebrew/opt/libaec/lib -lsz -lz -ldl -lm

However with these changes h5cc now returns:

% h5cc -show 
/opt/homebrew/h5cc
dir is /opt/homebrew
clang: error: unknown argument: '-show'

This appears to be because the script is now a very thin wrapper missing a lot of functionality: compare the arg handling in the version that used to be provided https://github.com/HDFGroup/hdf5/blob/develop/bin/h5cc.in with the version now provided https://github.com/HDFGroup/hdf5/blob/develop/config/cmake/libh5cc.in.

Also it surprises me that this change has more than tripled the size of the package from 21.4MB to 77.7MB according to brew info hdf5.

@jpfeuffer
Copy link
Contributor Author

Ok sorry, let's revert. Unfortunately I do not have the resources to debug this.
The idea was actually not to rely on the hand-written, externally updated FindHDF5 module that also lacks information about compiler options, runtime dependencies and shared vs static targets distinction.
But I think this is out of reach.

@jpfeuffer
Copy link
Contributor Author

I will forward this to the HDF forum though.

@eli-schwartz
Copy link

Can't you just search in "MODULE" mode then? No one forces someone to use those config files.

Sure.

Just tell me how to do that from an autotools Makefile and I will get right on that immediately.

Considering that hdf5 has been pretty apathetic about delivering pkg-config files even after delivering them for cmake, there's lots of code out in the wild that uses h5cc -show.

Your specific linked case "just" seems to have problems with the c++ standard version and a (probably) missing -lhdf5 in your linker command line.

And at minimum, the missing -lhdf5 is because of the well known problem that cmake's version of h5cc heard about the idea of "compatibility" and said "what's that, some kind of boring Linux thing?" and implemented the absolute minimum scaffolding needed to be invoked internally by their cmake config file, and absolutely nothing for existing users.

If for no other reason, distributors have to ship an autotools build because they need to support existing software that relies on h5cc working. That software cannot use pkg-config because distributors ship an autotools build. This recursive logic will be fixed exactly no sooner than whenever upstream hdf5 accepts that the request to ship a pkg-config file for autotools isn't a request that should be ignored and closed every time someone offers one, then left to rot forever.

Admittedly it was some time ago that I was aware of the general brokenness of the cmake build. It is possible that at the time the cmake build was still windows-only despite advertising itself for Linux use, but I don't remember. Maybe it's gotten better. Given the overall disaster that is hdf5 and build systems, I'm not hopeful. They responded to the issue of h5cc being broken with "we have documented in the release.txt that h5cc has changed. Closing this bug report as solved".

@eli-schwartz
Copy link

Also it surprises me that this change has more than tripled the size of the package from 21.4MB to 77.7MB according to brew info hdf5.

This is because, even for a project notable for lacking expertise at build systems, they have orders of magnitude less expertise in cmake than they have in autotools.

But some of it is just cmake's fault, because CMake Is That Way™.

github-merge-queue bot pushed a commit to quokka-astro/quokka that referenced this pull request Jan 12, 2024
### Description
The macOS GitHub runner is broken once again due to broken Homebrew
packages.

We fix this by installing a specific version of Python using
`actions/setup-python` instead of `brew install python3` and by
specifying `brew install hdf5@1.10` (since the Homebrew `hdf5@1.14`
package is broken due to
Homebrew/homebrew-core#159165).

### Checklist
_Before this pull request can be reviewed, all of these tasks should be
completed. Denote completed tasks with an `x` inside the square brackets
`[ ]` in the Markdown source below:_
- [x] I have added a description (see above).
- [ ] I have added a link to any related issues see (see above).
- [x] I have read the [Contributing
Guide](https://github.com/quokka-astro/quokka/blob/development/CONTRIBUTING.md).
- [ ] I have added tests for any new physics that this PR adds to the
code.
- [ ] I have tested this PR on my local computer and all tests pass.
- [ ] I have manually triggered the GPU tests with the magic comment
`/azp run`.
- [ ] I have requested a reviewer for this PR.
@github-actions github-actions bot added the outdated PR was locked due to age label Feb 11, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-linux-self-hosted Build on Linux self-hosted runner CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HDF5 incomplete without building with CMake
7 participants