Conversation
There was a problem hiding this comment.
Comment on commit: setup.sh: avoid subshells: shopt -po nounset:
TL;DR: replacing
local oldOpts="$(shopt -po nounset)"
# ...
eval "$oldOpts"with
local oldOpts="-u"
shopt -qo nounset || oldOpts="+u"
# ...
set "$oldOpts"Test example
#!/usr/bin/env bash
set -eu
set -o pipefail
orig() {
local oldOpts="$(shopt -po nounset)"
echo -n "... "
eval "$oldOpts"
}
new() {
local oldOpts="-u"
shopt -qo nounset || oldOpts="+u"
echo -n "... "
set "$oldOpts"
}
echo orig
set -u; orig; shopt -po nounset || true
set +u; orig; shopt -po nounset || true
echo
echo new
set -u; new; shopt -po nounset || true
set +u; new; shopt -po nounset || true| eval "${!hookName}" | ||
| else | ||
| return "$def" | ||
| fi |
There was a problem hiding this comment.
Comment on _callImplicitHook:
This is tricky. Following assumption is taken:
$hookName can't be an alias, builtin or keyword. It's either a function, or a file, or a variable, or the default value is returned.
Test example
#!/usr/bin/env bash
set -eu
set -o pipefail
orig() {
local hookName=$1
case "$(type -t "$hookName")" in
(function|alias|builtin) echo function;;
(file) echo file;;
(keyword) :;;
(*) if [ -z "${!hookName:-}" ]; then
echo default
else
echo variable
fi;;
esac
}
new() {
local hookName=$1
if declare -F "$hookName" > /dev/null; then
echo function
elif type -p "$hookName" > /dev/null; then
echo file
elif [ -n "${!hookName:-}" ]; then
echo variable
else
echo default
fi
}
dummyfunc() { :; }
dummyvar=value
echo orig
orig dummyfunc
orig ./05.sh
orig bash
orig dummyvar
orig nonexistent
echo
echo new
new dummyfunc
new ./05.sh
new bash
new dummyvar
new nonexistent| # command can take them | ||
| _eval() { | ||
| if [ "$(type -t "$1")" = function ]; then | ||
| if declare -F "$1" > /dev/null 2>&1; then |
There was a problem hiding this comment.
Test example
#!/usr/bin/env bash
set -eu
set -o pipefail
orig() {
local rc=0
[ "$(type -t "$1")" = function ] || rc=$?
echo $rc
}
new() {
local rc=0
declare -F "$1" > /dev/null 2>&1 || rc=$?
echo $rc
}
dummyfunc() { :; }
alias dummyalias=dummyfunc
dummystmt='if [ -z "$noAuditTmpdir" -a -e "$prefix" ]; then auditTmpdir "$prefix"; fi'
echo orig
orig dummyfunc
orig dummyalias
orig echo
orig pwd
orig "$dummystmt"
echo
echo new
new dummyfunc
new dummyalias
new echo
new pwd
new "$dummystmt"| local pkgNext | ||
| for pkgNext in $(< "$pkg/nix-support/$file"); do | ||
| read -r -d '' pkgNext < "$pkg/nix-support/$file" || true | ||
| for pkgNext in $pkgNext; do |
There was a problem hiding this comment.
Test example
#!/usr/bin/env bash
set -eu
set -o pipefail
orig() {
local pkgNext
for pkgNext in $(< "$1"); do
echo -n " [$pkgNext]"
done
echo
}
new() {
local pkgNext
read -r -d '' pkgNext < "$1" || true
for pkgNext in $pkgNext; do
echo -n " [$pkgNext]"
done
echo
}
printf 'foo ba\\r baz\nabc def\n\n123' > /tmp/testfile1
printf 'foo ba\\r baz\nabc def\n\n123\n' > /tmp/testfile2
echo orig
orig /tmp/testfile1
orig /tmp/testfile2
echo new
new /tmp/testfile1
new /tmp/testfile2|
Is this ready for review or do you still working on it? |
|
@Mic92 It is ready for review. |
|
Looks good to me, maybe @zimbatm could also have a look. |
| # directories and bloats the size of the environment variable space. | ||
| if [[ -n "$(echo $1/lib/lib*)" ]]; then | ||
| local -a glob=( $1/lib/lib* ) | ||
| if [ "${#glob[*]}" -gt 0 ]; then |
There was a problem hiding this comment.
Just my personal preference. In this case both can be used.
There was a problem hiding this comment.
Since we are not trying to be POSIX compatible I think it's best to always use [[ ]] and pretend that [ ] don't exist.
There was a problem hiding this comment.
For example here you can use [[ "${#glob[*]}" > 0 ]]. That syntax also reacts better with missing quotes. Eg:
foo="x -o true"
[ x = $foo ] # => 0
[[ x = $foo ]] # => 1
| export "${role_pre}${upper_case}=@targetPrefix@${cmd}"; | ||
| export "${upper_case}${role_post}=@targetPrefix@${cmd}"; | ||
| export "${role_pre}${cmd^^}=@targetPrefix@${cmd}"; | ||
| export "${cmd^^}${role_post}=@targetPrefix@${cmd}"; |
There was a problem hiding this comment.
I originally did something like this, but people complained on darwin that nix-shell uses the ambient bash which is old as hell. But Apple is switching to zsh anyways I think? And also I think supporting ambient bashes is stupid; fix the usability a different way.
There was a problem hiding this comment.
I don't think we need to worry about builtin bash anyway. It can be an issue with bootstrapping, but we just need to tell people to build a newer Bash before they build Nix from source.
There was a problem hiding this comment.
Oh! running nix-shell shows that it is not impure anymore yay!!!
There was a problem hiding this comment.
I think this was only ever a problem in the impure stdenv though. It would be good to document our minimum Bash version though. I think all of these new features were added in Bash 4?
| runHook() { | ||
| local oldOpts="$(shopt -po nounset)" | ||
| local oldOpts="-u" | ||
| shopt -qo nounset || oldOpts="+u" |
There was a problem hiding this comment.
Hahahaha nice trick. I missed it until @matthewbauer pointed it out.
| local oldOpts="$(shopt -po nounset)" | ||
| local oldOpts="-u" | ||
| shopt -qo nounset || oldOpts="+u" | ||
| set -u # May be called from elsewhere, so do `set -u`. |
There was a problem hiding this comment.
It would be nice if we could just stop set +u-ing in nixpkgs! Wanna tackle that next? :)
|
@xzfc Great work! This is doubly useful for a) making things fast b) convincing people we need to stop using bash. I heartily approve of both. Now that it's clear we don't need to support bash 3. Let's also use |
Avoid using subshells (cherry picked from commit 268d510) NixOS#69131
Backport pull request #69131 from xzfc/subshells
Avoid subshells (cherry picked from commit 268d510)
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/wrapping-activation-scripts-into-subshell/15564/2 |
Motivation for this change
Subshells take a significant amount of time during
nix-shellstartup process.This PR replaces the most overly used subshell invocations in
setup.shandbintools-wrapper.Benchmark
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)Further work
If someone wants to hunt remaining subshell invocations, there is a possible starting point:
Inside nix-shell:
Notify maintainers
cc @Ericson2314 @matthewbauer