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

autogen: fix patchelf invocation to remove generic build path from rpath #106758

Closed
wants to merge 1 commit into from

Conversation

siscia
Copy link
Contributor

@siscia siscia commented Dec 12, 2020

Refers to #106165

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@siscia siscia changed the title fix patchelf invocation to remove generic build path from rpath autogen: fix patchelf invocation to remove generic build path from rpath Dec 12, 2020
@siscia siscia closed this Dec 12, 2020
@siscia siscia reopened this Dec 12, 2020
Copy link
Contributor

@ztzg ztzg left a comment

Choose a reason for hiding this comment

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

LGTM.

(Hit this when rebuilding a number of packages on RHEL with sandboxing disabled.)

@Dridus
Copy link
Contributor

Dridus commented Apr 14, 2021

LGTM. this resolved an issue I had with cross compiling to armv7 on a WSL+Debian+Nix build platform

@cpakkala
Copy link

Had the same problem and this fixed it. I think we, who have to compile our systems (non-standard nix dir location) without sandboxing (work permission restrictions; no I can't use proot or the like; big brother doesn't allow user namespace in kernel) are, and forever will be, the red-headed step-children.

@SuperSandro2000
Copy link
Member

/rebase staging

@SuperSandro2000 SuperSandro2000 added this to WIP in Staging via automation May 23, 2021
@github-actions
Copy link
Contributor

Failed to rebase

Copy link
Contributor

@r-burns r-burns left a comment

Choose a reason for hiding this comment

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

Thanks! This fixes the linked issue for me. Agree with Sandro that this should be rebased onto staging due to the quantity of linux rebuilds. Otherwise LGTM.

I think we, who have to compile our systems [...] are, and forever will be, the red-headed step-children.

While I certainly wouldn't expect first-class support, I think there are enough of us out there that if we stick up for each other and nip these issues in the bud, we'll have a pretty good user experience. From what I've seen, issues like this are rare and fairly easy to patch around.

@@ -79,7 +79,7 @@ stdenv.mkDerivation rec {
'' + stdenv.lib.optionalString (!stdenv.hostPlatform.isDarwin) ''
# remove /build/** from RPATHs
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# remove /build/** from RPATHs
# remove build directory (/build/**, or /tmp/nix-build-**) from RPATHs

might make it a little more clear why this change was needed

@siscia
Copy link
Contributor Author

siscia commented Jun 10, 2021

Guys please don't expect me to work on this.

If somebody want to take charge of this PR, please go ahead with any means, it just won't be me.

@r-burns
Copy link
Contributor

r-burns commented Jun 11, 2021

Yeah no worries, thanks for sharing this patch. Continued in #126557

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

Successfully merging this pull request may close these issues.

None yet

7 participants