Shell changes towards enabling an alternative shell (backward compatible with bash)#127736
Shell changes towards enabling an alternative shell (backward compatible with bash)#127736happysalada merged 38 commits intoNixOS:stagingfrom
Conversation
This is not the "correct" way to check if a variable is non null in bash. There is already an instance of the "right" way to do it in setup.sh. Bash is "generous" enough to accept the original input though. I couldn't find the relevant shellcheck.
https://github.com/koalaman/shellcheck/wiki/SC2206 https://github.com/koalaman/shellcheck/wiki/SC2207 admittedly this one is a lot less pretty
this one is a little more controversial see oils-for-unix/oils#864 for more information
|
Here is the last error I got, after explicitely adding oil to the set of allowedRequisites I verified that the resulting setup file (inside bootstrap-stage3-stdenv-darwin) doesn't contain explicit references to those. Perhaps the forbidden references are in one of those. Here are the forbidden dependency so no matter what we do, we need to have those in allowed requisites otherwise the build will fail. |
|
Next: |
|
adding the dependencies to allowedRequisites lead me to the next failure The offending function is the following since osh has errexit set by default and type fails, I think that is the problem here. |
|
That did not work. on strict errexit, adding |
type -p will exit 1 on failure. Test makes the intent clearer here.
|
That was a superficial error hiding the following error I have to say I'm a little puzzled at the mapOffset function being called with arguments that are variables that are not expanded. Here is the map offset function and it's being called like I don't understand why it's not |
|
I now get a different error, so I guess it was the right call |
| # implements. | ||
| findInputs() { | ||
| local -r pkg="$1" | ||
| local -ri hostOffset="$2" |
There was a problem hiding this comment.
Does shellcheck say to not do this? It seems reasonable to set -i:
The variable is to be treated as an integer;
arithmetic evaluation is performed when the
variable is assigned a value.
There was a problem hiding this comment.
This is the more controversial change. -i does not work at the moment with osh.
Apparently, -i is basically a no-op in bash. The recommended way to do arithmetic is to use ((...)) where the -i does not make a difference.
There was a longer discussion though between the creator of osh and zimbatm
oils-for-unix/oils#864
(I tried to link it in the commit message)
the -i does not come from shellcheck and could be made to work with osh, it would be a matter of making a PR upstream. At the moment it does not, so I just removed it.
(perhaps the commit title could have been clearer)
There was a problem hiding this comment.
Here is more relevant things around this at http://mywiki.wooledge.org/BashGuide/Parameters
Specifically
However, in practice the use of declare -i is exceedingly rare. In large part, this is because it creates behavior that can be surprising to anyone trying to maintain the script, who misses the declare statement. Most experienced shell scripters prefer to use explicit arithmetic commands (with ((...)) or let) when they want to perform arithmetic.
There was a problem hiding this comment.
Abandon all hopes of types, it's bash :D
If I remember correctly, -i changes the parser on the fly. It also doesn't catch errors so it's not super useful. Eg: declare -ri foo=bar && echo $foo will output 0
| # shellcheck disable=SC2086 | ||
| local flagsArray=( | ||
| $configureFlags ${configureFlagsArray+"${configureFlagsArray[@]}"} | ||
| IFS=" " read -r -a configureFlagsTemp <<< "$configureFlags" |
There was a problem hiding this comment.
I'm not super familiar with this, but this looks like it works in the way we want, while avoiding the shellcheck warning.
/cc @Ericson2314
There was a problem hiding this comment.
This might be one of those things that was only an issue pre-Bash4?
There was a problem hiding this comment.
This is coming from https://github.com/koalaman/shellcheck/wiki/SC2206
it's not so pretty, but it seems to work.
There was a problem hiding this comment.
Additionally reading up on arrays, I found this, which recommends the same
http://mywiki.wooledge.org/BashGuide/Arrays
|
👍 on this! I think all of these changes to the shell (or bashish) scripts are reasonable on their own. Any stats on comparisons to bash would be interesting, though. That varSlice part can probably be rewritten based on the todo. It should just return 0 if $pkg is in the $var map. |
|
@matthewbauer thank you for the explanation. I also found a reference commit here I'm guessing it could be rewritten as testing this right now |
…onditional Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
|
I had some commits break building bash. After a couple of git bisect, I removed the commits. |
|
It seems the eval has not started yet. I'm going to change the branch to master to see if this will trigger a faster eval. |
|
eval passed, I'm merging this on staging. |
andychu
left a comment
There was a problem hiding this comment.
(author of Oil here)
I realize this is already merged, but I just saw it and figured I would make some comments. Feel free to ignore them :)
To be honest I find the commit description misleading, because it feels like 90% of the changes have nothing to do with making it run under OSH? The changes I see that Oil probably won't run are:
- the
export FOO+=one. - the
local -richanges
Other changes seem to have more to do with ShellCheck than OSH? And just misc style changes like changing [ to [[ (which I generally agree with, but not really related to OSH).
ShellCheck and OSH are mostly complementary, but for my own edification and that of future OSH users/migrators, I would have liked to see the two groups of changes separated. Personally I would be scared to make all these foundational changes to what looks like fairly fundamental code in one big bunch, but you know the code better than I !
|
|
||
| if [[ -d "$1/lib64" && ! -L "$1/lib64" ]]; then | ||
| export NIX_LDFLAGS${role_post}+=" -L$1/lib64" | ||
| varName=NIX_LDFLAGS${role_post} |
There was a problem hiding this comment.
I would add local varName so it doesn't "leak". Also it seems like the style is local var_name, e.g. akin to role_post ?
It took me awhile to figure this code out but I get what it's doing now... it is indeed very dynamic.
I think export "$var_name" is sufficient?
There was a problem hiding this comment.
FWIW I don't believe this should be affected by OSH but I can see why you would want to change it
| echo "defaultBuildInputs=\"$defaultBuildInputs\"" | ||
| echo "$preHook" | ||
| cat "$setup" | ||
| } > "$out/setup" |
There was a problem hiding this comment.
This is a great change! One of my pet peeves :)
The >> style is less readable and actually causes a ton of open() and close() syscalls
| # in stdenv. | ||
| mkdir $out/nix-support | ||
| if [ "$propagatedUserEnvPkgs" ]; then | ||
| if [ -n "${propagatedUserEnvPkgs:-}" ]; then |
There was a problem hiding this comment.
A few comments:
- FWIW this shouldn't be affected by OSH. That is, strictly speaking, no change is necessary (I worry about people being confused based on the PR).
- And it seems like the
:-is a behavior change and not a style change? Maybe that is intended. - Since you're using
[[everywhere, you could also use it here.
There was a problem hiding this comment.
Actually this is interesting.
I've been running it as /bin/osh -x -O strict:all
just to be able to debug and to get "potential" bugs early.
This variable was actually undefined in some cases and so the script could fail.
What you are saying is that I should not run it with strict:all and just try to see what comes out of it?
| times > "$NIX_BUILD_TOP/.times" | ||
| local -a times=($(cat "$NIX_BUILD_TOP/.times")) | ||
| local -a buildTimesArray | ||
| IFS=" " read -r -a buildTimesArray < "$NIX_BUILD_TOP/.times" |
There was a problem hiding this comment.
FWIW I thought the original was OK, and this shouldn't be affected by OSH? I would be interested to learn otherwise.
There was a problem hiding this comment.
This one was just a shell check
|
|
||
| local varVar="${pkgAccumVarVars[$hostOffset + 1]}" | ||
| local varRef="$varVar[\$targetOffset - \$hostOffset]" | ||
| local varVar="${pkgAccumVarVars[$(( hostOffset + 1 ))]}" |
There was a problem hiding this comment.
BTW this can be ${vars[hostOffset + 1]}, because whatever is within ${a[i]} is parsed like arithemetic. Maybe that is too subtle for some but I'm used to it.
(Also now I notice the naming style is sometimes camelCase and sometimes role_host)
I guess it is pretty confusing because below you there is some "lazy evaluation" with ${varVar}[ $(( expr )) ], which indeed does need $(( )), because it's constructing a string to be evaluated later. This code is very confusing :)
There was a problem hiding this comment.
I don't like these references that much, but it seems that logic is in various places, I'm still trying to find a "better" solution, but I have to be able to understand the code holistically and I'm not there.
| for depHostOffset in "${allPlatOffsets[@]}"; do | ||
| local hookVar="${pkgHookVarVars[$depHostOffset + 1]}" | ||
| local pkgsVar="${pkgAccumVarVars[$depHostOffset + 1]}" | ||
| local hookVar="${pkgHookVarVars[$(( depHostOffset + 1 ))]}" |
There was a problem hiding this comment.
ditto here and throughout, $(( )) is optional in a bash/ksh array subscript
There was a problem hiding this comment.
Thank you for this one, I'll keep this in mind!
|
@andychu Thanks again for your comment! |
|
Ah OK so It's not surprising that turning on If you're NOT running this script under bash with But maybe you are planning to, and it's fine! It's up to you either way, but I guess I would expect the PR to also set I also wonder if we should turn off Also it's true that if you're going to eventually upgrade to the "OIl language", you want to run with |
|
Thank you for sharing. |
|
This seems to cause a weird regression in some cases: #129506 (comment) I suspect it's cases where stdenv is used in some more complex way, e.g. the 64+32-bit for wine. Anyway, if we can't pinpoint it rather soon, I'd suggest to revert for now until we can solve it, so other changes don't get blocked by this. |
|
@vcunat: bf99a81#r53315516 is causing the regression, so I'd suggest reverting that commit for now instead of the whole pull request. |
I suspect that some of the stdenv changes (PR #127736 maybe?) affected how the newline was handled. Anyway, it was ugly, so let's use a more standard approach.
|
For reference, this whole PR got reverted (for now). It caused multiple unexpected regressions. If you want to redo this, I suggest that:
|
|
I'm going to try to separate this into logical parts so hopefully the simple fixes can be merged first. |
Motivation for this change
Oil shell is a new shell that aims to retain compatibility with bash but brings in new feature that simplify shell scripting. Using it in nix would make shell scripting more accessible to many people (IMO). The drawback being that osh has not reached 1.0 yet and that there might still be some bugs.
This PR starts with a mix of shellcheck fixes, the ultimate goal being to be able to change the default shell shipped with stdenv.
Current state: Only shell fixes have been added to this PR at the moment, so stdenv should build fine. I am currently working on making the shell change work. The current error is related to a shell reference being included in the stdenv-stage3. At the moment stdenv-stage3 explicitely defines an
allowedRequisitesfor bash.@zimbatm I'm wondering who else might be interested by this. Specifically who we should make sure to expose early enough to know if this will be a no-go or not.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)