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

drop rpath patch #111

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

drop rpath patch #111

wants to merge 1 commit into from

Conversation

holymonson
Copy link

This rpath patch was introduced to fix glib-feedstock/issues/40. As stated in e655eb2, this is no longer needed, it will cause meson to skip fixing some should-be-fix RPATH on macos, making meson behaving differently from other packaging system.

If someone does need this, it should be submitted to upstream instead of keeping it as privite patch in conda-forge.

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-admin, please rerender

This rpath patch was introduced to fix glib-feedstock/issues/40.
As stated in e655eb2, this is no longer needed, it will cause meson
to skip fixing some should-be-fix RPATH on macos, making meson behaving
differently from other packaging system.

If someone does need this, it should be submitted to upstream instead of
keeping it as privite patch in conda-forge.
@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

@holymonson
Copy link
Author

cc @pkgw

Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/meson-feedstock/actions/runs/10158768838.

Copy link
Contributor

@pkgw pkgw left a comment

Choose a reason for hiding this comment

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

It's been a long time since I've thought about this, so if there was subsequent work that we think makes it so that this patch is no longer needed, I'm inclined to trust that.

@h-vetinari
Copy link
Member

Well, far be it for me to contradict @pkgw on this, but I did check out the old thread in conda-forge/glib-feedstock#40, and the upstream issue for this is still open (mesonbuild/meson#4685). Any thoughts @rgommers @eli-schwartz?

I think it might be easiest/safest for now to point this PR to the dev branch, and release the builds from that into a separate label (which is already set up in the dev branch), and then we can test the cases like glib (and others if possible) where we expect this to potentially run into issues.

@eli-schwartz
Copy link

eli-schwartz commented Aug 2, 2024

Meson hasn't changed its handling. We still create binaries with rpaths, and in the common case we then delete the rpath entirely. This can be seen with basically any package that installs both a library installed to /usr/lib64 and a binary.

At build time, the binary has to be runnable via rpath pointing into the build directory. At install time, it cannot have an rpath (that points to the build directory! having one pointing to an official install location is fine), since this is a security vulnerability to have one.

There are two ways to do this. One of them is to edit the rpath, the other is to veeeeeeeeeeeeeery slooooooooooooooooooooooowly rebuild anything with an rpath at install time.

As far as I am aware, we've never had any real bug report about it. There is the bug report by @pkgw, but it cites the patchelf program, which performs much more sophisticated rewriting than meson does -- because meson's needs are very simple, and all it has to do is back out of changes it has made, whereas patchelf performs extensive section reordering in order to hack the ELF format and squeeze in some space that never formerly existed, because it needs that space to write potentially very long rpaths etc.

patchelf is a fully generic ELF rewriter! It can add new DT_NEEDED entries as well as create an rpath if one never existed, or infinitely extend it. Every time you run patchelf on a binary, it grows in size, and if you do it enough times it hits a ceiling and corrupts the living daylights out of that binary. It can do some very clever things before then, however, especially if you know exactly what to change it to and only change it once. :)

It is unclear to me from the meson ticket, why meson's more limited/targeted rewriting is a problem, but also, why only conda reports it as one.

@rgommers
Copy link
Contributor

rgommers commented Aug 2, 2024

Meson hasn't changed its handling.

It has had an important fix though, after the linked issues for the patch under discussion here, namely mesonbuild/meson#7103.

As far as I am aware, we've never had any real bug report about it .... why only conda reports it as one.

mesonbuild/meson#13046 comes to mind - the current behavior isn't problem-free. And it wouldn't be too surprising that an issue shows up just for Conda, since it has a fairly unique setup with libdir not being on the default search path for ld and conda-forge's compiler activation inserting -Wl,-rpath,,/path/to/env/libdir/ unconditionally.

it will cause meson to skip fixing some should-be-fix RPATH on macos, making meson behaving differently from other packaging system.

@holymonson is this PR triggered by a real-world issue?

@pkgw
Copy link
Contributor

pkgw commented Aug 2, 2024

There are two ways to do this. One of them is to edit the rpath, the other is to veeeeeeeeeeeeeery slooooooooooooooooooooooowly rebuild anything with an rpath at install time.

This is getting off on a side track, but since this is something that I've always wanted to see ... there's no need to rebuild at install time, since at build time you know what the install prefix is going to be. During the main build you could make two versions of every executable, one with RPATHs for uninstalled execution, and one with RPATHs for final installation. Maybe use the "usual" name for the in-tree version (path/to/program) and something hidden for the to-be-installed version (path/to/.program.install). As an optimization, the build tool probably has all the information it needs to detect when one binary will suffice. It's true that you wouldn't be testing exactly the binary that you're installing, but if you're rewriting RPATHs, that's already true.

I suspect that users would find this somewhat annoying and confusing, but it feels like the most formally correct approach to me.

FWIW the discussion of the related NixOS/patchelf#44 indicates that the underlying ldconfig bug has been fixed, but as other bug discussions mention, it will probably be a very long time before that fix propagates out through the entire ecosystem.

(edited to add: there's also libtool's approach: build the executables with RPATH settings needed for final installation; but then hide the executables; and basically trick users into running wrapper scripts that futz with the environment for local testing. But I doubt that anyone is super excited about that option these days.)

@holymonson
Copy link
Author

@holymonson is this PR triggered by a real-world issue?

Yes, it takes me some time to trace "Skipping RPATH fixing" down to here, because it doesn't exist in other meson from other packaging system or the source code.
To be clear, I'm using conda to set up an isolated build tool environment to build something outside conda, not conda related staff. It is expected to fix RPATHs to absolute linking paths, like other-edition meson do, but now it leaves me some .dylib with missing linkable.

As for building conda staff, I manually tested building libsoup-feedstock and glib-feedstock, with this PR or no, conda-build at its packing stage will always turn absolute linking paths (back) into RPATHs, that can explain why few issues have been reported, because this conda-edition meson functions well as long as it's used only inside the conda build system.

@eli-schwartz I mainly work on macos and didn't encounter those elf issues, but according to mesonbuild/meson@d7235c5 and mesonbuild/meson@c2f1d91, those redundant fixes you worry about seems will be skipped in fix_elf() eventually.

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

Successfully merging this pull request may close these issues.

5 participants