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

bootstrap: on NixOS, also patchelf libexec/*, just like bin/*. #104938

Closed
wants to merge 1 commit into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Nov 26, 2022

This is identical to the change nixpkgs had to make to their rustup auto-patching:

However, somehow this got missed for months in rust-lang/rust, after the RA proc macro libexec change.

r? @nagisa cc @fasterthanlime

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 26, 2022
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

This should also update the Rust code which runs patchelf.

@eddyb eddyb marked this pull request as draft November 26, 2022 15:36
if f.read(2) == b"#!":
continue

self.fix_bin_or_dylib(bin_file)
Copy link
Member

Choose a reason for hiding this comment

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

I mildly remember seeing this exact pattern before. Can’t tell if it was here, or in nixpkgs. Either way something like this seems fine to me, despite the apparent brittleness in the way patchable binaries are being detected. It would seem to me that an ideal approach would filter all ELF-looking files within . with the execute permissions and try to patch those, since that’s pretty much the intent behind this code (and would transparently cover both DSOs and executables alike…)

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear, the shell filtering just removes patchelf printing a warning, it's otherwise a noop on non-ELF files, it's just an aesthetic fix for the output, I can remove the filtering entirely if that seems better.

(Not filtering would also mean easier to port the logic to Rust - which @jyn514 told me about, I had no idea this got duplicated)

@jyn514
Copy link
Member

jyn514 commented Nov 29, 2022

I'm going to close this in favor of #105068, which already has the work done to modify the rust code.

@jyn514 jyn514 closed this Nov 29, 2022
@eddyb eddyb deleted the nixos-ra-proc-macros branch December 1, 2022 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants