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.

Sign up for GitHub

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

Jump to bottom

buildFHSEnv: implement overriding #358977

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

buildFHSEnv: implement overriding #358977

wants to merge 3 commits into from

Conversation

zimward
Copy link
Contributor

@zimward zimward commented Nov 25, 2024 •
edited
Loading

The primary function has been moved to bwScript.nix to reduce the amount of indentation.

I am planing to refactor this in the next few weeks, but this should be good to go for now.

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
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (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.

@nix-owners nix-owners bot requested a review from philiptaron November 25, 2024 12:20
@zimward zimward force-pushed the fully-hysterical-screaming branch from 5560c72 to e34ca12 Compare November 25, 2024 12:20
@zimward
Copy link
Contributor Author

zimward commented Nov 25, 2024

I didn't run nixfmt against bwScript.nix because i only renamed it. Should i do so regardless to fix the CI check?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/buildfhsenv-overriding/56457/3

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 26, 2024
@Atemu
Copy link
Member

Atemu commented Nov 26, 2024

No need to format, we'll just let that happen when nixfmt is in a reasonably complete state and the big reformat happens.

@Atemu Atemu requested a review from K900 November 26, 2024 10:54
@K900
Copy link
Contributor

K900 commented Nov 26, 2024

And if you really want to reformat it, please do so in a separate commit so it's actually possible to review.

@zimward
Copy link
Contributor Author

zimward commented Nov 26, 2024

Im not planning to do so as I haven't changed anything in there, but I wasn't sure about how important 100% passed CI is for some committers, which is why I asked.

@Atemu
Copy link
Member

Atemu commented Nov 26, 2024

Don't worry, we are able to use our brains to deduce that the error is due to git not detecting the rename ;)

@zimward zimward force-pushed the fully-hysterical-screaming branch 3 times, most recently from 8560248 to 93a9a9d Compare December 3, 2024 06:30
@zimward zimward marked this pull request as draft December 3, 2024 07:31
@zimward zimward force-pushed the fully-hysterical-screaming branch from 93a9a9d to 0b962bc Compare December 3, 2024 07:57
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 3, 2024

Eval summary

  • Added packages: 0
  • Removed packages: 222
  • Changed packages: 3
  • Rebuild Linux: 0
  • Rebuild Darwin: 0

@zimward zimward force-pushed the fully-hysterical-screaming branch 3 times, most recently from de91470 to 1e3067e Compare December 4, 2024 08:20
@zimward zimward marked this pull request as ready for review December 4, 2024 08:27
@zimward
Copy link
Contributor Author

zimward commented Dec 4, 2024

Typical case of ending up re-implementing something in the same way as the original, apart from some cleanups and completely separating the building of the fhsenv and the runscript.

@philiptaron philiptaron requested review from Atemu and removed request for philiptaron December 4, 2024 20:26
@zimward zimward force-pushed the fully-hysterical-screaming branch from 1e3067e to 770ab0e Compare December 9, 2024 01:37
@nix-owners nix-owners bot requested a review from philiptaron December 9, 2024 01:38
@zimward zimward removed the request for review from philiptaron December 9, 2024 01:45
SuperSandro2000
SuperSandro2000 reviewed Dec 9, 2024
View reviewed changes
pkgs/build-support/build-fhsenv-bubblewrap/bwScript.nix Outdated
Copy link
Member

@SuperSandro2000 SuperSandro2000 Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The primary function has been moved to bwScript.nix to
reduce the amount of indentation.

I don't agree with that one. Now we have single line file and a big one next to it. 👎🏼

Edit: With the commit afterwards it is more justified but I am still undecided

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in hindsight the reasoning in the commit message isn't good as there is also a way of wrapping it without causing nixfmt to indent the whole file. With the separation we gain some clarity in my opinion as buildFHSenv basically consists of two mostly independent steps (build rootfs and wrap it).

Though i have to admit that the clarity gain is marginal.

@zimward zimward force-pushed the fully-hysterical-screaming branch from 770ab0e to 6386fbc Compare December 10, 2024 01:13
@nix-owners nix-owners bot requested a review from philiptaron December 10, 2024 01:14
@zimward zimward removed the request for review from philiptaron December 10, 2024 01:15
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/4959

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 4, 2025
@zimward
Copy link
Contributor Author

zimward commented Jan 6, 2025

Before i address the merge conflict i would like some final feedback if the split into wrapper.nix is useful or not.

Atemu
Atemu approved these changes Jan 7, 2025
View reviewed changes
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

No that's absolutely valid I think.

I had a quick glance at the diff and it generally LGTM. I haven't looked at any details but those could be ironed out after the rebase.

pkgs/build-support/build-fhsenv-bubblewrap/default.nix Outdated
Comment on lines 6 to 23
fhsenv = buildFHSEnv (
lib.attrsets.removeAttrs args [
"runScript"
"extraInstallCommands"
"meta"
"passthru"
"extraPreBwrapCmds"
"extraBwrapArgs"
"dieWithParent"
"unshareUser"
"unshareCgroup"
"unshareUts"
"unshareNet"
"unsharePid"
"unshareIpc"
"privateTmp"
]
);
Copy link
Member

Choose a reason for hiding this comment

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

Generally, I think I'd also like to get rid of this indirection somehow but that doesn't need to happen in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out this indirection is just plain unnecessary as only the {,p}name and version attrs of the @args pattern where used.

@zimward zimward force-pushed the fully-hysterical-screaming branch from 6386fbc to e590871 Compare January 7, 2025 13:53
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 7, 2025
@nix-owners nix-owners bot requested a review from philiptaron January 7, 2025 13:54
@zimward zimward removed the request for review from philiptaron January 7, 2025 13:56
@zimward
Copy link
Contributor Author

zimward commented Jan 8, 2025

Rebased onto master if it wasn't obvious.

I haven't looked at any details but those could be ironed out after the rebase.

There are not that many details TBH, ignoring the nixfmt commit the diff pretty small.

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 21, 2025
@nix-owners nix-owners bot requested a review from philiptaron January 21, 2025 10:56
@zimward zimward force-pushed the fully-hysterical-screaming branch from 3aac007 to a10b6cc Compare January 21, 2025 10:57
@zimward
buildFHSEnv: nixfmt
c4e1131
@zimward
buildFHSEnv: implement overriding
cbdc24b 
The primary function has been moved to wrapper.nix
to reduce the amount of indentation and seperate functunality
@zimward zimward force-pushed the fully-hysterical-screaming branch from a10b6cc to 68f3b3f Compare January 21, 2025 11:11
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 21, 2025
@zimward zimward force-pushed the fully-hysterical-screaming branch from 68f3b3f to 4f5c383 Compare January 21, 2025 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@Atemu Atemu Atemu approved these changes

@K900 K900 Awaiting requested review from K900

@philiptaron philiptaron Awaiting requested review from philiptaron

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zimward @nixos-discourse @Atemu @K900 @SuperSandro2000 @wegank