Skip to content
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

pkgs/stdenv/generic/setup.sh: lint with ShellCheck #351849

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Oct 28, 2024

The behaviour of [[ -n/-z "${FOO[@]}" ]] is unspecified. Use [[ -n/-z "${FOO[*]-}" ]] instead.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Oct 28, 2024
@ShamrockLee
Copy link
Contributor Author

@ofborg build stdenv
@ofborg build hello hello.tests

@ShamrockLee ShamrockLee marked this pull request as ready for review October 28, 2024 11:03
pkgs/stdenv/generic/setup.sh Outdated Show resolved Hide resolved
Comment on lines 399 to 400
if [[ ! -n "${nameref[@]}" && -n "$default" ]]; then
if [[ -z "${nameref[*]-}" && -n "${default-}" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We gotta be careful here, there was quite some discussion in #335666 around this part. Is this 100% the same as before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should remove the hyphens:

        if [[ -z "${nameref[*]}" && -n "$default" ]]; then

As the Bash Reference Manual doesn't specify the behavior of [[ -n "${arr[@]}" ]], let's experiment with the command if [[ -n "${foo[@]}" ]]; then echo yes; else echo no; fi; unset foo:

unset foo foo="" foo=" " foo=" hi" foo="hello hi"
no no yes yes yes
foo=() foo=("") foo=("" "") foo=("" hi) foo=("hello" "hi")
no no yes yes yes

According to the Bash Reference Manual,

Any element of an array may be referenced using ${name[subscript]}. ... If the word is double-quoted, ${name[*]} expands to a single word with the value of each array member separated by the first character of the IFS variable

This means the following conversion when the first character of the IFS variable is " ":

foo=() foo=("") foo=("" "") foo=("" hi) foo=("hello" "hi")
"" "" " " " hi" "hello hi"

It should be safe to say that [[ -n "${foo[@]}" ]] behaves like [[ -n "$foo[*]" ]], but only the latter is specified in the Bash Reference Manual.

The behaviour of [[ -n/-z "${FOO[@]}" ]] is unspecified.
Use [[ -n/-z "${FOO[*]-}" ]] instead
@ShamrockLee ShamrockLee force-pushed the stdenv-setup-shellcheck branch from 6c73c49 to 34ebbd6 Compare October 28, 2024 14:26
@wolfgangwalther
Copy link
Contributor

This should be good despite the eval error, which is related to the target branch at that time.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 31, 2024
@philiptaron philiptaron merged commit f2b00cb into NixOS:staging Nov 1, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: stdenv Standard environment 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants