Always make shared library IDs absolute on Darwin

Mitsos101 commented 4 years ago

Purpose of this patch

This patch aims to eliminate "relative library path" errors. [1] Relative library paths, as defined above, are designed to allow application folders to be relocated. [2] This is an unnecessary feature for pkgsrc, as binary packages are not designed to be relocatable. Homebrew [3], MacPorts [4], and Fink [5] allow relative library paths, but the rationale for this decision is not sufficiently explained. Since a relative library path is only resolvable at runtime, it is better to err on the side of caution and assume that it will not be found, as doing otherwise would defeat the purpose of check-shlibs. Nix has generally addressed the @rpath (@loader_path, @executable_path) issue, and I was also able to find a (stale) issue that is similar to this pull request. [6,7] It is trivial to fix "relative library path" errors on Nix, as it implements a generalized solution that works for any package. [8,9] pkgsrc has not satisfactorily addressed this issue: I was able to find 35 nearly identical functions that emulate Nix's solution. [10] Therefore, I have chosen to implement an approach similar to Nix's, but with only install_name_tool -id. This does not fix all of the affected packages, but it does fix the vast majority of them.

Implementation

I originally considered adding a post-install target to mk/platform/Darwin.mk, but I was unable to find any precedent for adding targets to mk/platform/*.mk. Therefore, I have chosen to implement the solution as a privileged-install-hook, given how similar check-shlibs is to the intended solution: one runs otool(1) on every shared library, whereas the other runs install_name_tool(1). An important difference is that install_name_tool modifies the libraries, whereas most other privileged-install-hooks in the mk directory do not modify any files. However, checks are also allowed to fix the files they examine: mk/check/check-perms.mk does so with CHECK_PERMS_AUTOFIX. [11] For this reason, I have decided to add a CHECK_SHLIBS_AUTOFIX variable to mk/check/check-shlibs.mk. Note that CHECK_PERMS_AUTOFIX has not seen much use: Its only use was in lang/python/extension.mk for 48 hours, before being removed for "breaking unprivileged bulk builds". [12] However, this seems to be an issue with sysutils/checkperms, and not the privileged-install-hook itself. [13] Note also @jsonn's concern with this option in PR #50649: "It is not run for PKG_DEVELOPER=no by default and errors only visible in that case are horrible...": I have attempted to address this by making it possible to useCHECK_SHLIBS_AUTOFIX without enabling PKG_DEVELOPER. [14] As this is my first contribution to pkgsrc, please excuse me for any mistakes.

References

krytarowski commented 4 years ago

@jperkin

jperkin commented 4 years ago

I like having a centralised way to fix these, but I don't agree for doing them on every package build, as the vast majority of packages do not need it. It also shouldn't be part of the mk/check infrastructure.

Could you put it behind a per-package variable, I'd suggest FIX_INSTALL_NAME, which takes a list of paths relative to ${DESTDIR}${PREFIX}, and move the logic to mk/install/install.mk? Yes, this loses some of the automation of having things fixed magically, but we generally prefer to identify issues clearly so that they can be verified at later dates to see if they are still necessary. This way would still reduce the various copy/paste post-install targets down to single line additions in each Makefile.

I'd be happy to test this in a macOS bulk build when ready. Thanks.

Mitsos101 commented 4 years ago

I like having a centralised way to fix these, but I don't agree for doing them on every package build, as the vast majority of packages do not need it.

It is impossible to know if a package needs this before check-shlibs when PKG_DEVELOPER is enabled, and before it runs if it is not. It is impossible for this change to break a package, since a package that passes check-shlibs already has absolute install names. Since it is better to prevent problems before they arise, especially if they can be solved in a generalized manner, this should be enabled for all packages.

It also shouldn't be part of the mk/check infrastructure.

Please justify this and the 2nd paragraph, having regard to CHECK_PERMS_AUTOFIX and Nix's successful implementation of fixDarwinDylibNames.

NetBSD / pkgsrc