treewide: *Flags convert to list from str #194256
Conversation
Artturin
requested review from
piegamesde,
mmahut,
RaghavSood,
thoughtpolice,
ttuegel,
matthewbauer,
marsam,
stigtsp,
zakame,
dasJ,
FRidh,
jonringer and
fricklerhandwerk
as code owners
| @@ -14,7 +14,7 @@ stdenv.mkDerivation rec { | |||
| nativeBuildInputs = [ cmake ]; | |||
| buildInputs = [ zlib ]; | |||
|
|
|||
| cmakeFlags = lib.optional stdenv.hostPlatform.isStatic [ | |||
| cmakeFlags = lib.optionals stdenv.hostPlatform.isStatic [ | |||
There was a problem hiding this comment.
on the other hand this was wrong before and should definitely be changed.
There was a problem hiding this comment.
splitting in to a separate pr because i got some ideas
|
I'd appreciate some detailed description of the changes including their motivation, preferably in the PR description as well as in the commit messages. Is this change a potentially breaking one? ("API" wise, ignoring mistakes in execution) |
ofborg
bot
requested review from
matthuszagh
and removed request for
RaghavSood,
dasJ,
FRidh,
jtojnar,
aanderse,
globin,
Ma27,
edolstra,
nbp,
Profpatsch and
limeytexan
| @@ -67,7 +67,7 @@ let | |||
| '' + | |||
| '' | |||
| machine.succeed( | |||
| "gnome-desktop-testing-runner ${testRunnerFlags} -d '${tested.installedTests}/share'" | |||
| "gnome-desktop-testing-runner ${toString testRunnerFlags} -d '${tested.installedTests}/share'" | |||
There was a problem hiding this comment.
Perhaps use lib.escapeShellArgs then?
There was a problem hiding this comment.
it's already escaped in the execute function of the tester
There was a problem hiding this comment.
That just ensures that the whole command is passed as a single argument to sh. But if we accept list of arguments, I would expect them to properly handle arbitrary values, unlike a argument string, where the user is expected to ensure individual arguments are properly escaped (compare Python’s subprocess.run with and without shell=True). Though annoyingly, e.g. makeFlags attribute currently does not follow this and values containing spaces will just break.
| @@ -12,7 +12,7 @@ | |||
| } @ args : | |||
|
|
|||
| let | |||
| mvnFlags = "-Dmaven.repo.local=$M2_REPO ${if doTest then "" else "-Dmaven.test.skip.exec=true"} ${extraMvnFlags}"; | |||
| mvnFlags = builtins.toString [ "-Dmaven.repo.local=$M2_REPO" "${if doTest then "" else "-Dmaven.test.skip.exec=true"}" "${extraMvnFlags}" ]; | |||
There was a problem hiding this comment.
Maybe something like:
| mvnFlags = builtins.toString [ "-Dmaven.repo.local=$M2_REPO" "${if doTest then "" else "-Dmaven.test.skip.exec=true"}" "${extraMvnFlags}" ]; | |
| mvnFlags = builtins.toString ([ | |
| "-Dmaven.repo.local=$M2_REPO" | |
| (lib.optionalString (!doTest) "-Dmaven.test.skip.exec=true") | |
| ] ++ extraMvnFlags); |
There was a problem hiding this comment.
extraMvnFlags is a string but github search shows only one non-nixpkgs usage of it so it should be fine to change it to a list
https://github.com/search?p=6&q=extraMvnFlags&type=Code
https://github.com/taktoa/nix-packages/blob/master/packages/kframework/default.nix
@taktoa
Artturin
force-pushed
the
treewides2
branch
3 times, most recently
from
a8fe748
to
007b1fc
Compare
*Flags implies a list slightly relevant: > stdenv: start deprecating non-list configureFlags NixOS#173172 the makeInstalledTests function in `nixos/tests/installed-tests/default.nix` isn't available outside of nixpkgs so it's not a breaking change
the argument to optional should not be list
*Flags implies a list
slightly relevant:
the makeInstalledTests function in
nixos/tests/installed-tests/default.nixisn't available outside of nixpkgs soit's not a breaking change
Description of changes
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes