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

tests/hdf5: set C++ standard to c++11, which is required for macos CI #12748

Closed
wants to merge 1 commit into from

Conversation

dcbaker
Copy link
Member

@dcbaker dcbaker commented Jan 17, 2024

No description provided.

Copy link
Member

@bruchar1 bruchar1 left a comment

Choose a reason for hiding this comment

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

Yes, I realized that too. Unfortunately, that doesn't fix the broken CI caused by the broken homebrew package...

@dcbaker
Copy link
Member Author

dcbaker commented Jan 17, 2024

Yeah, we can't fix homebrew. sigh

@eli-schwartz
Copy link
Member

It's easy to fix homebrew. Homebrew just needs a PR to revert their hdf5 package to use autotools instead of cmake -- like everyone else builds it.

hdf5 has a totally broken h5cc when built with cmake -- and possibly other issues as well. It's been a long time, but in the past there used to be serious concerns that the cmake build was altogether totally broken on unix, and as a result, no one built with it or reported issues with it so I'd be hesitant to claim it's well tested even.

@jpakkane
Copy link
Member

Undefined symbols for architecture x86_64:
  "_H5close", referenced from:
      _main in main.c.o
  "_H5get_libversion", referenced from:
      _main in main.c.o
  "_H5open", referenced from:
      _main in main.c.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
[3/4] c++ -Iexecpp.p -I. '-I../test cases/frameworks/25 hdf5' -I/usr/local/include -fdiagnostics-color=always -D_GLIBCXX_ASSERTIONS=1 -Wall -Winvalid-pch -std=c++11 -O0 -g -MD -MQ execpp.p/main.cpp.o -MF execpp.p/main.cpp.o.d -o execpp.p/main.cpp.o -c '../test cases/frameworks/25 hdf5/main.cpp'
ninja: build stopped: subcommand failed.

@eli-schwartz
Copy link
Member

Yes, this is Homebrew/homebrew-core#159691

@dcbaker
Copy link
Member Author

dcbaker commented Jan 17, 2024

It looks like using h5cc is deprecated, so we should deprecate that support and skip the test

@eli-schwartz
Copy link
Member

eli-schwartz commented Jan 17, 2024

No, because they refuse to support pkg-config in the build system that all distributors other than "homebrew in the past week" use.

Which means that projects have zero choice, if they want to build against the hdf5 packages in any distro, they must use h5cc -show, there is no other option -- so that's what they do.

It also means, of course, that meson cannot check for hdf5 with pkg-config if there are no operating systems where hdf5 comes with .pc files (or cmake files). :)

(There are a couple people that build hdf5 with autotools and then manually install some pkg-config files, e.g. by configuring cmake in a temporary directory and then copying the .pc files out of cmake's working directory.)

@dcbaker
Copy link
Member Author

dcbaker commented Jan 17, 2024

Nix uses cmake... But it's broken in different ways. I guess I'll send a patch for the nixos build and get that working. What a cluster. I guess what we really should do here is check that we got something reasonable from h5cc and return a graceful message if h5cc is the cmake version. I can do that

@eli-schwartz
Copy link
Member

I guess what we really should do here is check that we got something reasonable from h5cc and return a graceful message if h5cc is the cmake version. I can do that

From 2bc592b0e426afab88bd5a7abf3067ea214ff198 Mon Sep 17 00:00:00 2001
From: Eli Schwartz <eschwartz93@gmail.com>
Date: Wed, 17 Jan 2024 13:23:56 -0500
Subject: [PATCH] dependencies: hdf5: mark configtool dependency not-found for
 cmake build

When hdf5 is built with cmake instead of autotools, it makes a number of
weird changes. What we care about in particular is that h5cc exists but
doesn't work -- it happily ignores -show and tries to compile stuff,
then leaves us with a dependency that has no libraries, and fails when
running `ninja`.

See: #12748
Signed-off-by: Eli Schwartz <eschwartz93@gmail.com>
---
 mesonbuild/dependencies/hdf5.py | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/mesonbuild/dependencies/hdf5.py b/mesonbuild/dependencies/hdf5.py
index a00b5988b..622653025 100644
--- a/mesonbuild/dependencies/hdf5.py
+++ b/mesonbuild/dependencies/hdf5.py
@@ -122,13 +122,20 @@ class HDF5ConfigToolDependency(ConfigToolDependency):
         # and then without -c to get the link arguments.
         args = self.get_config_value(['-show', '-c'], 'args')[1:]
         args += self.get_config_value(['-show', '-noshlib' if self.static else '-shlib'], 'args')[1:]
+        found = False
         for arg in args:
             if arg.startswith(('-I', '-f', '-D')) or arg == '-pthread':
                 self.compile_args.append(arg)
             elif arg.startswith(('-L', '-l', '-Wl')):
                 self.link_args.append(arg)
+                found = True
             elif Path(arg).is_file():
                 self.link_args.append(arg)
+                found = True
+
+        # cmake h5cc is broken
+        if not found:
+            raise DependencyException('HDF5 was built with cmake instead of autotools, and h5cc is broken.')
 
     def _sanitize_version(self, ver: str) -> str:
         v = re.search(r'\s*HDF5 Version: (\d+\.\d+\.\d+)', ver)
-- 
2.43.0

@thesamesam
Copy link
Contributor

thesamesam commented Jan 18, 2024

We hit this in Gentoo before as well (https://bugs.gentoo.org/813924, https://bugs.gentoo.org/820158).

See also HDFGroup/hdf5#1814.

@dcbaker
Copy link
Member Author

dcbaker commented Jan 19, 2024

@eli-schwartz Is it really correct to raise in that case? shouldn't it just be set to self.found = false; return and let the normal code go on to say "couldn't find anything, sorry"?

@eli-schwartz
Copy link
Member

Raising a DependencyException would also mean that it logs the exception text in the log file, and if it cannot resolve via a different method then detect() will use that exception text as part of the "error: dependency hdf5 not found" message.

eli-schwartz added a commit to eli-schwartz/meson that referenced this pull request Feb 1, 2024
When hdf5 is built with cmake instead of autotools, it makes a number of
weird changes. What we care about in particular is that h5cc exists but
doesn't work -- it happily ignores -show and tries to compile stuff,
then leaves us with a dependency that has no libraries, and fails when
running `ninja`.

See: mesonbuild#12748
nirbheek pushed a commit that referenced this pull request Feb 6, 2024
When hdf5 is built with cmake instead of autotools, it makes a number of
weird changes. What we care about in particular is that h5cc exists but
doesn't work -- it happily ignores -show and tries to compile stuff,
then leaves us with a dependency that has no libraries, and fails when
running `ninja`.

See: #12748
@eli-schwartz
Copy link
Member

I think this was fixed in #12802

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants