-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
arrayUtilities: init #385960
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
Merged
Merged
arrayUtilities: init #385960
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
45 changes: 45 additions & 0 deletions
45
pkgs/build-support/setup-hooks/arrayUtilities/getSortedMapKeys/getSortedMapKeys.bash
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # shellcheck shell=bash | ||
|
|
||
| # getSortedMapKeys | ||
| # Stores the sorted keys of the input associative array referenced by inputMapRef in the indexed arrray referenced by | ||
| # outputArrRef. | ||
| # | ||
| # Note from the Bash manual on arrays: | ||
| # There is no maximum limit on the size of an array, nor any requirement that members be indexed or assigned contiguously. | ||
| # - https://www.gnu.org/software/bash/manual/html_node/Arrays.html | ||
| # | ||
| # Since no guarantees are made about the order in which associative maps are traversed, this function is primarly | ||
| # useful for getting rid of yet another source of non-determinism. As an added benefit, it checks that the arguments | ||
| # provided are of correct type, unlike native parameter expansion which will accept expansions of strings. | ||
| # | ||
| # Arguments: | ||
| # - inputMapRef: a reference to an associative array (not mutated) | ||
| # - outputArrRef: a reference to an indexed array (contents are replaced entirely) | ||
| # | ||
| # Returns 0. | ||
| getSortedMapKeys() { | ||
| if (($# != 2)); then | ||
| nixErrorLog "expected two arguments!" | ||
| nixErrorLog "usage: getSortedMapKeys inputMapRef outputArrRef" | ||
| exit 1 | ||
| fi | ||
|
|
||
| local -rn inputMapRef="$1" | ||
| # shellcheck disable=SC2178 | ||
| # Don't warn about outputArrRef being used as an array because it is an array. | ||
| local -rn outputArrRef="$2" | ||
|
|
||
| if ! isDeclaredMap "${!inputMapRef}"; then | ||
| nixErrorLog "first argument inputMapRef must be a reference to an associative array" | ||
| exit 1 | ||
| elif ! isDeclaredArray "${!outputArrRef}"; then | ||
| nixErrorLog "second argument outputArrRef must be a reference to an indexed array" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # shellcheck disable=SC2034 | ||
| local -a keys=("${!inputMapRef[@]}") | ||
| sortArray keys "${!outputArrRef}" | ||
|
|
||
| return 0 | ||
| } |
17 changes: 17 additions & 0 deletions
17
pkgs/build-support/setup-hooks/arrayUtilities/getSortedMapKeys/package.nix
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| { | ||
| callPackages, | ||
| isDeclaredArray, | ||
| isDeclaredMap, | ||
| makeSetupHook, | ||
| sortArray, | ||
| }: | ||
| makeSetupHook { | ||
| name = "getSortedMapKeys"; | ||
| propagatedBuildInputs = [ | ||
| isDeclaredArray | ||
| isDeclaredMap | ||
| sortArray | ||
| ]; | ||
| passthru.tests = callPackages ./tests.nix { }; | ||
| meta.description = "Gets the sorted indices of an associative array"; | ||
| } ./getSortedMapKeys.bash |
80 changes: 80 additions & 0 deletions
80
pkgs/build-support/setup-hooks/arrayUtilities/getSortedMapKeys/tests.nix
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| # NOTE: Tests related to getSortedMapKeys go here. | ||
| { | ||
| getSortedMapKeys, | ||
| lib, | ||
| testers, | ||
| }: | ||
| let | ||
| inherit (lib.attrsets) recurseIntoAttrs; | ||
| inherit (testers) shellcheck shfmt testEqualArrayOrMap; | ||
|
|
||
| check = | ||
| { | ||
| name, | ||
| valuesMap, | ||
| expectedArray, | ||
| }: | ||
| (testEqualArrayOrMap { | ||
| inherit name valuesMap expectedArray; | ||
| script = '' | ||
| set -eu | ||
| nixLog "running getSortedMapKeys with valuesMap to populate actualArray" | ||
| getSortedMapKeys valuesMap actualArray | ||
| ''; | ||
| }).overrideAttrs | ||
| (prevAttrs: { | ||
| nativeBuildInputs = prevAttrs.nativeBuildInputs or [ ] ++ [ getSortedMapKeys ]; | ||
| }); | ||
| in | ||
| recurseIntoAttrs { | ||
| shellcheck = shellcheck { | ||
| name = "getSortedMapKeys"; | ||
| src = ./getSortedMapKeys.bash; | ||
| }; | ||
|
|
||
| shfmt = shfmt { | ||
| name = "getSortedMapKeys"; | ||
| src = ./getSortedMapKeys.bash; | ||
| }; | ||
|
|
||
| empty = check { | ||
| name = "empty"; | ||
| valuesMap = { }; | ||
| expectedArray = [ ]; | ||
| }; | ||
|
|
||
| singleton = check { | ||
| name = "singleton"; | ||
| valuesMap = { | ||
| "apple" = "fruit"; | ||
| }; | ||
| expectedArray = [ "apple" ]; | ||
| }; | ||
|
|
||
| keysAreSorted = check { | ||
| name = "keysAreSorted"; | ||
| valuesMap = { | ||
| "apple" = "fruit"; | ||
| "bee" = "insect"; | ||
| "carrot" = "vegetable"; | ||
| }; | ||
| expectedArray = [ | ||
| "apple" | ||
| "bee" | ||
| "carrot" | ||
| ]; | ||
| }; | ||
|
|
||
| # NOTE: While keys can be whitespace, they cannot be null (empty). | ||
| keysCanBeWhitespace = check { | ||
| name = "keysCanBeWhitespace"; | ||
| valuesMap = { | ||
| " " = 1; | ||
| " " = 2; | ||
| }; | ||
| expectedArray = [ | ||
| " " | ||
| " " | ||
| ]; | ||
| }; | ||
| } |
14 changes: 14 additions & 0 deletions
14
pkgs/build-support/setup-hooks/arrayUtilities/isDeclaredArray/isDeclaredArray.bash
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| # shellcheck shell=bash | ||
|
|
||
| # isDeclaredArray | ||
| # Tests if inputArrayRef refers to a declared, indexed array. | ||
| # | ||
| # Arguments: | ||
| # - inputArrayRef: a reference to an indexed array (not mutated) | ||
| # | ||
| # Returns 0 if the indexed array is declared, 1 otherwise. | ||
| isDeclaredArray() { | ||
| # NOTE: We must dereference the name ref to get the type of the underlying variable. | ||
| # shellcheck disable=SC2034 | ||
| local -nr inputArrayRef="$1" && [[ ${!inputArrayRef@a} =~ a ]] | ||
| } | ||
9 changes: 9 additions & 0 deletions
9
pkgs/build-support/setup-hooks/arrayUtilities/isDeclaredArray/package.nix
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| { | ||
| callPackages, | ||
| makeSetupHook, | ||
| }: | ||
| makeSetupHook { | ||
| name = "isDeclaredArray"; | ||
| passthru.tests = callPackages ./tests.nix { }; | ||
| meta.description = "Tests if an array is declared"; | ||
| } ./isDeclaredArray.bash |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do a bunch of checks for "is array" in
stdenv/generic/setup.sh. Even though we use case statements there, I think abstracting away this into two calls toisDeclaredArray/isDeclaredMapmakes the code much more readable.If we wanted to use it there, we'd need to make this part of stdenv, though - not another setup hook. Thus.. I suggest to do just that, for those two functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These have more utility than just for
stdenv, so I want them to remain separate.That said, I'm not opposed to this implementation find its way into
stdenv-- I just don't want them to be available solely throughstdenv.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. I thought that whatever was available in
stdenvwas available everywhere? What exactly is the benefit of having them available "separately"? How would that look like? I might be missing something, but I only see a negative effect here - aka that they are not available instdenvright now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One example is creating scripts which run outside
stdenv-- for example, themagma.passthru.testers.allscript introduced here: #414612.Is there an easy way to ensure
setup.shand all the goodies it has are sourced in such scripts, without polluting the shell with a bunch of other variables?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds a bit like we'd maybe want a separation of something like
stdenvandstdlib? (wherestdenvcan make use ofstdlib!)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to see something like that and am interested in helping make that happen --
in the short term, I'd like to have these merged so I can bring in fixes for CUDA setup hooks which require more complicated Bash functionality (and I hate writing common functionality in multiple places).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a use-case for
sortArrayinstdenvnow:getAllOutputNamesreturns a different order of outputs depending on whether__structuredAttrsis turned on or not. This causes things like #425323 (comment). If we were to sort these outputs by name, they would match between both cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like that these functions are separate so I can use them outside of things providing
setup.shand have an easy way to test them. I'm not opposed to the ol' copy/paste or including the files inmkDerivation'snativeBuildInputs... what are your thoughts? Although maybe copy-pasting would be better so any changes to these functions don't cause a rebuild ofstdenv.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can (or should) use something from
mkDerivation.nativeBuildInputsinsetup.sh, aka in core stdenv.I don't like copy&paste either.
I think this was still a good idea: