treewide: *Flags convert to list from str

[Artturin]

*Flags implies a list

slightly relevant:

stdenv: start deprecating non-list configureFlags #173172

the makeInstalledTests function in nixos/tests/installed-tests/default.nix isn't available outside of nixpkgs so
it's not a breaking change

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• 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/)
• 22.11 Release Notes (or backporting 22.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
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[SuperSandro2000]

on the other hand this was wrong before and should definitely be changed.

[Artturin]

splitting in to a separate pr because i got some ideas

[Artturin]

did it in this pr

[piegamesde]

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)

[jtojnar]

Perhaps use lib.escapeShellArgs then?

[Artturin]

it's already escaped in the execute function of the tester

nixpkgs/nixos/lib/test-driver/test_driver/machine.py

Line 539 in 5d533d4

f"{timeout_str} sh -c {shlex.quote(command)} | (base64 --wrap 0; echo)\n"

[jtojnar]

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.

[Artturin]

switched

[jtojnar]

Maybe something like:

Suggested change

	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);

[Artturin]

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