-
-
Notifications
You must be signed in to change notification settings - Fork 14.9k
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
build-support: add new hooks #370869
build-support: add new hooks #370869
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# shellcheck shell=bash | ||
# This setup hook add $out/bin to the PATH environment variable. | ||
|
||
export PATH | ||
|
||
addBinToPath () { | ||
# shellcheck disable=SC2154 | ||
if [ -d "$out/bin" ]; then | ||
PATH="$out/bin:$PATH" | ||
export PATH | ||
fi | ||
} | ||
|
||
# shellcheck disable=SC2154 | ||
addEnvHooks "$targetOffset" addBinToPath |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# shellcheck shell=bash | ||
# This setup hook set the HOME environment variable to a writable directory. | ||
|
||
export HOME | ||
|
||
writableTmpDirAsHome () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It wasn't needed so far, but it could be done in a future PR. |
||
if [ ! -w "$HOME" ]; then | ||
HOME="$NIX_BUILD_TOP/.home" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is that a hidden directory and not just home? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It has been suggested to name it as such, I don't really have any preference. |
||
mkdir -p "$HOME" | ||
export HOME | ||
fi | ||
} | ||
|
||
# shellcheck disable=SC2154 | ||
addEnvHooks "$targetOffset" writableTmpDirAsHome |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we log something if the directory doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been changed since then: https://github.com/NixOS/nixpkgs/pull/378110/files#diff-d390481be231cb8f7d3eaf4cbc0cd6580b5e469d9c3ac6c5a91a8c9fa62f3799
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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