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

[conan-center] KB-H071 Validate relocable shared libraries #403

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

uilianries
Copy link
Member

@uilianries uilianries commented Mar 8, 2022

  • Check KEEP_RPATH on CMakeLists.txt

closes #376

@SpaceIm
Copy link
Contributor

SpaceIm commented Mar 9, 2022

The check should be more powerful.
KEEP_RPATHS may not be sufficient (and limited to CMake based recipes), it depends whether upstream is doing install_name babysitting.
This hook should inspect produced dylib, there is no other choice: see #376 (comment).
In this proposal, there are 2 hooks actually:

  • check with otool that install_name is @rpath in shared libs on macOS (in the name of LC_ID_DYLIB cmd)
  • check that RUNPATH/RPATH/LC_RPATH is empty (all platforms supporting rpath: linux, macos etc).

The second one should not be implemented for the moment because it's too hard to honor this requirement in conan v1 when pkg_config generator is used in a recipe. This issue comes from conan-io/conan#7878, which has been fixed in PkgConfigDeps generator by conan-io/conan#10192 but not in pkg_config generator (requested by @memsharded to avoid breaking change: conan-io/conan#10192 (comment)).

@SSE4
Copy link
Contributor

SSE4 commented Mar 9, 2022

yes, take a look at #171, it inspects dylib/otool output
I think it should validate the binaries itself, not the cmake files (as there are other build systems than cmake, that may potentially suffer exactly that issue - maybe autotools, meson, etc).

@SpaceIm
Copy link
Contributor

SpaceIm commented Mar 9, 2022

It's worth noting that I still don't know how to install shared libs with @rpath install_name with Meson (currently, all Meson based recipes put absolute path in install_name, like autotools, which is bad).

@SSE4
Copy link
Contributor

SSE4 commented Mar 9, 2022

It's worth noting that I still don't know how to install shared libs with @rpath install_name with Meson (currently, all Meson based recipes put absolute path in install_name, like autotools, which is bad).

maybe we can reach to meson developers, in the past, they were friendly enough to help us
but in general, my point that hook should be irrelevant to the build system

@SpaceIm
Copy link
Contributor

SpaceIm commented Mar 9, 2022

but in general, my point that hook should be irrelevant to the build system

Agree, I just want to say that before adding any hook in conan-center, we should ensure that we have a workaround for all build systems, otherwise it may block PR of several recipes.

@SSE4
Copy link
Contributor

SSE4 commented Mar 9, 2022

Agree, I just want to say that before adding any hook in conan-center, we should ensure that we have a workaround for all build systems, otherwise it may block PR of several recipes.

I worry, this one (and #171) could be enabled only as warnings.
reason - we don't have anything to check post_package hook for all the packages, so it's hard to estimate the amount of damage before enabling it. I would personally say, the majority of libraries are still not prepared for that, so we don't want to block new contributors doing PRs (like bump version) forcing them to fix that complex errors.
it's techinically possible to run something that downloads all the packages and runs a hook against a package folder, but it will take hours (if not days) to finish :(

@SpaceIm
Copy link
Contributor

SpaceIm commented Mar 9, 2022

Regarding Meson, I asked on Slack but never get any answer: https://cpplang.slack.com/archives/C01AVS654P8/p1643164802012000

This PR seems to have removed any capability to install relocatable shared libs on macOS with Meson: mesonbuild/meson#3691
I don't know whether an option is available to change install_name.

EDIT: I've opened mesonbuild/meson#10099

@SSE4
Copy link
Contributor

SSE4 commented Mar 26, 2022

@uilianries conflicts, please rebase

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.

[conan-center] Add a hook to check whether installed shared libs are relocatable on Linux & macOS
3 participants