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

Remove -lm -ldl from hdf5.pc on macOS #4616

Closed
wants to merge 1 commit into from

Conversation

mabruzzo
Copy link

@mabruzzo mabruzzo commented Jun 30, 2024

Motivation

Prior to this commit, the Libs.private entry within hdf5.pc contain the -lm and -ldl flags on macOS. Becuase macOS (for at least a number of recent releases) doesn't specify actually contain these libraries, this means that passing the flags produced by

pkg-config --static --libs hdf5

will produce linker errors on macOS. The contents of these particular libraries are instead all consolidated within a single system library.

The Solution

It turns out that this issue originates from some "magic" that CMake does to removes these flags from all calls to the linker on macOS. This "magic" seems to cause the built-in logic for checking for the existence of these libraries to falsely report that the libraries exist on macOS. These false positives propagate to the contents of the contents of hdf5.pc. At the same time, this same "magic" seems to prevent linker-errors during the actual compilation of hdf5.

Concerns

I think that much older versions of macOS may have actually provided the "m" and "dl" libraries. If that is the case, then my fix may break builds on these versions of macOS...

I'm not really sure when/if this transition occurred and I don't know how to figure this out. If this is a serious concern, I could try to figure this out. (With that said, I don't have a ton of bandwidth for this. If there isn't an easy answer, we might just need to close this PR)

Motivation
----------

Prior to this commit, the Libs.private entry within hdf5.pc contain the
``-lm`` and ``-ldl`` flags on macOS. Becuase macOS (for at least a
number of recent releases) doesn't specify actually contain these
libraries, this means that passing the flags produced by

   ``pkg-config --static --libs hdf5``

will produce linker errors on macOS. The contents of these particular
libraries are instead all consolidated within a single system library.

The Solution
------------

It turns out that this issue originates from some "magic" that CMake
does to removes these flags from all calls to the linker on macOS. This
"magic" seems to cause the built-in logic for checking for the existence
of these libraries to falsely report that the libraries exist on macOS.
These false positives propagate to the contents of the contents of
hdf5.pc. At the same time, this same "magic" seems to prevent
linker-errors during the actual compilation of hdf5.

Concerns
--------

I think that much older versions of macOS may have actually provided the
"m" and "dl" libraries. If that is the case, then my fix may break
builds on these versions of macOS... I'm not really sure when this
transition occured.
@byrnHDF
Copy link
Contributor

byrnHDF commented Jul 1, 2024

I am currently looking into this and fixing a number of issues - are you including any compression libs? Because they are incorrect for static linking. Also, I believe the dl library shouldn't be in the link line for static anyway.

@mabruzzo
Copy link
Author

mabruzzo commented Jul 1, 2024

I am currently looking into this and fixing a number of issues - are you including any compression libs? Because they are incorrect for static linking. Also, I believe the dl library shouldn't be in the link line for static anyway.

Yeah I was -- I noticed that was an issue. I should have been more clear in the PR.

For some added context, I was testing out a pkg-config file that I was thinking about providing for a downstream library (that internally employs hdf5) using the copy of hdf5 installed with homebrew. While doing this, I saw that there were linker errors associated with -lm and -ldl and I thought that might be the only issue, which prompted me to submit this PR.

After making the changes for the PR, I noticed that the revised pkg-config file still complains about -l/path/to/libz.tbd. I'm relatively new to macOS (coming from Linux) and I have no idea how to properly deal with a tbd file so there is no easy way for me to fix that. Now that I look at the hdf5.pc that comes with the homebrew installation, I'm also a little skeptical that macOS's ld implementation will properly handle -l/opt/homebrew/lib/libsz.dylib (but I could be wrong).

Bottom line: this PR is really just a partial fix (I figured something could be more useful that just opening an issue). I should have really been more clear about that in the PR description. Sorry about that! If you're working on this stuff, feel free to close my PR.

At the end of the day, I've decided against relying upon this, anyways, since most of my libraries' users are on Linux (and the Libs.private line is stripped out of the hdf5.pc file shipped in the commonly used hdf5 Debian package)

@byrnHDF
Copy link
Contributor

byrnHDF commented Jul 1, 2024

This is useful and hopefully I will get #4613 fixed this week. Then we can decide on this PR (which looks like I will close then)

If you run across anything else or have any suggestions - please comment on #4613 if applicable.

@derobins derobins added Merge - To 1.14 Priority - 2. Medium ⏹ It would be nice to have this in the next release Component - Build CMake, Autotools Type - Improvement Improvements that don't add a new feature or functionality labels Jul 1, 2024
@byrnHDF
Copy link
Contributor

byrnHDF commented Jul 1, 2024

Closing because #4613 will cover issue.

@byrnHDF byrnHDF closed this Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Build CMake, Autotools Priority - 2. Medium ⏹ It would be nice to have this in the next release Type - Improvement Improvements that don't add a new feature or functionality
Projects
Status: Merges Complete
Development

Successfully merging this pull request may close these issues.

3 participants