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

boost: patch for INTERFACE_LINK_LIBS bug #67615

Closed

Conversation

scpeters
Copy link
Member

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with 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 brew install <formula>)?

In boost 1.75.0, the cmake INTERFACE_LINK_LIBRARIESS now include non-boost dependencies like libicudata from the
keg-only formula icu4c. This introduces linking errors for software that previously linked successfully in brew against boost 1.74.0 shared libraries (see #67427 and boostorg/boost_install#47).

This adds a patch to the boost formula that has been merged upstream to limit the expansion of INTERFACE_LINK_LIBRARIES to boost's static libraries, so that the linking errors with shared libraries are resolved. I've tested it locally to confirm that it resolves my issues documented in osrf/homebrew-simulation#1235.

@carlocab
Copy link
Member

Thanks. Can you also include a comment before the patch about when we can expect it to be removed?

@scpeters
Copy link
Member Author

Thanks. Can you also include a comment before the patch about when we can expect it to be removed?

I added a comment that it can be removed with the next release in b45380c

SMillerDev
SMillerDev previously approved these changes Dec 24, 2020
@carlocab carlocab mentioned this pull request Dec 25, 2020
5 tasks
@carlocab
Copy link
Member

carlocab commented Dec 25, 2020

I've opened a PR to fix cucumber-cpp: #67689

Unfortunately, I think it'll only fix the failure on Big Sur and not Mojave.

@carlocab
Copy link
Member

I can't reproduce the pcl error locally (instead I am getting an entirely different build error I can't seem to fix), so my best guess given the error is to add

ENV.prepend_path "PKG_CONFIG_PATH", Formula["eigen"].opt_share/"pkgconfig"

to the start of pcl's test block. (Yes, that's where eigen's pkgconfig directory is. I don't know why either.)

@scpeters
Copy link
Member Author

I can't reproduce the pcl error locally (instead I am getting an entirely different build error I can't seem to fix), so my best guess given the error is to add

ENV.prepend_path "PKG_CONFIG_PATH", Formula["eigen"].opt_share/"pkgconfig"

to the start of pcl's test block. (Yes, that's where eigen's pkgconfig directory is. I don't know why either.)

I believe it's a bug in homebrew-test-bot. I noticed in the console log of these failing jobs that just before freeling is tested, it calls brew unlink eigen because freeling conflicts with eigen. It remains unlinked when later testing pcl, which fails. Explicitly adding eigen's opt folder to the pkgconfig path would work around this

This didn't fail in #66792, because pcl had a revision bump in that PR, so it took a different code path in test-bot

@scpeters
Copy link
Member Author

This didn't fail in #66792, because pcl had a revision bump in that PR, so it took a different code path in test-bot

I'll finish the thought here. I looked in the logs of #66792 and noticed the same brew unlink eigen before freeling but also a brew link eigen before pcl was built. I believe this is because there is logic for matching brew unlink for conflicting formulae with calls to brew link for unlinked dependencies (see setup_formulae_deps_instances() in lib/tests/formulae.rb).

By contrast, I believe pcl is handled as a dependent in this PR (possibly in this code?), which doesn't check for unlinked dependencies

@scpeters
Copy link
Member Author

I believe it's a bug in homebrew-test-bot

I'll file an issue for this

@scpeters
Copy link
Member Author

I believe it's a bug in homebrew-test-bot

I'll file an issue for this

Homebrew/homebrew-test-bot#544

@scpeters
Copy link
Member Author

I can't reproduce the pcl error locally (instead I am getting an entirely different build error I can't seem to fix), so my best guess given the error is to add

ENV.prepend_path "PKG_CONFIG_PATH", Formula["eigen"].opt_share/"pkgconfig"

to the start of pcl's test block. (Yes, that's where eigen's pkgconfig directory is. I don't know why either.)

I've rebased and added this workaround in 92701db

@carlocab
Copy link
Member

carlocab commented Dec 26, 2020

