Conversation
Could you give please a somewhat simple and self contained example of existing Nixpkgs code that would gain from the changes scattered in these different PRs? |
40ec963 to
0a22f98
Compare
0a22f98 to
654106a
Compare
22aa3b5 to
ef724c3
Compare
a50d370 to
022aadb
Compare
ambroisie
left a comment
There was a problem hiding this comment.
This is pretty cool :-).
I'm kind of failing to see the value of occursOnlyOrAfterInArray compared to a two-step check of:
elem1in arrayelem1strictly occurs afterelem2(fails ifelem2is not present).
Same for occursOnlyOrBeforeInArray.
pkgs/build-support/setup-hooks/arrayUtilities/arrayDifference/arrayDifference.bash
Outdated
Show resolved
Hide resolved
pkgs/build-support/setup-hooks/arrayUtilities/arraysAreEqual/tests.nix
Outdated
Show resolved
Hide resolved
pkgs/build-support/setup-hooks/arrayUtilities/getElfFiles/getElfFiles.bash
Outdated
Show resolved
Hide resolved
pkgs/build-support/setup-hooks/arrayUtilities/mapIsSubmap/mapIsSubmap.bash
Outdated
Show resolved
Hide resolved
022aadb to
952a4e3
Compare
|
Rebased on master and force-pushed -- thank you for the review @ambroisie! I removed |
952a4e3 to
9587f6c
Compare
d2fc06d to
1f5adc8
Compare
|
I rebased on |
wolfgangwalther
left a comment
There was a problem hiding this comment.
Thanks for reducing scope, that helps a lot!
pkgs/build-support/setup-hooks/arrayUtilities/isDeclaredArray/package.nix
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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 to isDeclaredArray / isDeclaredMap makes 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.
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 through stdenv.
There was a problem hiding this comment.
Hm. I thought that whatever was available in stdenv was 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 in stdenv right now.
There was a problem hiding this comment.
One example is creating scripts which run outside stdenv -- for example, the magma.passthru.testers.all script introduced here: #414612.
Is there an easy way to ensure setup.sh and all the goodies it has are sourced in such scripts, without polluting the shell with a bunch of other variables?
There was a problem hiding this comment.
This sounds a bit like we'd maybe want a separation of something like stdenv and stdlib? (where stdenv can make use of stdlib!)
There was a problem hiding this comment.
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.
There is a use-case for sortArray in stdenv now: getAllOutputNames returns a different order of outputs depending on whether __structuredAttrs is 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.
I really like that these functions are separate so I can use them outside of things providing setup.sh and have an easy way to test them. I'm not opposed to the ol' copy/paste or including the files in mkDerivation's nativeBuildInputs... what are your thoughts? Although maybe copy-pasting would be better so any changes to these functions don't cause a rebuild of stdenv.
There was a problem hiding this comment.
I'm not opposed to the ol' copy/paste or including the files in
mkDerivation'snativeBuildInputs... what are your thoughts?
I don't think we can (or should) use something from mkDerivation.nativeBuildInputs in setup.sh, aka in core stdenv.
I don't like copy&paste either.
I think this was still a good idea:
This sounds a bit like we'd maybe want a separation of something like stdenv and stdlib? (where stdenv can make use of stdlib!)
pkgs/build-support/setup-hooks/arrayUtilities/getMapKeys/getMapKeys.bash
Outdated
Show resolved
Hide resolved
pkgs/build-support/setup-hooks/arrayUtilities/getMapKeys/getMapKeys.bash
Outdated
Show resolved
Hide resolved
pkgs/build-support/setup-hooks/arrayUtilities/getMapKeys/getMapKeys.bash
Outdated
Show resolved
Hide resolved
pkgs/build-support/setup-hooks/arrayUtilities/sortArray/sortArray.bash
Outdated
Show resolved
Hide resolved
pkgs/build-support/setup-hooks/arrayUtilities/sortArray/sortArray.bash
Outdated
Show resolved
Hide resolved
pkgs/build-support/setup-hooks/arrayUtilities/sortArray/sortArray.bash
Outdated
Show resolved
Hide resolved
1f5adc8 to
d0b738b
Compare
|
Addressed some comments, rebased, and force-pushed. Generally, I:
|
|
Sorry for the delay in getting back to your comments, I'll try to address those today. |
d0b738b to
e402b65
Compare
|
I've rebased, squashed, and force-pushed.
|
wolfgangwalther
left a comment
There was a problem hiding this comment.
I did not go through the extensive test-suite in detail, but they are a great base and give me confidence that the implementation is solid. I did test whether some sorting tests would fail without sorting behavior - they did, cool.
LGTM in principle.
pkgs/build-support/setup-hooks/arrayUtilities/getMapKeys/getMapKeys.bash
Outdated
Show resolved
Hide resolved
Signed-off-by: Connor Baker <[email protected]>
Signed-off-by: Connor Baker <[email protected]>
e402b65 to
4b80e59
Compare
|
Force-pushed changes:
|
|
Successfully created backport PR for |
There are patterns throughout the shell scripts in Nixpkgs (particularly those involving ELF files and CUDA) which can/should be componentized and made into a rigorous, tested library of functions. Given the push for
__structuredAttrs, it makes sense to offer high(er)-level array utilities rather than rely on each individual's knowledge of Bash. Promoting code-reuse in this way avoids disparate re-implementations of common patterns which introducing bugs, reduces line count, and makes intent unambiguous.This PR is the beginning of such re-usable library of Bash functions. Currently, it includes:
isDeclaredArray: a function which tests whether a nameref refers to a declared, indexed arrayisDeclaredMap: a function which tests whether a nameref refers to a declared, associative arraysortArray: a function which lexicographically sorts the contents of an indexed arraygetSortedMapKeys: a function which populates an indexed array with the keys of an associative array, sorted in lexicographic orderFunctions in (and to be added to) this library are largely written in destination-passing style, where the final arguments are the outputs in which the result of calling the function will be stored. (This is enabled by heavy use of namerefs.) Constraints on whether arguments may alias each other are documented for each relevant function.
The bulk of this PR is dedicated to testing these bash functions (generated with
cloc pkgs/build-support/setup-hooks/arrayUtilities --md):The second commit of this PR shows where such functionality would have been useful in implementing
testers.testEqualArrayOrMap.isDeclaredArrayandisDeclaredMapThese two functions underpin the library and allow testing the type of variable to which a nameref refers.
sortArrayThis function performs a stable-sort of an indexed array passed by reference, storing the results to the second argument, which is also an indexed array passed by reference.
Two points of note in the implementation:
printf, the format string is applied to each argument. In this way, we produce a NULL-delimited list of values for consumption bysort.sort, we specify it in a sub-shell to avoid polluting the enclosing environment.getSortedMapKeysThis function accepts an associative array by reference and stores the lexicographically sorted keys in the second argument, an indexed array passed by reference. Since order of traversal of associative arrays in Bash is not guaranteed to be consistent and Nix pursues reproducibility, being able to iterate over the values of a map in a deterministic fashion is a boon -- especially for test scripts.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.