*FlagsArray do not work as people expect them to

[kvtb]

Describe the bug

buildGoModule ignores buildFlagsArray

For example nebula is built without -ldflags='-X main.Build=${version}' despite the instruction

nixpkgs/pkgs/tools/networking/nebula/default.nix

Line 20 in bff1304

buildFlagsArray = [ "-ldflags='-X main.Build=${version}'" ];

To Reproduce
Steps to reproduce the behavior:

$(nix-build -E "with (import (builtins.fetchTarball https://github.com/NixOS/nixpkgs/archive/master.tar.gz) {}); nebula")/bin/nebula -version
$(nix-build -E "with (import (builtins.fetchTarball https://github.com/NixOS/nixpkgs/archive/release-20.09.tar.gz) {}); nebula")/bin/nebula -version
$(nix-build -E "with (import (builtins.fetchTarball https://github.com/NixOS/nixpkgs/archive/release-20.03.tar.gz) {}); nebula")/bin/nebula -version

Expected behavior

Version: 1.3.0

Screenshots

Version: 

Notify maintainers
@zowoq @flokli @c00w @kalbasit @marsam @basvandijk @Mic92 @Br1ght0ne

Metadata
reproducible with release-20.03, release-20.09 and master

[Mic92]

It is not ignored in general for other packages i.e. glow:

nix-shell -p glow --run 'glow --version'

[Mic92]

fixed in #112104

[kvtb]

It is not ignored in general for other packages i.e. glow:

nix-shell -p glow --run 'glow --version'

No, this is not a nebula derivation bug, nebula is just a symptom
If another package works that means it might have a workaround for the bug like one you did in #112104

There is still a mismatch between buildFlagsArray and go build command line

If I run go build manually:

1. go build -ldflags='-X main.Build=${version}' ./cmd/nebula <-- this works
2. go build -ldflags '-X main.Build=${version}' ./cmd/nebula <-- this works
3. go build -ldflags= '-X main.Build=${version}' ./cmd/nebula <-- fails with "flag provided but not defined: -X main.Build"

You did the #112104 fix by switching correct flags (1) to invalid (3)
That means buildFlagsArray gets passed to go build command line in damaged form, breaking good flags and curing invalid flags.

[kvtb]

It should be fixed in buildGoModule code, and nebula "fix" is to be reverted.

[Mic92]

Can you provide a pull request with a fix? I don't see why I should revert the fix for nebula. It now correctly displays the version. If you have a better fix go ahead.

[kvtb]

Who cares how nebula displays the version?
It is too minor an issue to fix.

The real issue is:

1. buildFlagsArray = [ "-ldflags='-X main.Build=${version}'" ]; resulted in empty version
2. buildFlagsArray = [ "-ldflags=" "'-X main.Build=${version}'" ]; resulted in build success (should be build error)

[kvtb]

I suspect ' symbol is not escaped properly when passed to bash

[Mic92]

Who cares how nebula displays the version?
It is too minor an issue to fix.

Sorry for wasting your time. Please don't ping me in future.

[grahamc]

If you find a credible problem, please report it to the NixOS Security Team. If you'd like to remove the FUD, we could un-hide the comment.

[yorickvP]

Yes, this looks like a buildGoModule bug and we'd be happy to merge a fix for that. In the meantime, I don't think we should refuse to work around it, since that would cause breakage for users.

[jtojnar]

This is not just buildGoModule issue, it happens with stdenv.mkDerivation too.

If you pass an attribute containing a list of strings to mkDerivation, Nix will intercalate the elements with spaces and pass the whole list to the builder as a single string. The builder will then only be able to extract a single item from "${buildFlagsArray[@]}", containing all elements of the list concatenated together.

That means the single quotes in the original Nix expression got passed as literals (bash does not really interpret the values of variables passed to derivations), which is why it did not work. The “fixed” nebula only works because buildFlagsArray actually resolves into a single flag (even if the new quoting in the Nix expression is misleading).

That is why I recommend using buildFlags in Nix if you are certain your list items do not contain spaces. If any of them may contain spaces, you will have to resort to setting buildFlagsArray in Bash (e.g. in preBuild). Unfortunately, we cannot really do any better until we switch to __structuredAttrs.

[kalbasit]

Perhaps buildGoModule should expose ldflags as a list, and abstracts away the buildFlagsArray?

[kalbasit]

Perhaps buildGoModule should expose ldflags as a list, and abstracts away the buildFlagsArray?

Another reason why I think this might the right way to go is to abstract away the -s and -w flags we ask package maintainers to set on their packages.

[jtojnar]

For now, I opened a PR adding a lint for this to nixpkgs-hammering: jtojnar/nixpkgs-hammering#30

[kvtb]

Unfortunately, we cannot really do any better until we switch to __structuredAttrs.

Isn't it just a case to find implicit List's toString and replace it with lib.escapeShellArgs ?
toString works like lib.concatStringsSep " " but often used in places where lib.escapeShellArgs should be

[kvtb]

For now, I opened a PR adding a lint for this to nixpkgs-hammering: jtojnar/nixpkgs-hammering#30

If you have a linter, it would be nice to warn on every List's toString forcing user to make a choice between lib.concatStringsSep " " and lib.escapeShellArgs and write that explicitly.

[jtojnar]

Isn't it just a case to find implicit List's toString and replace it with lib.escapeShellArgs ?
toString works like lib.concatStringsSep " " but often used in places where lib.escapeShellArgs should be

Adding explicit escaping will not work, Nix passes everything to Bash as a literal string so all the backslashes and quotes would just become literal parts of the first array item, AFAICT. See #112104 (comment)