Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buildGoModule: Use the env attribute to pass environment variables #359641

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Nov 27, 2024

Pass all environment variables with env and instruct users to do so in the Nixpkgs Reference Manual.

This is part of the efforts to allow buildGoModule to take __structuredAttrs = true. This change would only rebuild the documentation and is easy to merge (it turns out to be a tree-wide fix, but fortunately, reviewers can reproduce the tree-wide changes by string substitution). Other significant/mass-rebuilding changes are split away to make reviewing smoother.

One may notice that the added code's indentation differs from the surrounding code. That is because the original code is out-of-formatting and indented too much.

These changes should rebuild no packages other than the documentation.

Cc:
@doronbehar @kalbasit @zowoq (people who might be interested in buildGoModule)
@emilazy @wolfgangwalther (people who might be interested in __structuredAttrs)

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:
  • 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.

@ShamrockLee ShamrockLee force-pushed the build-go-module-env branch 5 times, most recently from 9665fbe to 99d7fb6 Compare November 28, 2024 05:28
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog labels Nov 28, 2024
@ShamrockLee ShamrockLee marked this pull request as ready for review November 28, 2024 05:30
@ShamrockLee
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 359641


x86_64-linux

⏩ 1 package blacklisted:
  • nixos-install-tools
✅ 2 packages built:
  • nixpkgs-manual
  • promtail

@emilazy
Copy link
Member

emilazy commented Nov 28, 2024

Does it maybe make sense to keep CGO_ENABLED as part of the interface here like GOFLAGS, just because it’s so ubiquitous? (No strong opinion here, just wondering. Isn’t GOFLAGS also just an environment variable ultimately?)

Edit: Ah, I see you have plans for GOFLAGS too, okay :)

@ShamrockLee
Copy link
Contributor Author

Does it maybe make sense to keep CGO_ENABLED as part of the interface here like GOFLAGS, just because it’s so ubiquitous?

It would be easier for now, as you are suggesting.

Still, if we plan to convert build helpers to support fixed-point attributes (finalAttrs: { ... }) like stdenv.mkDerivation does, the fewer removeAttrs the better. (Build helpers without removeAttrs can use overrideAttrs to achieve out-of-the-box conversion without waiting for #234651.)

@ShamrockLee
Copy link
Contributor Author

Edit: Ah, I see you have plans for GOFLAGS too, okay :)

#359744 is the plan.

@ShamrockLee
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 359641


x86_64-linux

⏩ 1 package blacklisted:
  • nixos-install-tools
✅ 1 package built:
  • nixpkgs-manual

Copy link
Member

@kalbasit kalbasit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ShamrockLee

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to buildGoModule look pretty good to me. Haven't iterated the packages' specific commits, but I guess they are trivial and should be good to go.

pkgs/build-support/go/module.nix Outdated Show resolved Hide resolved
@ShamrockLee
Copy link
Contributor Author

Regarding the use of environment variables to control the Go compiler, the current implementation of this PR shadows env.GO111MODULES, env.GOTOOLCHAIN, env.GOOS, and env.GOARCH, as the previous buildGoModule implementation did to the directly-specified version of these arguments.

Should we respect users' specifications for all these variables via env or keep shadowing them silently?

@TomaSajt TomaSajt self-requested a review November 28, 2024 08:32
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 28, 2024
buildGoModule internally shadow the GOOS and GOARCH specification as its
arguments.

Beside, GOOS and GOARCH inheritance from buildGoModule.go is broken,
since buildGoModule.go doesn't exist.
This help Go modules support __structuredAttrs = true.

This commit doesn't touch GOFLAGS, which will be handled in subequent
changes.
Programmatically prefixing "CGO_ENABLED =" and "CGO_ENABLED=0;" with
"env.", but excluding the files
* pkgs/build-support/go/module.nix (buildGoModule implementation)
* pkgs/development/compilers/go/* (the Go compiler)
* pkgs/build-support/docker/tarsum.nix (not using buildGoModule)
This reverts commit e53afdda6e01c4886d58e86c2f84bcccacf4a744.
Tell users to specify environment variables via `env`.

Rename the `var-go-CGO_ENABLED` documentation title
from `CGO_ENABLED` to `env.CGO_ENABLED`
and move the paragraphs under the `ssec-go-environment`.
@ShamrockLee
Copy link
Contributor Author

Rebased and resolved merge conflicts with #359798.

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Nov 29, 2024
@TomaSajt
Copy link
Contributor

Is there a plan to also move GOFLAGS in the future?
Currently, the nix logic adds additional values to GOFLAGS, but that could also be done from bash as well.
I did it like this:

export GOFLAGS
# currently pie is only enabled by default in pkgsMusl
# this will respect the `hardening{Disable,Enable}` flags if set
if [[ $NIX_HARDENING_ENABLE =~ "pie" ]]; then
prependToVar GOFLAGS "-buildmode=pie"
fi
if [ -z "${allowGoReference-}" ]; then
appendToVar GOFLAGS "-trimpath"
fi
if [ -z "${proxyVendor-}" ]; then
appendToVar GOFLAGS "-mod=vendor"
fi

(This doesn't handle the current nix-side GOFLAGS validation warnIfs)


(I don't have a lot of experience with different variable types in bash, so I might have some misconceptions.)

@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Nov 29, 2024

Is there a plan to also move GOFLAGS in the future?

It's #359744.

The [placeholder] commit puts only the GOFLAGS and not other environment variables under env = {...} as a proof of concept. Once this PR is merged, I'll rebase #359744, dropping the [placeholder] ... commit and put GOFLAGS into env = { ... } together with other environment variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

7 participants