makeBinaryWrapper: --add-flags arguments now properly escaped

[BirdeeHub]

arguments passed to --add-flags were ALWAYS split by spaces regardless of how you passed them. This PR also applies to --append-flags as well

This splits them correctly as if they were shell arguments.

Addresses #330471

--add-flags is useless for makeBinaryWrapper for a ton of programs because of the inability to accept arguments with spaces regardless of escaping.

I also hope to introduce a --run in a future PR (Edit: although, unsure if I will be able to in a way that makes sense to add) such that makeBinaryWrapper becomes truly a drop in replacement for makeWrapper, but for now, this should be fixed.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• 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/)
• 25.05 Release Notes (or backporting 24.11 and 25.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.

---

Add a 👍 reaction to pull requests you find important.

[BirdeeHub]

I wasnt able to properly test on the whole set of packages that depend on this change, partially because it's over 5000 packages but also...

Due to a serious of completely unrelated unfortunate mistakes involving updating a system flake, doing a full garbage collection run, and then rebooting, my computer no longer boots and I can't fix it until tomorrow when I get back to my better computer.

I did, however, get to test it out with several things still, and it works as expected as far as I have been able to tell!

I will update with my findings tomorrow when I can do that, feel free to help me test it in the meantime.

[BirdeeHub]

Also, I missed my invite to the GitHub organization when I made my first contribution over a year ago, so I still can't request anyone for review.

If anyone knows how to send me a new invite, PLEASE let me know and do it, I would really appreciate it. Thanks!

@RossComputerGuy you were last person in the commit log so I'm pinging you to either review it, or set someone else to review.

[BirdeeHub]

Tfw you paste the wrong link... Sorry about the random mention. Phone issues

[GaetanLepage]

This PR needs to target staging.

[BirdeeHub]

Hmmm. Makes sense.

Alright. Thank you!

Hopefully I can do it on my phone but otherwise I'll do that tomorrow.

To be clear I didn't expect it to be accepted yet, it needs many to review because it is used in so many places.

But it works! At least as far as I'm aware so far!

[BirdeeHub]

It is easier to change the branch on a phone than it is to reopen it apparently

[GaetanLepage]

It is easier to change the branch on a phone than it is to reopen it apparently

@BirdeeHub, with all due respect, I suggest you use the GitHub PR and issue threads more sporadically.
They are not meant to be used as chat conversations. Messages should have a clear goal and convey relevant information.
I noticed that you tend to write whatever is on your mind directly on GitHub.
Please remember that each message leads to pinging/emailing each person involved in the thread.

Thank you very much for your comprehension.

(For instance, we don't necessarily care that you are on your phone. Your complaints about the GH UI can be made on more informal channels.)

[RossComputerGuy]

@RossComputerGuy you were last person in the commit log so I'm pinging you to either review it, or set someone else to review.

It was a basic touch heh. Maybe @philiptaron may be willing to look at this? I don't have the bandwidth to look at this, I know Philip has done bash stuff before and I think he would be a good person to look at this.

[BirdeeHub]

https://github.com/BirdeeHub/testBinWrapper

Hey everyone.

I made a repo with makeBinaryWrapper separate from nixpkgs for testing purposes for people.

If you try to pull from the new branch, EVERYTHING will build from source because makeBinaryWrapper is used everywhere.

So, you can import this with

inputs = {
  makeBinWrapper.url = "github:BirdeeHub/testBinWrapper";
  makeBinWrapper.flake = false;
};

or with fetchGit

and then

makeBinaryWrapper = pkgs.callPackage inputs.makeBinWrapper {};

and put that in nativeBuildInputs of the package you wish to test instead.

That way you only rebuild the packages you intend to test and not every single thing ever.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/makebinarywrapper-change/62885/1

[ElvishJerricco]

This must be marked as a draft until you've done a treewide sweep to fix all uses that will be affected by this. A quick look with git grep --name-only 'makeBinaryWrapper' | xargs grep -C 2 -e --add-flags reveals that it's quite a lot. Enough that in fact I would recommend making a different flag with better behavior rather than trying to fix an existing flag.

Inaccurate

[BirdeeHub]

A quick look with git grep --name-only 'makeBinaryWrapper' | xargs grep -C 2 -e --add-flags reveals that it's quite a lot. Enough that in fact I would recommend making a different flag with better behavior rather than trying to fix an existing flag.

As to the concern about existing behavior. 1. the docs say it is a drop in replacement for makeWrapper, and that would include the names of the flags. It could be a new wrapper entirely, we could factor this out into 2 different ones?

Existing behavior was that it only accepted single value arguments with no spaces. You could do an --add-flags that had several single value flags in it, but the moment you want a quoted string with a space in it, that still gets split, and you are out of luck.

So basically all people using it were forced into a restricted set of arguments they could pass, and would sanitize them such that they have no spaces beforehand, otherwise their quoted content with spaces would get all chopped up.

Now they just... dont have to. In theory this allows a superset of behaviors but in very few cases should it affect any existing ones that were actually useable?

But yes, more sweeping checks must be done I agree.

[ElvishJerricco]

Ok I think I misunderstood what this shell code is accomplishing. This is preserving the old behavior, unless the old values contained shell-escaping strings, e.g. --add-flags='x\ y'. I still worry that it could risk breaking a number of things, but I'm much less worried now. I do still wonder if it would be better to just add a different behaving option.

[BirdeeHub]

I am also worried that it could break a bunch of things. I fully agree. I need to get as many people building stuff and checking as possible because when I replace my nixpkgs, everything builds successfully that has had to build so far, except, it takes 3 hours to build most things when I replace nixpkgs itself, rather than targetedly replace it for derivations. Because there are just that many packages that use this.

I am going to mark it as a draft and figure out how to add tests to it maybe for existing behavior? I will use your grep and grab some real ones maybe? Im going to let the CI finish its last 2 checks out of curiosity and then put it back as draft.

But ideally, this will be drop in for the old one, and I am confident enough in saying that that I THINK we can get away with changing it and doing the makeWrapper interface we have created justice

[BirdeeHub]

I can explain how my addition works Edit: OUTDATED

in the loop up top, it collects a beforeFlags and afterFlags (I didnt change this)

those are strings, made up of space appended values from --add-flags and --append-flags

Those are $1 and $2 to the addFlags function

Prior, it just treated those directly as arrays.

before=($1) after=($2)

Now, there is the arg2list function

arg2list() {
    local toset=$1
    shift 1
    eval "$toset=($@)"
}
# USAGE:    `arg2list before $1` where the name passed as 1st arg is the name of the local variable to set

We pass $1 and $2 unescaped to these function calls, just like how we put them in () unescaped before

This allows them to expand as though they were a normal argument list to the arg2list function.

And then we grab the name of the local to set, shift 1 to get it out of the args, and do the $@ which has special behavior under expansion which keeps arguments grouped properly when expanded within " chars (inside an eval so we can set the local that was passed)

In this case, the key difference is that "$@" keeps the args expanded correctly together as one would expect, even when the "$@" arent directly on either side of it, such as in eval "$toset=($@)". This ensures the created array has been expanded correctly, whereas putting them directly into the array did not.

This allows arguments passed to --add-flags to be treated exactly as if they were normal shell arguments and then assigned to the local variable as an array.

Prior, it behaved more like an --add-flag than an --add-flags and if your single flag had a quoted string with a necessary space you were out of luck and had to use the regular makeWrapper. Using it as --add-flag like it worked before still works. Hence why I am reasonably confident this will break very few things if any, despite it being so ubiquitous, and this brings it in line with the more familiar pkgs.makeWrapper.

Previously impossible

lib.escapeShellArgs [ "--add-flags" "--cmd 'lua somestuff'" ]

lib.escapeShellArgs [ "--add-flags" "--cmd" "--add-flags" "'lua somestuff'" ]

This worked and still does

lib.escapeShellArgs [ "--add-flags" "-u ${somepath}" ]

And also this did and still does

lib.escapeShellArgs [ "--add-flags" "-u" "--add-flags" "${somepath}" ]

[BirdeeHub]

Also, to be clear to people on the actual usecase for this function.

its mostly for systems that refuse things that arent actual binaries being used as shebang interpreters as far as I can tell.

Some people think its going to be faster because its compiled.

Its not. At least not from my testing. Bash was made for this. Measuring the difference at all would be challenging.

But if your system wont let you wrap python and use it in a shebang unless its a binary, this is the way to go.

So it should be used for such programs.

And if you want to let people pass --run as an argument, the standard makeWrapper is always going to be the better bet for speed, even if thats implemented for this function, because it already is bash, it can just be ran with no bells and whistles inline.

But nonetheless, this is a bug and should be fixed in my opinion.

[BirdeeHub]

added some tests. I don't think I need to make this a draft anymore because it has tests now and is done and works and stuff?

[Gerg-L]

From all my testing this looks to be correct

[BirdeeHub]

made shellcheck happier (by adding more comments disabling it). I really wish I could find a better way to do arg2list but nothing I have tried that is supposed to work, or actually makes sense, seems to actually work other than eval "$toset=($@)".

Ive tried like 50 variations of this with looping over "$@" , or breaking it up into 2 functions and inlining them, and several other things. I can only seem to get this version to work correctly.

We need to get someone to test this on darwin and make sure it still works.

[BirdeeHub]

WAY BETTER

    eval "before=($1)"
    eval "after=($2)"

It is no longer the most cursed bash thing I have ever seen in my life.

Im happy enough with it. It should work in all the same places the old one did in terms of mac vs linux stdenvs, and it no longer looks absolutely crazy, and it gets rid of all the shellchecks, even the ones there before

[BirdeeHub]

I can't meaningfully test this on my machine.

There are too many packages. It just crashes my PC.

What would be the best way of leveraging existing CI infrastructure to test this change against all the packages it causes rebuild for?

I can run nix flake check -Lv on nixpkgs, and test random packages in isolation by using overrideAttrs on their nativeBuildInputs, but thats about the most I can do on my personal machine.

I also can only test changes on x86_64-linux on my machine. It would be nice to run this on all systems to find out for sure.

[ncfavier]

This change goes in the wrong direction: see my previous comment here #255208 (review)

It is unreasonable to expect makeBinaryWrapper to be a drop-in replacement for makeShellWrapper, because shell wrappers can use shell features at runtime, e.g. make use of variable expansion to use environment variables that are only defined at runtime, whereas a binary wrapper would have to essentially reimplement a shell to get this functionality, which I don't think is desirable.

The correct interface for a binary wrapper, which would also allow arguments with whitespace, is to have an --add-flag (and corresponding --append-flag) flag that adds a single verbatim flag to the wrapper's command line. One can then do things like makeBinaryWrapper --add-flag --cmd --add-flag 'lua somestuff'.

(Making this a new, separate flag seems more realistic than changing --add-flags's behaviour, given how breaking of a change this is.)

[BirdeeHub]

This change goes in the wrong direction: see my previous comment here #255208 (review)

It is unreasonable to expect makeBinaryWrapper to be a drop-in replacement for makeShellWrapper, because shell wrappers can use shell features at runtime, e.g. make use of variable expansion to use environment variables that are only defined at runtime, whereas a binary wrapper would have to essentially reimplement a shell to get this functionality, which I don't think is desirable.

The correct interface for a binary wrapper, which would also allow arguments with whitespace, is to have an --add-flag (and corresponding --append-flag) flag that adds a single verbatim flag to the wrapper's command line. One can then do things like makeBinaryWrapper --add-flag --cmd --add-flag 'lua somestuff'.

(Making this a new, separate flag seems more realistic than changing --add-flags's behaviour, given how breaking of a change this is.)

This change goes in the wrong direction: see my previous comment here #255208 (review)

It is unreasonable to expect makeBinaryWrapper to be a drop-in replacement for makeShellWrapper, because shell wrappers can use shell features at runtime, e.g. make use of variable expansion to use environment variables that are only defined at runtime, whereas a binary wrapper would have to essentially reimplement a shell to get this functionality, which I don't think is desirable.

The correct interface for a binary wrapper, which would also allow arguments with whitespace, is to have an --add-flag (and corresponding --append-flag) flag that adds a single verbatim flag to the wrapper's command line. One can then do things like makeBinaryWrapper --add-flag --cmd --add-flag 'lua somestuff'.

(Making this a new, separate flag seems more realistic than changing --add-flags's behaviour, given how breaking of a change this is.)

First of all, thank you for the context, it is very useful.

While I think it unreasonable for makeShellWrapper and makeBinaryWrapper to be perfectly drop in replacements, I would think the only reason for that would be --run, and not --add-flags?

In the manual it just says "they implement the same bash functions" which is basically just incorrect?

I think it would be good for them to be compatible for the arguments they share.

But I also agree that the shell variables shouldnt be expanding.

With the shell variables expanding it would be massively breaking, and tbh I didnt realize my change did that.

I would think that if the shell variables dont expand, there shouldnt be much that breaks if anything.

I have managed to implement that today, proper grouping without variable expansion or globbing, but I had to use xargs, and am working on a way to use only stdenv functions still, as I dont think xargs is in the stdenv on all systems? If I can use xargs, I have a new proposal for how to do it that doesnt expand variables.

While I agree that --add-flag would be good for makeBinaryWrapper, I think that if that were to be added, it would need to be added to makeShellWrapper as well.

But I also think that, with the addition of --add-flag for both, the difference for --add-flags would be even more confusing. In addition, its limitations are not mentioned anywhere I can find in official documentation.

Basically, at the VERY least the docs about these functions should change a lot but that doesn't feel good to me. Without types, consistency is good for making things intuitive, rather than having to read up on every difference.

[ncfavier]

OK, let's look at a concrete example:

nixpkgs/pkgs/by-name/zu/zulip/package.nix

Lines 54 to 57 in 9c6d45c

	makeShellWrapper '${lib.getExe electron_35}' "$out/bin/zulip" \
	--add-flags "$out/share/lib/zulip/app.asar" \
	--add-flags "\''${NIXOS_OZONE_WL:+\''${WAYLAND_DISPLAY:+--ozone-platform-hint=auto --enable-wayland-ime=true}}" \
	--inherit-argv0

The Zulip wrapper uses variable expansion at runtime: the generated shell script looks like

#! /nix/store/…-bash-5.2p37/bin/bash -e
exec -a "$0" "/nix/store/…-electron-35.1.4/bin/electron" \
  /nix/store/…-zulip-5.12.0/share/lib/zulip/app.asar \
  ${NIXOS_OZONE_WL:+${WAYLAND_DISPLAY:+--ozone-platform-hint=auto --enable-wayland-ime=true}} \
  "$@" 

This means that, when you run the wrapper, bash will look up NIXOS_OZONE_WL and WAYLAND_DISPLAY in the environment and change the list of arguments depending on whether both are nonempty, because that is what those variable expansions mean.

By contrast, the C programs that makeBinaryWrapper generates are "stupid": all they can do is hardcode a static list of arguments to pass to the wrapped program via execv (you should convince yourself that your change does not allow Zulip to switch to makeBinaryWrapper). If we wanted makeBinaryWrapper to be a drop-in replacement for makeShellWrapper, then we would need to somehow partially evaluate a shell against the script generated by makeShellWrapper in order to turn it into an executable (some kind of first Futamura projection). One possible way to do this without reimplementing a shell in C would be to run bash inside the binary wrapper and get it to output the list of arguments to feed to execv. This is a more complex project than what your PR suggests anyway, which is why I suggest to keep things simple and embrace the binary wrapper's dumb list-of-arguments interface for now.

For some context, the binary wrapper was not introduced for performance reasons, but in order to work around limitations of interpreter/shebang scripts (see #124556 and some of the discussion in #126248).

Basically, at the VERY least the docs about these functions should change

Yes, that I can get behind.

[ncfavier]

Actually, perhaps an easier way to get a drop-in replacement would be to generate something like the following C program, where SHELL_WRAPPER is the wrapper generated by makeShellWrapper:

int main(int argc, char **argv) {
    char **argv_ = calloc(argc + 3, sizeof(*argv_));
    argv_[0] = "bash";
    argv_[1] = "-c";
    argv_[2] = ". SHELL_WRAPPER";
    memcpy(argv_ + 3, argv, sizeof(*argv) * argc);
    execv("BASH_PATH", argv_);
}

The use of -c ensures that argv[0] is correctly passed through.

This is an idea for a future PR anyway.

[BirdeeHub]

Actually, perhaps an easier way to get a drop-in replacement would be to generate something like the following C program, where SHELL_WRAPPER is the wrapper generated by makeShellWrapper:

int main(int argc, char **argv) {
    char **argv_ = calloc(argc + 3, sizeof(*argv_));
    argv_[0] = "bash";
    argv_[1] = "-c";
    argv_[2] = ". SHELL_WRAPPER";
    memcpy(argv_ + 3, argv, sizeof(*argv) * argc);
    execv("BASH_PATH", argv_);
}

The use of -c ensures that argv[0] is correctly passed through.

This is an idea for a future PR anyway.

This is actually an interesting idea.

I did actually figure out how to do it just in bash in the wrapper without expanding things by the way, and it seems to work pretty well against a wide range of tests which I added?

It doesnt do runtime expansion, but it does split them correctly!

But yes, I do see what you are saying about it not being a drop in

And yes, I am aware that its for wrapping interpreters, but then those same interpreters expose the wrapper args as an interface... Also, because the docs aren't clear, many people think the binary wrapper is faster, and use it in places it shouldn't be. Not being able to include spaces AT ALL in wrapper provided arguments is a very hard limitation for some programs.

At the very least an --add-flag is needed, but I wanted to try 1 more time to see if it could be just a bit better than it is now.

Last attempt, check it out. It is at the very least cool if nothing else.

[NobbZ]

@ncfavier which basically ruins the idea of the binary wrapper…

One of its main points was to remove bash from closures where bash is only used to wrap the original program.

In this situation there was not any reason to use the binary wrapper anymore, as the shell wrapper is much easier to debug anyway…

[ncfavier]

@NobbZ I don't recall that as "one of the main points". Could you link to the relevant discussions? #124556 and #23018 are quite clear about the motivations for introducing binary wrappers, as I mentioned in a previous comment.

[BirdeeHub]

Yeah, I think I've been pretty clear about what the binary wrapper is useful for in general on this PR as well.

I'm sure there are some edge cases of things that it is also good for, but its definitely for interpreters mainly.

If this version doesnt look good to anyone, despite being a completely functional tokenizer in bash in ~100 lines, I can maybe try my luck at an --add-flag some other day.

Also, are we not allowed to use -n but we can use xargs? Im kinda surprised by that. I wanted to pass out my array and it throws when you try to use local -n out=$1 instead of local outvarname=$1

[rhendric]

despite being a completely functional tokenizer in bash in ~100 lines

Not despite, because. We should not be maintaining a ~100-line Bash tokenizer hidden away in this wrapper script builder for a use case this misguided. Every working software developer learns that a line of code is a debt, not an asset.

Sorry kid, you clearly are excited about getting this working, and that enthusiasm is wonderful, but each new iteration on this PR is worse and worse. Try --add-flag if you want, but keep it simple please.

[BirdeeHub]

--add-flag is a much simpler task yes. Less satisfying but much simpler. I personally feel --add-flags creates a lot of debt at the use site with the bug left in it, a lot of code spent removing all possible spaces from commands, and its used like, EVERYWHERE, so that's a lot of tech debt. That was my main motivation was to address that, and to remove footguns for people trying it out used to makeShellWrapper. I would bet that 100 lines of code here would save a lot more than 100 lines of time for others, but I may be wrong, and it doesn't necessarily matter all that much.

General conversion to --add-flag would address most of those problems. When I add that I will update both makeShellWrapper and makeBinaryWrapper to include the new flag. Since they are already separated as arguments, that is a much simpler task.

It was the last iteration for a reason, no good could come of going much further. Although its definitely neat and I'm quite fond of it regardless.

[BirdeeHub]

I already have a working first draft for makeBinaryWrapper --add-flag in my test repo. I also have a second one with the additions to makeShellWrapper, Im working on refining those, and then also help updates

Passing arrays in local variables without -n is annoying but otherwise, quite easy.