treewide: support structuredAttrs in setup hooks (part 2)

[wolfgangwalther]

Description of changes

This is a follow up to #318614, which had a lot more commits initially. After merging the basic changes for stdenv's appendTo, prependTo and new concatTo functions there, I now come back to apply those in more setup hooks.

I intentionally only took a part of long tail of commits I still have for this, so there will be more after this PR. All commits in this PR have one thing in common: I was able to test each of them by building the respective package mentioned in the commit message with __structuredAttrs turned on, similar to how I did it in #318614: The build for this package would fail on master, but succeed on this branch.

For some of the other setup hooks it's not that easy to find examples in nixpkgs, which actually fail with structuredAttrs turned on, so hard to prove the fix works. Thus I left them out for the moment and will bring them back once we have merged this set of changes, increasing the confidence in the general pattern of how the transition to concatTo works.

CC @emilazy @philiptaron @SomeoneSerge @tie

Since the changed setup hooks won't trigger ofborg to mention the maintainers:

• bmake: @thoughtpolice @AndersonTorres
• just: @xrelkd @06kellyjac
• waf: @AndersonTorres
• zig: @andrewrk @figsoda
• build2: @vale981 @r-burns
• scons: @AndersonTorres
• premake: @bjornfor
• gn: @stesie @matthewbauer @primeos
• qt5: @qknight @ttuegel @periklis @bkchr
• qt6: @milahu @NickCao
• perl: @edolstra

Quick recap for those not involved in #318614: That PR added a new function concatTo to allow merging of flags arrays / lists in either string or bash array format. Together with the already existing appendTo and prependTo, this allows to transparently support both building with and without structuredAttrs turned on, without putting checks whether structuredAttrs are enabled or not all over nixpkgs.

Making all the setup hooks support structuredAttrs is the first step to making this the default eventually.

Things done

Of course I built the respective packages themselves - but since the setup hook is not a part of the derivation, those still work. As mentioned above, I built another package for each of the setup hooks, which uses that hook in the build process.

• 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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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.

[philiptaron]

Haven't tested it yet. Lots of questions about shellcheck; thanks for adding it in a lot of areas. The overall approach is sound.

[wolfgangwalther]

Addressed the first round of comments.

[tie]

Looks good to me, although the changes introduce some side effects (see comment).

[philiptaron]

Thanks for addressing the shellcheck issues.

I like @tie's suggestion about

if [[ -v checkFlags ]]; then
  concatTo flagsArray checkFlags
else
  appendToVar flagsArray VERBOSE=y
fi

[philiptaron]

Is there anything that blocks this from being merged?

I move we merge it, and include it in the new staging-next and let it bake.

[emilazy]

We can’t merge a nice‐to‐have Darwin stdenv rebuild into staging-next. Hydra has already built over 10,000 packages. It will have to go in the next cycle.

[SomeoneSerge]

Can't contribute much to the discussion above but I read through the diffs ✅

flagsArray ... visible in postX-hooks

I have some concerns over what the flagsArray pattern means for #296544 (example: use part of the cmake build phase and part of the pypa-build phase in the same derivation)

[wolfgangwalther]

flagsArray ... visible in postX-hooks

I have some concerns over what the flagsArray pattern means for #296544 (example: use part of the cmake build phase and part of the pypa-build phase in the same derivation)

Do you mean whether flagsArray might leak into other phases, e.g. when calling them in post-hooks? The pattern we used so far is, that each hook creates a clean local flagsArray=(...) first, before using concatTo. So even if it was set before, it should not matter. So I think flagsArray is non-critical.

I will look into the default values thread above again to make sure that it's non-leaking, even in hooks, though.

[wolfgangwalther]

I'm currently looking into how I can fallback to the default better.

I'm currently running a local build through with this check

boost built fine, so pushed. Still building the world to confirm each of the commits still works.

[philiptaron]

This all looks good to me.

[tie]

IFS="=" read -r name default <<< "$arg"
local -n nameref="$name"
if [[ ! -n "${nameref[@]}" && -n "$default" ]]; then
           targetref+=( "$default" )

@tie do you see any problem with that approach?

It treats an array with a single element that is an empty string as empty 😅
Probably not an issue in practice though.

declare -a arr=( '' )
if [[ -v arr ]]; then
  echo defined
fi
if [[ -z "${arr[@]}" ]]; then
  echo oops
fi
# Output:
# defined
# oops

What do you think about the following check?

# Either variable is not declared,
! type=$(declare -p -- "$name" 2>/dev/null) ||
  # or it is declared but no value is assigned (e.g. empty array),
  [[ -z ${!name+x} ]] ||
  # or it is not an array and is empty.
  [[ ${type#* } != -[aA]* && -z ${!name} ]]

That is, we’d treat empty values (empty arrays and strings) consistently by using the default value.

For example,

set -eu -o pipefail

concatTo() {
    local -n targetref=$1
    shift
    local arg default name type
    for arg; do
        IFS='=' read -r name default <<<"$arg"
        # Append the default value if $name variable is not declared. Note that
        # variable attributes are separate from variable assignment. In addition
        # to that, declare assigns an attribute for name but does not actually
        # create the variable for empty arrays (i.e. `declare -a arr=()`).
        #
        # For compatibility, we treat variables with empty values as undeclared as well.
        if
            # Either variable is not declared,
            ! type=$(declare -p -- "$name" 2>/dev/null) ||
                # or it is declared but no value is assigned (e.g. empty array),
                [[ -z ${!name+x} ]] ||
                # or it is not an array and is empty.
                [[ ${type#* } != -[aA]* && -z ${!name} ]]
        then
            case "$default" in
            '('*)
                # Evaluate array expressions, i.e.
                # `concatTo fooFlags barFlags='(baz)'`.
                #
                # Beware that, just like the regular array assignments, this
                # evaluates Bash code inside the default value. E.g.
                # `declare -a arr='( "$(echo element)" )' && echo "${arr[*]}"` prints
                # `element`.
                local -a defaultArray=$default
                targetref+=("${defaultArray[@]}")
                ;;
            '')
                # Ignores both `undefinedVar=` and `undefinedVar` to avoid
                # appending an empty string for the latter.
                # TODO: perhaps return an error if no default is given and
                # variable is not declared?
                ;;
            *)
                targetref+=("$default")
                ;;
            esac
            continue
        fi

        local -n nameref=$name
        case "${type#* }" in
        -A*)
            echo "concatTo(): ERROR: trying to use concatTo on an associative array." >&2
            return 1
            ;;
        -a*)
            targetref+=("${nameref[@]}")
            ;;
        *)
            if [[ $name == *"Array" ]]; then
                targetref+=("${nameref[@]}")
            else
                # shellcheck disable=SC2206
                targetref+=(${nameref-})
            fi
            ;;
        esac
    done
}


declare -a definedFlags=(b c)
declare -a emptyArray=()
declare emptyString=
declare unboundString
declare string=h
concatTo flagsArray emptyArray=a definedFlags=unused undefinedWithDefaultArray='(d e)' emptyString=f unboundString=g string=unused undefinedWithoutDefault
echo "${flagsArray[@]}"
# Output:
# a b c d e f g h

Note that the code above also allows using arrays as default values, e.g.

concatToVar flagsArray defaultArray='( --foo --bar --baz )'

[wolfgangwalther]

@tie do you see any problem with that approach?

It treats an array with a single element that is an empty string as empty 😅

Yes, that's on purpose to not introduce a breaking change.

Note that the code above also allows using arrays as default values, e.g.

concatToVar flagsArray defaultArray='( --foo --bar --baz )'

Interesting. I have only found one use-case, so far, the autoreconf hook. I might look into this later (travelling this week).

[wolfgangwalther]

Still building the world to confirm each of the commits still works.

Confirmed that all commits still work and have the intended change for building with structuredAttrs.

[tie]

Yes, that's on purpose to not introduce a breaking change.

Perhaps a check for [[ -z ${nameref[@]:+x} ]] is more clear then, I usually expect ${var[@]} to expand to multiple words (which I don’t think is the case here).

$ bash -e -u -c 'declare -a arr=(); echo "${arr[@]:+x}"'

$ bash -e -u -c 'declare -a arr=(""); echo "${arr[@]:+x}"'

$ bash -e -u -c 'declare -a arr=("" ""); echo "${arr[@]:+x}"'
x
$ bash -e -u -c 'declare -a arr=("" "brr"); echo "${arr[@]:+x}"'
x
$ bash -e -u -c 'declare -a arr=("brr"); echo "${arr[@]:+x}"'
x
$ bash -e -u -c 's=; echo "${s[@]:+x}"'

$ bash -e -u -c 's=brr; echo "${s[@]:+x}"'
x

[philiptaron]

@tie, let's finesse it in follow-on PRs. Welcome to the committer team, btw!