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

autoPatchelfHook: move multiline hook into a function #295099

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

Qyriad
Copy link
Member

@Qyriad Qyriad commented Mar 11, 2024

In NixOS/nixpkgs#290081 stdenv: log hooks as they run it came to attention that autoPatchelfHook is one of if not the only hook in Nixpkgs that is a multiline string expression. Almost all hooks are functions, which guard with something like if [ -z "${dontDoTheThing-}" ]; then ... in the function, or single-line strings which include that guard inline and then call the real function, e.g. if [ -z "${dontDoTheThing-} ]; then doTheThing; fi.

This commit moves autoPatchelfHook to the former, which seems to be the most common style now.

Description of changes

There should be no functional changes.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
    • Mostly. autoPatchelfHook is largely used for proprietary packages, some of which rely on requireFile; I skipped those, and nixpkgs-review supposedly queued a great deal many other packages for build but didn't actually build them. Regardless, all packages that were attempted built did build.
  • (N/A) Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (N/A) (Package updates) Added a release notes entry if the change is major or breaking
    • (N/A) (Module updates) Added a release notes entry if the change is significant
    • (N/A) (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@Qyriad Qyriad requested a review from layus as a code owner March 11, 2024 19:48
@Qyriad
Copy link
Member Author

Qyriad commented Mar 11, 2024

cc @philiptaron, since you were interested in this.

Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

Couple of naming and comment-related things to consider. I'm inclined to accept if they're explained or addressed.

pkgs/build-support/setup-hooks/auto-patchelf.sh Outdated Show resolved Hide resolved
pkgs/build-support/setup-hooks/auto-patchelf.sh Outdated Show resolved Hide resolved
pkgs/build-support/setup-hooks/auto-patchelf.sh Outdated Show resolved Hide resolved
In NixOS#290081 it came to attention that autoPatchelfHook is
one of if not the only hook in Nixpkgs that is a multiline string
expression. Almost all hooks are functions, which guard with something
like `if [ -z "${dontDoTheThing-}" ]; then ...` in the function, or
single-line strings which include that guard inline and then call the
real function, e.g. `if [ -z "${dontDoTheThing-} ]; then doTheThing; fi`.

This commit moves autoPatchelfHook to the former, which seems to be the
most common style now.
@Qyriad Qyriad force-pushed the maint/autopatchelf-single-line branch from 3ed4aa6 to 2ad2295 Compare March 17, 2024 15:49
@Qyriad
Copy link
Member Author

Qyriad commented Mar 17, 2024

Thanks @philiptaron; fixed those up now!

@layus layus changed the base branch from master to staging March 18, 2024 09:15
@layus
Copy link
Member

layus commented Mar 18, 2024

Changing the PR base went smoothly. This is a trivial change (but a mass rebuild nonetheless). Merging.
Thanks for your work :-)

@layus layus merged commit fb1539a into NixOS:staging Mar 18, 2024
23 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants