conan-io / hooks

Official Conan client hooks
MIT License
32 stars 46 forks source link

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

Open uilianries opened 2 years ago

uilianries commented 2 years ago

closes #376

SpaceIm commented 2 years ago

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:

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

SSE4 commented 2 years ago

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

SpaceIm commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

SSE4 commented 2 years ago

@uilianries conflicts, please rebase