makeBinaryWrapper: protect wildcards in flags

[rhendric]

Description of changes

The split-on-spaces behavior of --add-flags and --append-flags was also causing wildcards to be interpreted when the wrapper was built. This is a quick fix to prevent that.

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/)
• 23.11 Release Notes (or backporting 23.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.

[rhendric]

test/make-binary-wrapper/cross.nix is failing on aarch64-linux per OfBorg, but I don't see how I could have caused this. Is this a known issue with that architecture?

[Artturin]

test/make-binary-wrapper/cross.nix is failing on aarch64-linux per OfBorg, but I don't see how I could have caused this. Is this a known issue with that architecture?

#255231

[rhendric]

Should this be a staging PR? I've never done one of those; based on the number of rebuilds OfBorg reports it seems like probably yes? But also this is not really a package and I don't know if staging is for changes like this.

[Artturin]

Yes staging
follow these instructions to rebase https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#rebasing-between-branches-ie-from-master-to-staging

[Artturin]

Whats the behaviour of makeWrapper

[rhendric]

Preserves wildcards. I amended the makeWrapper tests to cover this but didn't include that change because I wasn't changing makeWrapper; should I?

[Artturin]

Looks like it's already being tested

nixpkgs/pkgs/test/make-wrapper/default.nix

Line 67 in d64d76d

(mkWrapperBinary { name = "test-prefix-noglob"; args = [ "--prefix" "VAR" ":" "./*" ]; })

Let's do the same reenableGlob thing in here too

nixpkgs/pkgs/build-support/setup-hooks/make-wrapper.sh

Lines 66 to 70 in d64d76d

	local reenableGlob=0
	if [[ ! -o noglob ]]; then
	reenableGlob=1
	fi
	set -o noglob

Do you think noglob is only necessary in addFlags?

[rhendric]

I haven't done a meticulous review of that entire file for quote hygiene. --add-flags and --append-flags are the only makeWrapper flags documented to accept multiple arguments in one space-separated string, so I would expect the other handlers to quote their values and not need this.

[Artturin]

Any chance that there's something relying on the current behaviour?

[rhendric]

I certainly hope not, especially since makeWrapper doesn't do the same thing. If you want wildcard expansion when either implementation of makeWrapper is called, you can leave the wildcards unquoted/unescaped and bash will expand them first.

Here's the result of a quick ripgrep:

$ rg -e '--a(ppen|d)d-flags.*\*'
pkgs/development/tools/schemacrawler/default.nix
26:      --add-flags "-cp $out/lib/*:$out/config" \

pkgs/development/tools/flyway/default.nix
19:      --add-flags "-classpath '$out/share/flyway/lib/*:$out/share/flyway/drivers/*'" \

pkgs/development/libraries/rabbitmq-java-client/default.nix
23:      --add-flags "-Djava.awt.headless=true -cp $out/share/java/\* com.rabbitmq.examples.PerfTest"

pkgs/tools/security/cryptomator/default.nix
43:      --add-flags "--class-path '$out/share/cryptomator/libs/*'" \

pkgs/applications/networking/instant-messengers/signal-cli/default.nix
23:      --add-flags "-classpath '$out/lib/*:${libmatthew_java}/lib/jni'" \

pkgs/applications/office/jameica/default.nix
64:      --add-flags "-cp $out/share/java/jameica.jar:$out/share/jameica-${version}/* ${

All of these files use makeWrapper or makeShellWrapper, not makeBinaryWrapper.

[ncfavier]

The makeBinaryWrapper CLI is wrong: it should only take one flag per --add-flags argument, so as to allow whitespace. This is different from the makeWrapper CLI, which expects shell expressions so you can always pass quoted strings.

Nevertheless this change is obviously good.

[Gerg-L]

The makeBinaryWrapper CLI is wrong: it should only take one flag per --add-flags argument, so as to allow whitespace. This is different from the makeWrapper CLI, which expects shell expressions so you can always pass quoted strings.

Nevertheless this change is obviously good.

yes if possible doing that change at the same time as this one will reduce rebuilds

[Artturin]

The makeBinaryWrapper CLI is wrong: it should only take one flag per --add-flags argument, so as to allow whitespace. This is different from the makeWrapper CLI, which expects shell expressions so you can always pass quoted strings.
Nevertheless this change is obviously good.

yes if possible doing that change at the same time as this one will reduce rebuilds

rebuilds don't matter on staging as the builds are batched, every staging rebuilds everything anyway.

[Artturin]

Suggested change

	set -f
	# shellcheck disable=SC2086
	before=($1) after=($2)
	set +f
	set -o noglob
	# shellcheck disable=SC2086
	before=($1) after=($2)
	set +o noglob

[Gerg-L]

My bad: if both could hit master at the same time it'd be ideal

[Artturin]

@ofborg build tests.makeBinaryWrapper

testing if the ci fail is actual

x86_64-darwin ci fails

error: builder for '/nix/store/zi42bhf8ma8nkmrvbamxcbbjqfk7llqw-make-binary-wrapper-test-basic.drv' failed with exit code 139;
       last 1 log lines:
       > /private/tmp/nix-build-make-binary-wrapper-test-basic.drv-0/.attr-0l2nkwhif96f51f4amnlf414lhl4rv9vh8iffyp431v6s28gsr90: line 23: 11162 Segmentation fault: 11  env -i ./wrapped > env.txt
       For full logs, run 'nix log /nix/store/zi42bhf8ma8nkmrvbamxcbbjqfk7llqw-make-binary-wrapper-test-basic.drv'.
building '/nix/store/npr757669f71yiqrp2dha13sfppkw3a7-make-binary-wrapper-test-combination.drv'...

https://logs.ofborg.org/?key=nixos/nixpkgs.255208&attempt_id=60e79d85-f81c-4588-941e-d8ff527ad2da

it passed in a another makeBinaryWrapper PR https://github.com/NixOS/nixpkgs/runs/16818083116

[Artturin]

Now it timed out at llvm, I'll just assume the machine the test ran on was flaky.