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 incomplete without building with CMake #157078

Closed
4 tasks done
jpfeuffer opened this issue Dec 12, 2023 · 13 comments · Fixed by #159165
Closed
4 tasks done

HDF5 incomplete without building with CMake #157078

jpfeuffer opened this issue Dec 12, 2023 · 13 comments · Fixed by #159165
Labels
bug Reproducible Homebrew/homebrew-core bug outdated PR was locked due to age

Comments

@jpfeuffer
Copy link
Contributor

jpfeuffer commented Dec 12, 2023

brew gist-logs <formula> link OR brew config AND brew doctor output

❯ brew gist-logs hdf5
Error: No logs.

# but should be irrelevant since the installation is successful but incomplete (please read further)

Verification

  • My "brew doctor output" says Your system is ready to brew. and am still able to reproduce my issue.
  • I ran brew update and am still able to reproduce my issue.
  • I have resolved all warnings from brew doctor and that did not fix my problem.
  • I searched for recent similar issues at https://github.com/Homebrew/homebrew-core/issues?q=is%3Aissue and found no duplicates.

What were you trying to do (and why)?

In CMake:
find_package(HDF5 CONFIG ... )

To find HDF5 in my tool/lib.

What happened (include all command output)?

Fail.

[cmake] CMake Error at cmake/cmake_findExternalLibs.cmake:172 (find_package):
[cmake]   Could not find a package configuration file provided by "HDF5" with any of
[cmake]   the following names:
[cmake] 
[cmake]     HDF5Config.cmake
[cmake]     hdf5-config.cmake

What did you expect to happen?

Find HDF5 successfully.

Requirement would be the config files that hdf5 comes with if you built it with CMake.

https://forum.hdfgroup.org/t/can-we-retire-the-autotools/10362
Looks like many people are in favour of retiring autotools.

