Conversation
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/request-for-comments-adding-two-new-hooks/58282/1 |
mutableHomeDirHook
mutableHomeDirHooktmpdirAsHome
9e45c4c to
c30ae4a
Compare
c30ae4a to
1a6ea00
Compare
tmpdirAsHome1a6ea00 to
48ddab5
Compare
48ddab5 to
2246ddf
Compare
2246ddf to
e45fb8f
Compare
|
Will this hook also be avaliable in preConfig or some thing like this? Here is some example: nixpkgs/pkgs/by-name/af/affine/package.nix Line 109 in 1a685a9 nixpkgs/pkgs/by-name/af/affine/package.nix Line 156 in 1a685a9 |
It can actually be added to any phase hooks, so yeah. |
hsjobeki
left a comment
There was a problem hiding this comment.
I like the Idea overall.
I also have two feedback Points:
- This could be two PRs.
- For my taste the amount of documentation is not enough. Especially i wouldn like to know stuff that happens internally, like what value is. $HOME set to and when?
While I'm all for splitting PR, make sure that they have a clear scope, I really don't understand why I would need to split this one. The scope is clear, even the commits are correctly split. So I'm sorry but no, I won't split this PR in two.
OK I will improve it. |
e45fb8f to
e8e0a8d
Compare
4823191 to
881d72c
Compare
|
Planning to merge this tomorrow morning, feel free to raise your voice if you believe it is a bad idea. |
|
I'd like to see some examples of packages helped by these hooks, at the very least. |
Here's some Github search where it would be used: |
doc/stdenv/stdenv.chapter.md
Outdated
There was a problem hiding this comment.
This should probably at least explicitly mention it's adding $out/bin and not $sourceRoot/bin or whatever. Honestly, this is my big concern with this hook: it's not immediately obvious what exactly it does, and the thing it does comes with a bunch of weird caveats (what if there are multiple outputs? what if the binaries are in sbin? etc).
There was a problem hiding this comment.
Thank you, I just updated the documentation. Let me know if this resolves this feedback.
881d72c to
f811073
Compare
|
|
||
| addBinToPath () { | ||
| # shellcheck disable=SC2154 | ||
| if [ -d "$out/bin" ]; then |
There was a problem hiding this comment.
Shouldn't we log something if the directory doesn't exist?
There was a problem hiding this comment.
There was a problem hiding this comment.
It hasn't been suggested during the month this PR has been opened, so no.
That said, while implementing that hook this morning, I had an issue because of that if condition. I removed it in 62d4ca6
|
|
||
| export HOME | ||
|
|
||
| writableTmpDirAsHome () { |
There was a problem hiding this comment.
If we name the hook "writable" shouldn't it also call chmod +w or so to make sure it is writable? Right now with the wrong umask it isn't
There was a problem hiding this comment.
It wasn't needed so far, but it could be done in a future PR.
|
|
||
| writableTmpDirAsHome () { | ||
| if [ ! -w "$HOME" ]; then | ||
| HOME="$NIX_BUILD_TOP/.home" |
There was a problem hiding this comment.
Why is that a hidden directory and not just home?
There was a problem hiding this comment.
It has been suggested to name it as such, I don't really have any preference.
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-24.11
git worktree add -d .worktree/backport-370869-to-release-24.11 origin/release-24.11
cd .worktree/backport-370869-to-release-24.11
git switch --create backport-370869-to-release-24.11
git cherry-pick -x 87521c59b636ae9073c58bd549b4cdc334a3dc28 f8110737aeedf8ba92ea46a0b1d2d298843d7c06 |
|
|
||
| writableTmpDirAsHome () { | ||
| if [ ! -w "$HOME" ]; then | ||
| HOME="$NIX_BUILD_TOP/.home" |
There was a problem hiding this comment.
I worry that this will break when run under nix-shell. Can we use something other than $NIX_BUILD_TOP please? (Or define a new env var for where the build actually happens, which is also valid in nix-shell?)
There was a problem hiding this comment.
Well, break nix-shell and use of the keepBuildTree setup hook, as per the linked issue above.
|
Is there a sane way that E.g. if I wanted a writable HOME for the build phase, but still wanted to use Or a better example, if I only wanted the check phase to be affected, effectively: preCheck = ''
export HOME="$NIX_BUILD_TOP/.home"
'';
postCheck = ''
export HOME=/homeless-shelter
''; |
Please use something other than |
That really wasn't the relevant part of my question. You've already made your point in #370869 (comment), maybe we should consider opening another PR where we can discuss moving away from My question was whether we could extend the |
Hello everyone,
During the holidays, I spent some time packaging Python packages and noticed that some of them require a valid and mutable home directory to build or function properly.
Currently, the default behavior of the Nix stdenv is to set the
HOMEenvironment variable to/homeless-shelter, a read-only directory. While this approach is consistent, it can cause issues for packages that expect a writable home directory.After performing a treewide search in
nixpkgs, I found approximately 500 instances whereexport HOMEis used (rg -c "export HOME" | wc -l) and 300 whereexport PATHis used (rg -c "export PATH" | wc -l). These repetitive patterns inspired me to propose a solution adhering to the DRY principle.I created custom hooks,
tmpdirAsHomeHook, which sets up a mutable home directory automatically when added tonativeBuildInputsor another appropriate place. Additionally,addBinToPath, which modifies thePATHenvironment variable to includebin/if available.I’d appreciate your feedback on this approach and have a few questions:
nixpkgs?tmpdirAsHomeHooktomutableHomeDirHookfor better clarity. What do you think?addEnvHooks "$targetOffset" addBinToPath. I’m not entirely confident about the best way to implement this. Could you provide some guidance?Thanks for taking the time to review this. I look forward to hearing your thoughts and suggestions!
Best regards
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.