hooks icon indicating copy to clipboard operation
hooks copied to clipboard

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

Open uilianries opened this issue 3 years ago • 8 comments

  • [x] Check KEEP_RPATH on CMakeLists.txt

closes #376

uilianries avatar Mar 08 '22 16:03 uilianries

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 https://github.com/conan-io/hooks/issues/376#issuecomment-1021424535. 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 https://github.com/conan-io/conan/issues/7878, which has been fixed in PkgConfigDeps generator by https://github.com/conan-io/conan/pull/10192 but not in pkg_config generator (requested by @memsharded to avoid breaking change: https://github.com/conan-io/conan/pull/10192#discussion_r771426868).

SpaceIm avatar Mar 09 '22 11:03 SpaceIm

yes, take a look at https://github.com/conan-io/hooks/pull/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).

SSE4 avatar Mar 09 '22 12:03 SSE4

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).

SpaceIm avatar Mar 09 '22 12:03 SpaceIm

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

SSE4 avatar Mar 09 '22 12:03 SSE4

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.

SpaceIm avatar Mar 09 '22 12:03 SpaceIm

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 :(

SSE4 avatar Mar 09 '22 12:03 SSE4

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: https://github.com/mesonbuild/meson/pull/3691 I don't know whether an option is available to change install_name.

EDIT: I've opened https://github.com/mesonbuild/meson/issues/10099

SpaceIm avatar Mar 09 '22 13:03 SpaceIm

@uilianries conflicts, please rebase

SSE4 avatar Mar 26 '22 09:03 SSE4