Also ".pc" files were requested before, which are only available with CMake:
#61490
And one can IMHO not say anymore that autotools is "the recommended" way (rather the other way round).
(Especially since the then-opened issue was one of the first ever and has not been addressed in their autotools build in almost 4 years HDFGroup/hdf5#8 )

Step-by-step reproduction instructions (by running brew commands)

# Something like this (but the problem should obvious by just inspecting what you are installing during the hdf5 build which uses autotools).

brew install hdf5

echo "find_package(HDF5 CONFIG)" >> script.cmake

cmake -P script.cmake
@jpfeuffer jpfeuffer added the bug Reproducible Homebrew/homebrew-core bug label Dec 12, 2023
@SMillerDev SMillerDev mentioned this issue Dec 14, 2023
6 tasks
@SMillerDev
Copy link
Member

@jpfeuffer how do we stop it from compiling in hardcoded paths for everything though?

@jpfeuffer
Copy link
Contributor Author

Cool! Thanks for starting this.
Can you give me an example?
I can try with my general cmake knowledge but I could also contact the HDF5 people.

@jpfeuffer
Copy link
Contributor Author

Do you mean paths to shared libraries? In that case I would need to know what the policies for homebrew are.
I assume you are using rpaths or something like that?

@SMillerDev
Copy link
Member

SMillerDev commented Dec 14, 2023

No, shared libraries seem fine. What I'm looking for is things like:

$ strings /opt/homebrew/bin/h5diff | grep /opt
Installation point: /opt/homebrew/Cellar/hdf5/1.14.3
C Compiler: /opt/homebrew/Library/Homebrew/shims/mac/super/clang 15.0.0.15000100

Not sure what the install point is used for, but the compiler shim is not useful for anyone.

@jpfeuffer
Copy link
Contributor Author

jpfeuffer commented Dec 14, 2023

I totally agree.. unfortunately there seem to be A LOT of hardcoded paths in the different binaries. I asked on the HDF5 forum for now.
Do we know if this was different with autoconf?
And more importantly, is this the only problem? Looks like h5cc is not executing correctly, right?

One thing I saw is that you are currently not enabling C++ and Fortran (but I think they should not be necessary for h5cc).

@jpfeuffer
Copy link
Contributor Author

jpfeuffer commented Jan 4, 2024

Hi! I just wanted to revive this discussion. Meanwhile I received an answer from the HDF5 staff that confirms that the hardcoded build information is not used by the programs and is therefore no issue for the functionality of the binaries.

The build information seemingly comes from this file, that is only included in the binaries with the CMake build. It seems merely informative for debugging.

I think the strings to the source files come from the __FILE__ macro and they might be different (in CMake) from what autotools creates based on how the source files are passed to the compiler. The developers say they are also seeing information from the source files in errors so I guess it can be considered "normal".

Is this a hard rule for homebrew or can we ignore/acknowledge this problem and continue if it does not affect functionality?
If we can continue, I think we should enable C++ and Fortran to see if the tests go further.

@SMillerDev
Copy link
Member

The developers say they are also seeing information from the source files in errors so I guess it can be considered "normal".

The way Homebrew builds and in doing so intercepts compiler calls, this information will be pretty useless to most people. For autotools we could still override it, but for CMake it'll likely just make it much harder to use HDF5.

@jpfeuffer
Copy link
Contributor Author

jpfeuffer commented Jan 4, 2024

Sorry, but can you elaborate?
I don't think you have to change or intercept anything. The binaries should work as-is, they just contain unnecessary but non-harmful information.
And I would say that 99% of the users will not run "strings" on their binaries. Regarding error messages: It should not matter much if users see Exception in line 9 of /opt/homebrew/hdf5/src/foo.c or Exception in line 9 of hdf5/src/foo.c since none of those files will be present. It will just be used for reporting.
But that is just my opinion since I think the CMake config and pkgconfig are bigger upsides than this downside.

If it is a strict rule however, we should file a bug report at HDF5 GitHub and ask the developers if they are willing to change that. They probably need to reimplement the __FILE__ macro then and add a switch to not include the "Build information" header in every binary.

@jpfeuffer
Copy link
Contributor Author

jpfeuffer commented Jan 4, 2024

You can also just add -DCMAKE_CXX_FLAGS="-fmacro-prefix-map=/opt/homebrew/=." -DCMAKE_C_FLAGS="-fmacro-prefix-map=/opt/homebrew=." to replace your build folder with a dot in the built-in macros like __FILE__, then we solved the error message problem.

And the H5build_settings.cmake.c.in file that I mentioned in my previous comment can be easily grepped to remove the information that you think is harmful before starting the build (or even replaced with a completely blank string probably).

@SMillerDev
Copy link
Member

Regarding error messages: It should not matter much if users see Exception in line 9 of /opt/homebrew/hdf5/src/foo.c or Exception in line 9 of hdf5/src/foo.c since none of those files will be present. It will just be used for reporting.

It'll say something like /tmp/fsdhkfhsdk/brewfkjshfkdsj/build/hdf5-1.2.3/hdf5/src/foo.c though. Being compiled by /opt/homebrew/Library/Homebrew/something/something/shims/clang, which could be either pointing to gcc or clang.

@jpfeuffer
Copy link
Contributor Author

Regarding error messages from macros: My suggestion still stands. You can use $pwd or $BUILD_FOLDER (you probably have such a variable) in the compiler flag.

Regarding shims, you are replacing the shims for autotools as well, so why don't you replace them for Cmake? You just need to choose another file.

    # Avoid shims in settings file
    inreplace_files = %w[
      src/H5build_settings.c
      src/libhdf5.settings
      src/Makefile
    ]

becomes

    # Avoid shims in settings file
    inreplace_files = %w[
      src/H5build_settings.cmake.c.in   <--- note this
      src/libhdf5.settings
    ]

And of course you have to run this before the build.

@jpfeuffer
Copy link
Contributor Author

My bad, see my comment in the PR. You apparently want to do it after cmake configuration, when the actual paths are already in the file. In this case your path build/src/H5build_settings.c was correct since the file is renamed during configuration.
I think the only mistake was then the order of replacing and building.

@eli-schwartz
Copy link

I'm not aware offhand of a single linux distro that uses anything but the autotools files to build hdf5. Usually with notes that the cmake build is badly broken.

Also ".pc" files were requested before, which are only available with CMake:
#61490
And one can IMHO not say anymore that autotools is "the recommended" way (rather the other way round).
(Especially since the then-opened issue was one of the first ever and has not been addressed in their autotools build in almost 4 years HDFGroup/hdf5#8 )

Linux distros are so convinced that the cmake build of hdf5 is irreparably broken that they will even configure with cmake just to generate the pkg-config files, install the pkg-config files on their own using cp, and build and install with autotools.

@github-actions github-actions bot added the outdated PR was locked due to age label Feb 10, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Reproducible Homebrew/homebrew-core bug outdated PR was locked due to age
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants