-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix RPATH and install_name handling on macOS for uninstalled and installed binaries #3691
Conversation
ccb4e04
to
5726a9e
Compare
mesonbuild/compilers/c.py
Outdated
if isinstance(extra_dirs, str): | ||
extra_dirs = [extra_dirs] | ||
key = (tuple(self.exelist), libname, tuple(extra_dirs), code, libtype) | ||
if not key in self.find_library_cache: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Flake8]
[E713] test for membership should be 'not in'
5726a9e
to
434d702
Compare
434d702
to
837f9ad
Compare
# outputs to not be installed). | ||
custom_install_dir = True | ||
else: | ||
custom_install_dir = False |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
198725f
to
5903beb
Compare
mesonbuild/mintro.py
Outdated
res[os.path.join(installdata.build_dir, path)] = os.path.join(installdata.prefix, installdir, os.path.basename(path)) | ||
for t in installdata.targets: | ||
res[os.path.join(installdata.build_dir, t.fname)] = \ | ||
os.path.join(installdata.prefix, t.outdir, os.path.basename(t.fname)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Flake8]
[E126] continuation line over-indented for hanging indent
6893a70
to
02eb35e
Compare
CCing @ePirat @SoapGentoo @siriobalmelli @tschoonj @ryandesign @mojca @ilovezfs (based on participation in previous PRs and issues) Reviews and/or comments are welcome. Just reading the commit messages should suffice to understand what the new behaviour will be. |
This looks like a good solution to me. |
if new_rpath: | ||
subprocess.check_call(['install_name_tool', '-add_rpath', new_rpath, fname], | ||
args += ['-add_rpath', new_rpath] | ||
# Rewrite -install_name @rpath/libfoo.dylib to /path/to/libfoo.dylib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you using absolute install_name
s now instead of relative ones? This would be a major breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you using absolute install_names now instead of relative ones?
Yes, see commit message.
This would be a major breaking change
Can't break what is already broken ;)
mesonbuild/dependencies/base.py
Outdated
libpaths.append(lib[2:]) | ||
continue | ||
elif lib.startswith('-l'): | ||
args = self.compiler.find_library(lib[2:], self.env, libpaths, libtype) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My code had an error message for self.compiler is None
. I don't remember the exact code paths that triggered that but i think we need this here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I noticed that but I didn't want to add untested or potentially dead code. So I thought I'd skip it and see what breaks so we know what that case is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure it's not dead. And i don't think leaving preventable stack traces around for unsuspecting users is a good approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like preemptively adding checks for cases without understanding what the consequences of those cases are, since silently taking a bad decision is worse. I'll add an assert telling the user to file a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you try in a java only (as in meson languages) project? I know that that is nonsensical, but maybe it's an easy way to reproduce it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the code again, and this can happen if the project doesn't have a clike language in the list of languages in project()
or add_languages()
: rust
, csharp
, python
. So I re-added the check.
@@ -484,7 +484,7 @@ def setUp(self): | |||
src_root = os.path.join(os.getcwd(), src_root) | |||
self.src_root = src_root | |||
self.prefix = '/usr' | |||
self.libdir = os.path.join(self.prefix, 'lib') | |||
self.libdir = 'lib' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this save? Does any other test use self.libdir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is safe w.r.t. passing to meson. Other tests do use self.libdir
and those have been changed in the same commit.
@@ -3198,7 +3198,6 @@ def test_old_gnome_module_codepaths(self): | |||
self.build() | |||
mesonbuild.modules.gnome.native_glib_version = None | |||
|
|||
@unittest.skipIf(shutil.which('pkg-config') is None, 'Pkg-config not found.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to hard depend on pkg-config for the test suite on *nix now? I feel that this does not belong into this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to ensure that the test would not be skipped on the CI just because pkg-config wasn't available. I'll change it to skip it when we're not on CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do. I think completely removing these skips should be the topic of another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
run_unittests.py
Outdated
|
||
TODO: On Windows, this can test whether PATH is set properly | ||
PkgConfigDependency) works. On macOS, this workflow works out of the | ||
box. On Linux, BSDs, Windows, etc, you need to set extra arguments such |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That this does not work on linux and BSDs sounds more like a bug in meson that needs to be fixed?
So i think that comment is misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's not a bug. If you do not install your libraries into the runtime linker paths (ld.so.conf), you need to set the install_rpath:
or your libraries won't be found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It says """Test that uninstalled usage of an external library [...] works""". So maybe that's not what it seems to say? Not sure.
When we link to an external library either with find_library() without any dirs:, or with dependency(), we should be able to run uninstalled out of the box without having to set any environment variables or other shenanigans. This is especially important on macOS because only the system frameworks directory is in the default runtime path, and all other frameworks and libraries need to be found with RPATH or absolute path to the dylib.
02eb35e
to
7621c83
Compare
This allows us to more aggressively de-dup them, and also sets RPATHs to all libraries that are not in the system linker paths so that binaries can be run uninstalled without any special steps. These RPATHs will be wiped on install, so they do not affect reproducible builds. De-duping: Fixes #2150 Fixes #2118 Fixes #3071 RPATHs: Fixes #314 Fixes #2881 Also fixes the uninstalled usage portions of: #3038 #3077
The install name is used by consumers of the library to find the library at runtime. If it's @rpath/libfoo.dylib, all consumers must manually add the library path to RPATH, which is not what people expect. Almost everyone sets the library install name as the full path to the library, and this is done at install time with install_name_tool.
On macOS, we set the install_name for built libraries to @rpath/libfoo.dylib, and when linking to the library, we set the RPATH to its path in the build directory. This allows all built binaries to be run as-is from the build directory (uninstalled). However, on install, we have to strip all the RPATHs because they point to the build directory, and we change the install_name of all built libraries to the absolute path to the library. This causes the install name in binaries to be out of date. We now change that install name to point to the absolute path to each built library after installation. Fixes #3038 Fixes #3077 With this, the default workflow on macOS matches what everyone seems to do, including Autotools and CMake. The next step is providing a way for build files to override the install_name that is used after installation for use with, f.ex., private libraries when combined with the install_rpath: kwarg on targets.
Our appveyor configuration provides pkg-config when building for mingw, cygwin, msvc, etc. Of course, people manually running the tests won't require pkg-config.
7621c83
to
176087e
Compare
Overall this is looking good. Nice job. Eventually we will need some solution to produce relocatable app bundles. Basically this but with an actual API. :D Maybe a base option with values |
Yeah, we were talking about this on IRC today. Seems like everyone is in favour of it.
Makes sense to me, and it would be good to build that around an app-based test case (perhaps using meson to build an actual app for the macOS app store) so we know we cover the use-case correctly. |
I'm happy to help with the macOS app bundle stuff, I've spent quite a long time investigating them, so if there will be any PR or Bug tracking this, please mention me. 😄 |
What's the status of this? Is this ready to merge or do we need the toggle option first to avoid potential breakage? |
Whats needed is an option to set the install_name to something else if desired. But that can be done in another PR. IMO the current behavior of meson when installing is broken on macOS and this patch fixes it, so I do not think it would break something. |
LGTM, feel free to merge if you think this is ready. |
It would still be nice to get the old behaviour back before cutting a new release as an option. For instance, for building in Anaconda, we need the old behaviour, as it is the only way to make macOS binaries fully relocatable. |
That's not really feasible at this point because we have a week to release, and it's not clear whether just a builtin option is sufficient for that, or if we need something more. The workaround currently is to manually run |
Someone reported an issue with shared modules to me, will look into that and then merge this. |
This turned out to be something unrelated to Meson; the library the module was linking to had an incorrect |
After e3757e3, generating for gtkdapp fails with See https://travis-ci.org/jon-turney/meson-corpus-test/jobs/393722545#L1265 |
We are missing a test somewhere that a d only project can use pkgconfig dependencies.
Can we extend that to D without breaking other parts? Or does this change need a fallback that works without |
That list already contains D? And that is actually what's breaking this, because....
I looked at the code, and it looks like D doesn't have a
I guess we need to differentiate between "C/C++/Objc/Objc++/Fortran" (aka c-abi) and "languages that can consume C/C++/Objc/Objc++ libraries" (c-interop). |
This commit also seems to regress building gst-transcoder (a subproject of pitivi):
compared to:
(difference seems to be that g-ir-scanner gets passed absolute paths to libraries, rather than library names with --extra-library) |
I just wanted to mention that this change seems to mostly work for me, however I'm still experiencing issues when running |
@mojca Sounds like the LC_RPATH is not correctly set on the test executable? Could you check with |
meson-internal was introduced in Homebrew#25667 A patch was proposed upstream in mesonbuild/meson#2577, but never merged. It looks like the issues were fixed by mesonbuild/meson#3555 mesonbuild/meson#3691 mesonbuild/meson#3356 Now that we are one year later, and things have settled down, try to use meson everywhere and drop meson-internal, which is outdated.
This works around an undocumented semantic in Meson as regard dylibs' IDs. The fix performed for dyld in 2c058e5 meant that anyone attempting to use relocated libraries within Meson would have their libraries or executables crash at launch, since Meson does not insert any RPATH entries for dependencies, only for build targets in the current project. See: https://gitlab.freedesktop.org/gstreamer/cerbero/-/merge_requests/1478 mesonbuild/meson#3691 (NOTE: for a more comprehensive fix, implement the post-install step on osxuniversalgenerator.py:do_merge inside the copy-pc action.) Part-of: <https://gitlab.freedesktop.org/gstreamer/cerbero/-/merge_requests/1485>
This works around an undocumented semantic in Meson as regard dylibs' IDs. The fix performed for dyld in 2c058e5 meant that anyone attempting to use relocated libraries within Meson would have their libraries or executables crash at launch, since Meson does not insert any RPATH entries for dependencies, only for build targets in the current project. See: https://gitlab.freedesktop.org/gstreamer/cerbero/-/merge_requests/1478 mesonbuild/meson#3691 (NOTE: for a more comprehensive fix, implement the post-install step on osxuniversalgenerator.py:do_merge inside the copy-pc action.) Part-of: <https://gitlab.freedesktop.org/gstreamer/cerbero/-/merge_requests/1491>
When we link to an external library either with
find_library()
without any dirs:, or withdependency()
, we should be able to run uninstalled out of the box without having to set any environment variables or other shenanigans.This is especially important on macOS because only the system frameworks directory is in the default runtime path, and all other frameworks and libraries need to be found with
RPATH
or absolute path to the dylib.I will be adding commits that fix this next.