buildGoModule: form GOFLAGS from goFlags
#359744
Conversation
github-actions
bot
added
6.topic: nixos
ShamrockLee
force-pushed
the
build-go-module-goflags
branch
4 times, most recently
from
088f5dd to
182f790
Compare
pkgs/build-support/go/module.nix
Outdated
| (lib.optional (!allowGoReference) "-trimpath"); | ||
| env = args.env or { } // { | ||
|
|
||
| GOFLAGS = lib.concatStringsSep " " finalAttrs.goFlags; |
There was a problem hiding this comment.
I'm not sure if we should do this from nix.
Can't we perhaps have goFlags be used as a bash array and set GOFLAGS later in bash?
There was a problem hiding this comment.
I am thinking about it. Operating on goFlags and forming GOFLAGS as needed makes the interface more consistent.
Still, as GOFLAGS acts as the "default flags for all Go commands if applicable," people are likely to expect GOFLAGS to be defined before the first execution of the go command. Defining and exporting GOFLAGS in one of the prePhases seems to defeat the purpose of making goFlags modifiable after evaluation.
There was a problem hiding this comment.
After examining the implementation again, it turns out the GOFLAGS is only used during the buildPhase and checkPhase provided by buildGoModule, and the configurePhase is still adjusting the flags.
Let's form the GOFLAGS at the beginning of the buildPhase then.
|
By the way, is the rationale behind having |
It's because a list is more structured than a string, and I hope to keep having a way to specify GOFLAGS with a list. As you said, the main benefit would be a smoother |
ShamrockLee
force-pushed
the
build-go-module-goflags
branch
from
182f790 to
00f676d
Compare
github-actions
bot
added
6.topic: python
6.topic: GNOME
ShamrockLee
changed the title
GOFLAGS with env and construct from goFlagsGOFLAGS from goFlags
|
I reimplemented this PR and made |
| @@ -209,6 +221,8 @@ in | |||
| '' | |||
| runHook preBuild | |||
|
|
|||
| export GOFLAGS="''${goFlags[*]}" | |||
There was a problem hiding this comment.
Not sure if we'd want to do it, but we could have goFlags be appended to GOFLAGS in case it's already set, for some reason.
I'm trying to look at this from a standpoint where the build script eventually becomes a hook, which wouldn't be able to cleanly enforce using goFlags over GOFLAGS.
There was a problem hiding this comment.
I'm trying to look at this from a standpoint where the build script eventually becomes a hook, which wouldn't be able to cleanly enforce using
goFlagsoverGOFLAGS.
If the "hook" hear means a "setup hook", it can as long as it provides the buildPhase and the checkPhase.
| @@ -385,6 +385,25 @@ appendToVar() { | |||
| fi | |||
| } | |||
|
|
|||
| removeAllPrefixedFromVar() { | |||
There was a problem hiding this comment.
Yes, this is something that would be useful globally, but should probably stay in module.nix until the testing is done.
wolfgangwalther
added
10.rebuild-darwin: 5001+
10.rebuild-linux: 5001+
and removed
10.rebuild-linux: 5000+
labels
ofborg
bot
added
10.rebuild-darwin-stdenv
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574/74 |
nix-owners
bot
requested review from
Artturin,
emilazy,
Ericson2314,
kalbasit,
katexochen,
Mic92,
philiptaron,
reckenrode,
RossComputerGuy and
zowoq
wegank
added
the
2.status: merge conflict
This PR introduces a new argument,
goFlags, to formGOFLAGSat build time at the beginning ofbuildPhaseandcheckPhase.This PR also adds a helper Bash function to
setup.shnamedremoveAllPrefixedFromVar, which removes all the elements with given prefixes from the Bash array/IFS-separated string variable of the given name.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.