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

Let .pc files and LDFLAGS provide rpaths. #7103

Merged
merged 3 commits into from
May 18, 2020

Conversation

dankegel
Copy link
Contributor

@dankegel dankegel commented May 8, 2020

Like #7075, but instead of marking the injected rpaths, remembers them.

Fixes #4027

Oh, and what the heck, fix #2567 en passant.

@dankegel dankegel requested a review from jpakkane as a code owner May 8, 2020 18:00
@dankegel dankegel force-pushed the bug4027-rpath-remember branch from 72fa946 to 0a86393 Compare May 8, 2020 19:12
@dankegel dankegel force-pushed the bug4027-rpath-remember branch from 0a86393 to 6474a7a Compare May 10, 2020 17:11
@lgtm-com
Copy link

lgtm-com bot commented May 10, 2020

This pull request introduces 1 alert when merging 6474a7a into efb8608 - view on LGTM.com

new alerts:

  • 1 for Signature mismatch in overriding method

@eli-schwartz
Copy link
Member

Is this just about pkg-config files, or is it intended to also preserve rpath which is set by e.g. LDFLAGS? The latter is a concern for yocto/buildroot/nixos/guix etc. as discussed in #2567.

@dankegel
Copy link
Contributor Author

dankegel commented May 10, 2020

I got into this because I had a buildroot-like system whose .pc files provided rpaths.

I imagine that the fix will be more broadly useful than just .pc files, though.

@eli-schwartz
Copy link
Member

eli-schwartz commented May 10, 2020

Sounds great... what do you think about using a slightly less confusing commit message, then? The PR title implies pkg-config files specifically, the commit message says "across prefix boundaries" which simply makes no sense as rpath is completely unrelated to prefix and are often, in fact, subdirectories of libdir.

@dankegel
Copy link
Contributor Author

I'd feel great about it, but I'd want to add a unit test for the particular use case in mind.

Care to suggest one?

@eli-schwartz
Copy link
Member

Seems like implementing essentially the same as test_usage_pkgconfig_prefixes, but by setting val1's project to build with env1['LDFLAGS'], might do the trick?

@dankegel
Copy link
Contributor Author

dankegel commented May 10, 2020

You mean instead of using the install_rpath argument to shared_library? Actually, I don't need those arguments at all, duh. Removing...

I did verify that a previous version of this change (#7075) rescues spack's gtkplus build when dropped in as a replacement for spack's existing kludge var/spack/repos/builtin/packages/meson/rpath-0.54.patch. spack's rpaths are long, one entry per library. Their glib.pc does specify an rpath.

I'll try adding a global LDFLAGS test per #2567 (comment)

@dankegel dankegel force-pushed the bug4027-rpath-remember branch from 6474a7a to 7b2e7be Compare May 11, 2020 00:13
@lgtm-com
Copy link

lgtm-com bot commented May 11, 2020

This pull request introduces 1 alert when merging 7b2e7be into efb8608 - view on LGTM.com

new alerts:

  • 1 for Signature mismatch in overriding method

@dankegel dankegel force-pushed the bug4027-rpath-remember branch 2 times, most recently from deacf45 to aacedb3 Compare May 11, 2020 19:30
@dankegel dankegel force-pushed the bug4027-rpath-remember branch from aacedb3 to 8eca8dd Compare May 12, 2020 16:39
@dankegel dankegel changed the title Let .pc files provide rpaths. Let .pc files and LDFLAGS provide rpaths. May 12, 2020
@dankegel dankegel force-pushed the bug4027-rpath-remember branch 2 times, most recently from 84d7f86 to 41b4e9f Compare May 12, 2020 20:28
@lgtm-com
Copy link

lgtm-com bot commented May 12, 2020

This pull request introduces 1 alert when merging 41b4e9f into 956cba0 - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

@jpakkane
Copy link
Member

From what I can tell, the rpath_dirs_to_update argument to build_rpath_args is only used as an out-parameter. This is strongly discouraged in general. We don't use out parameters at all in Meson, because they make code hard to reason about.

The better approach for this is to change the return value of the method to a tuple (rpath_args, rpath_dirs_added_to_run_from_build_dir), or words to that effect.

@dankegel
Copy link
Contributor Author

Roger, wilco. I used the out parameter out of sheer laziness in the proof-of-concept.

@jpakkane
Copy link
Member

Since this is a behavioural change, it should get a release note snippet under docs/markdown/snippets.

@dankegel dankegel force-pushed the bug4027-rpath-remember branch 2 times, most recently from c3f8137 to 4f88302 Compare May 16, 2020 19:40
@dankegel dankegel force-pushed the bug4027-rpath-remember branch from 4f88302 to f8cfb74 Compare May 16, 2020 20:27
@dankegel
Copy link
Contributor Author

All issues resolved.

@nickbroon
Copy link

Is there a work-around for older versions of meson?

I have project that makes use of external third-party libraries (via dependency() ) that provide a rpath in their .pc pkgconfig files. meson strips these rpaths, when installing my project and thus makes it unusable.
Is there some meson snippet that can extract the rpath from the dependency gotten from the .pc file, that can then be passed to install_rpath: and thus avoid the meson broken stripping behaviour?

@eli-schwartz
Copy link
Member

Why not just bump the minimum meson version?

@nickbroon
Copy link

I'm targeting a specific distribution version that only has an older version of meson available. I'm reluctant to have to backport and support a newer version of meson. (as this might impact other packages in distribution build).

@eli-schwartz
Copy link
Member

If they have an older version of meson and won't upgrade, that's because they have a stable release policy. If so, they won't accept new versions of your software either. So it really doesn't matter if you bump the minimum meson version.

People building outside of the distribution repository infrastructure can locally download the latest version of meson e.g. using pip install meson.

...You say the code currently doesn't work at all on older versions of meson. So no one is currently using it on these stale old distros. Bumping the minimum meson version doesn't prevent anyone from doing something they used to be able to do. All it does is correctly communicate the pre-existing constraints. It's definitely the correct thing to do if your code depends on this PR.

@nickbroon
Copy link

I'm not submitting my software to the stable distribution ,but my software does specifically targeted that stable distribution, and has be able to build on it. (That is the software forms part of overlay for a stable distribution with older version of meson).

As there appears to be no work around for this meson bug, I guess I'll have to include a newer version of meson as part of the overlay build collection.

@dankegel
Copy link
Contributor Author

dankegel commented Jul 8, 2020 via email

pass
for arg in args:
if arg.startswith('-Wl,-rpath='):
for dir in arg.replace('-Wl,-rpath=','').split(':'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This match pattern is insufficient, I've followed up with a fix in #7472.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants