Skip to content

Comments

make{,Binary}Wrapper: some cleanups#172366

Merged
thiagokokada merged 8 commits intoNixOS:stagingfrom
ncfavier:wrappers-cleanup
May 12, 2022
Merged

make{,Binary}Wrapper: some cleanups#172366
thiagokokada merged 8 commits intoNixOS:stagingfrom
ncfavier:wrappers-cleanup

Conversation

@ncfavier
Copy link
Member

@ncfavier ncfavier commented May 10, 2022

Nothing crazy, see individual commits.

Fixes #172249, addresses #172320 (comment)

@github-actions github-actions bot added 6.topic: vim Advanced text editor 8.has: documentation This PR adds or changes documentation labels May 10, 2022
This was referenced May 10, 2022
Closed
Merged
@ncfavier ncfavier requested a review from bergkvist May 10, 2022 18:22
dotlambda
dotlambda reviewed May 10, 2022
View reviewed changes
@ncfavier ncfavier force-pushed the wrappers-cleanup branch from 44b378b to 519c054 Compare May 10, 2022 18:50
thiagokokada
thiagokokada reviewed May 10, 2022
View reviewed changes
thiagokokada
thiagokokada reviewed May 10, 2022
View reviewed changes
Copy link
Contributor

@thiagokokada thiagokokada left a comment

Choose a reason for hiding this comment

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

Some general comments, but looks like a very good clean-up 👏 .

@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels May 10, 2022
ncfavier added 5 commits May 10, 2022 22:07
@ncfavier
makeBinaryWrapper: move into its own folder
0d6bcb5 
The derivation is complex enough to warrant moving out of
all-packages.nix
@ncfavier
makeWrapper,makeBinaryWrapper: introduce explicitly named functions
6224594 
Because both versions might end up in a derivation's build inputs, it
might be useful to be able to explicitly select which function to use.
@ncfavier
wrapGAppsHook: rename argument to makeWrapper
81b9c71 
...and pass it makeBinaryWrapper in all-packages.nix
@ncfavier
makeBinaryWrapper: remove cc from deps
8b79ef2 
Fixes NixOS#172249
@ncfavier
makeWrapper: implement --inherit-argv0
2ae6911 
For symmetry/interoperability with makeBinaryWrapper. Implemented as
--argv0 '$0'
@ncfavier ncfavier force-pushed the wrappers-cleanup branch from 519c054 to 2ae6911 Compare May 10, 2022 20:08
@ncfavier
Copy link
Member Author

ncfavier commented May 11, 2022

Failures are due to timeout building LLVM, but based on my tests in #172257 this should work on every platform.

thiagokokada
thiagokokada approved these changes May 11, 2022
View reviewed changes
Copy link
Contributor

@thiagokokada thiagokokada left a comment

Choose a reason for hiding this comment

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

Code-wise, LGTM.

Not tested, would like a second opinion here.

bergkvist
bergkvist approved these changes May 11, 2022
View reviewed changes
Copy link
Member

@bergkvist bergkvist left a comment

Choose a reason for hiding this comment

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

The changes to makeShellWrapper and makeBinaryWrapper look good to me, assuming you were able to run the tests successfully for x86-linux, x86-darwin and aarch64-linux locally.

@ncfavier
Copy link
Member Author

ncfavier commented May 11, 2022

I built and ran firefox-bin succesfully. Building discord now, but it takes a while.

@Artturin Artturin added the 12.approvals: 2 This PR was reviewed and approved by two persons. label May 11, 2022
@bergkvist bergkvist mentioned this pull request May 11, 2022
Open
@ncfavier
Copy link
Member Author

ncfavier commented May 11, 2022

Built and ran firefox-bin, discord, vscode, signal, neovim and mpv.

@ncfavier ncfavier mentioned this pull request May 11, 2022
Closed
@ncfavier
Copy link
Member Author

ncfavier commented May 11, 2022

Firefox still working.

@ncfavier
Copy link
Member Author

ncfavier commented May 11, 2022

I keep thinking of more things 😅 this is just a naming change though.

@thiagokokada
Copy link
Contributor

thiagokokada commented May 11, 2022

Well, tell me when you're ready. I was going to avoid merging this until 22.05 is released, but we still have some time and the changes seem safe-ish.

@ncfavier
Copy link
Member Author

ncfavier commented May 11, 2022

I think this is fine to merge now.

@bergkvist
Copy link
Member

bergkvist commented May 11, 2022 •
edited
Loading

With the extractCmd you added, the path should probably be escaped properly here (in case it contains spaces):
https://github.com/NixOS/nixpkgs/blob/71637916319afa4aa752fa158f4eca392c937069/pkgs/build-support/setup-hooks/make-binary-wrapper/make-binary-wrapper.sh#L336-L337

Like what is happening here:
https://github.com/NixOS/nixpkgs/blob/71637916319afa4aa752fa158f4eca392c937069/pkgs/build-support/setup-hooks/make-binary-wrapper/make-binary-wrapper.sh#L391

@ncfavier
makeBinaryWrapper: add extractCmd
8836999 
A small shell script that can be used to extract a binary wrapper's
makeCWrapper call from its embedded docstring, without depending on
makeBinaryWrapper.
@ncfavier ncfavier force-pushed the wrappers-cleanup branch from 7163791 to 2b0c728 Compare May 11, 2022 23:44
@ncfavier
Copy link
Member Author

ncfavier commented May 11, 2022 •
edited
Loading

Good catch!

Firefox still working.

@dotlambda dotlambda mentioned this pull request May 12, 2022
Closed
13 tasks
@ncfavier
makeShellWrapper: add explicitly named attribute
3c77d36 
So that things that use the makeShellWrapper/wrapProgramShell
functions can depend on makeShellWrapper explicitly, which should ease
migration in the future.
@ncfavier ncfavier force-pushed the wrappers-cleanup branch from 2b0c728 to 3c77d36 Compare May 12, 2022 09:01
SuperSandro2000
SuperSandro2000 reviewed May 12, 2022
View reviewed changes
@ncfavier
Copy link
Member Author

ncfavier commented May 12, 2022 •
edited
Loading

The --append-flags implementation is ready, but I'll make another PR for that to not delay this one forever.

@thiagokokada thiagokokada merged commit 5c51876 into NixOS:staging May 12, 2022
@ncfavier ncfavier deleted the wrappers-cleanup branch May 12, 2022 21:17
@zaphar zaphar mentioned this pull request May 22, 2022
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dotlambda dotlambda dotlambda left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@thiagokokada thiagokokada thiagokokada approved these changes

@jonringer jonringer Awaiting requested review from jonringer

@teto teto Awaiting requested review from teto

@mweinelt mweinelt Awaiting requested review from mweinelt

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

+1 more reviewer

@bergkvist bergkvist bergkvist approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: vim Advanced text editor 8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 12.approvals: 2 This PR was reviewed and approved by two persons.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ncfavier @thiagokokada @bergkvist @dotlambda @SuperSandro2000 @Artturin