Shellcheck some test scripts #10797
Shellcheck some test scripts #10797
Conversation
github-actions
bot
added
the
with-tests
Ericson2314
force-pushed
the
shellcheck-tests
branch
2 times, most recently
from
af6c6d0
to
7043fad
Compare
There was a problem hiding this comment.
These files aren't executable, and the presence of a shebang would suggest that that's by mistake.
I'd prefer the shellcheck hint, # shellcheck shell=bash.
There was a problem hiding this comment.
Actually we could come up with a proper shebang, but if the interpreter is a script, it doesn't work on mac, so we might need a helper in the dev shell.
Also shellcheck wouldn't recognize the helper as bash, so we're going to need the hint anyway.
There was a problem hiding this comment.
@roberth I think we could just make them executable. They are self-contained unlike the fragments.
There was a problem hiding this comment.
I don't think it's quite right.
We should then also apply the bash options from common-test.sh, and always go through the shebang instead of a non-standard invocation with $BASH.
Even then, I'm not sure running tests without TESTS_ENVIRONMENT is a sufficiently non-bad idea that we should seduce contributors into running them this way.
There was a problem hiding this comment.
I think I would rather just get rid of TESTS_ENVIRONMENT! :)
Ericson2314
force-pushed
the
shellcheck-tests
branch
from
82c4e99
to
0ab64e1
Compare
Progress on NixOS#10795
Ericson2314
force-pushed
the
shellcheck-tests
branch
from
0ab64e1
to
2e12b58
Compare
This is good for shebang, and also good for future build system simplifications
Motivation
See the issue.
Context
Progress on #10795
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.