On 11.0

make[2]: *** No rule to make target `/Library/Developer/CommandLineTools/SDKs/MacOSX11.0.sdk/usr/lib/libz.tbd', needed by `lib/libpcl_surface.1.11.1.dylib'.  Stop.

One of pcl's (possibly recursive) dependencies has the SDK path baked into its compiled artifacts. It will need to be rebuilt. I believe it's vtk@8.2 (or dependencies).

Cf. #67474, Homebrew/brew#10127

Update: Tried rebuilding only vtk@8.2. Didn't cut it on its own, sadly. Will look into it some more.

@scpeters
Copy link
Member Author

On 11.0

make[2]: *** No rule to make target `/Library/Developer/CommandLineTools/SDKs/MacOSX11.0.sdk/usr/lib/libz.tbd', needed by `lib/libpcl_surface.1.11.1.dylib'.  Stop.

One of pcl's (possibly recursive) dependencies has the SDK path baked into its compiled artifacts. It will need to be rebuilt. I believe it's vtk@8.2 (or dependencies).

Cf. #67474, Homebrew/brew#10127

Update: Tried rebuilding only vtk@8.2. Didn't cut it on its own, sadly. Will look into it some more.

I noticed a reference to MacOSX11.0.sdk in the netcdf bottle, so I've rev-bumped that and vtk@8.2, and we'll see if that helps

@carlocab carlocab mentioned this pull request Dec 31, 2020
5 tasks
@carlocab
Copy link
Member

carlocab commented Dec 31, 2020

Tried building pcl locally again.

The only other dependency with zlib linkage is flann, so I guess that's worth giving a revision bump.

vtk@8.2's dependencies with zlib linkage:

  • hdf5
  • libpng
  • libtiff
  • netcdf
  • qt

I've also looked whether we need to recurse further down the dependency tree from there, and it (thankfully) doesn't look like it.

scpeters and others added 5 commits January 1, 2021 22:37
In boost 1.75.0, the cmake INTERFACE_LINK_LIBRARIESS now
include non-boost dependencies like libicudata from the
keg-only formula icu4c. This introduces linking errors
for software that previously linked successfully in brew
against boost 1.74.0 shared libraries (see Homebrew#67427 and
boostorg/boost_install#47).

This adds a patch to the boost formula that has been merged
upstream to limit the expansion of INTERFACE_LINK_LIBRARIES
to boost's static libraries, so that the linking errors
with shared libraries are resolved.
@carlocab carlocab force-pushed the boost_interface_link_libs_patch branch from 361cd48 to 0b901a7 Compare January 1, 2021 22:37
@carlocab
Copy link
Member

carlocab commented Jan 1, 2021

Rebuilding flann seems to help, so I've given it a revision bump and rebased this against master.

pcl is still compiling as I write this, but it's gotten well past the point where CI failed now.

@carlocab
Copy link
Member

carlocab commented Jan 2, 2021

Ok, no idea what happened here, but the CI run shouldn't be taking this long. Will try to restart the tests.

@mitchblank
Copy link
Contributor

This makes me worried about my own mariadb-connector-odbc change 30e214e

I added the reference to the .tbd file because the project's cmake files expected some physically-present file for that setting (so on Big Sur /usr/lib/libiconv.dylib won't work, nor was it happy with just -liconv) It seemed that passing in the tbd file to the linker would be a harmless way of getting the -liconv behavior since the the tbd file is just a description of how to link to the library, but I guess it's possible for it to end up with the tbd as a runtime dependency?!?

@carlocab
Copy link
Member

carlocab commented Jan 3, 2021

CI is green on the Intel Big Sur runner now. Except that the Catalina runner seems to have gotten stuck somewhere...

@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

@scpeters scpeters deleted the boost_interface_link_libs_patch branch January 4, 2021 04:46
@scpeters
Copy link
Member Author

scpeters commented Jan 4, 2021

thanks for the help @carlocab!

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 4, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants