Skip to content

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

Merged
ShamrockLee merged 9 commits intoNixOS:masterfrom
ShamrockLee:build-go-module-env
Dec 18, 2024
Merged

buildGoModule: Use the env attribute to pass environment variables #359641
ShamrockLee merged 9 commits intoNixOS:masterfrom
ShamrockLee:build-go-module-env

Conversation

@ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Nov 27, 2024 •
edited
Loading

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.

@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. labels Nov 27, 2024
@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 This PR adds or changes release notes labels Nov 28, 2024
@ShamrockLee ShamrockLee marked this pull request as ready for review November 28, 2024 05:30
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Nov 28, 2024

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 •
edited
Loading

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

ShamrockLee 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?

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 ShamrockLee mentioned this pull request Nov 28, 2024
Open
13 tasks
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Nov 28, 2024

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

#359744 is the plan.

@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Nov 28, 2024

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

kalbasit
kalbasit approved these changes Nov 28, 2024
View reviewed changes
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

doronbehar
doronbehar approved these changes Nov 28, 2024
View reviewed changes
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.

zowoq
zowoq reviewed Nov 28, 2024
View reviewed changes
katexochen
katexochen reviewed Nov 28, 2024
View reviewed changes
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Nov 28, 2024

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?

@wolfgangwalther wolfgangwalther mentioned this pull request Dec 2, 2024
Open
4 tasks
@ShamrockLee ShamrockLee mentioned this pull request Dec 7, 2024
Merged
12 tasks
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 10, 2024
@github-actions github-actions bot removed the 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. label Dec 17, 2024
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Dec 17, 2024

I rebased and resolved the merge conflict (seemingly caused by the recent treewide formatting).

This PR triggers no rebuild or cleanup except for the documentation-related packages, indicating that it works as intended. Do you think we should merge it now?

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 18, 2024
@wegank wegank added the 12.approvals: 3+ This PR was reviewed and approved by three or more persons. label Dec 18, 2024
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Dec 18, 2024 •
edited
Loading

ofborg-eval reports three new evaluation warnings after this change. (They are the warnings I added against the obsolete way of specifying the CGO_ENABLED environment variable.) I'll fix those packages before merging this PR.

This also informs us that the GH-Action-based evaluation currently misses the additional warnings generated after the changes.

@ShamrockLee ShamrockLee mentioned this pull request Dec 18, 2024
Closed
7 tasks
@ShamrockLee
treewide: remove dead code (shadowed GOOS and GOARCH specification)
9e13482 
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.
@ShamrockLee
buildGoModule: pass environment variables with the env attribute
44876c6 
This help Go modules support __structuredAttrs = true.

This commit doesn't touch GOFLAGS, which will be handled in subequent
changes.
@ShamrockLee
treewide: specify CGO_ENABLED with env.CGO_ENABLED
905dc8d 
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)
@ShamrockLee
etcd_3_5: fix env.CGO_ENABLED
7a2eff4
@ShamrockLee
promtail: fix implementation (env overriding)
6906681
@ShamrockLee
tailscale-gitops-pusher: fix CGO_ENABLED inheritance
44defef
@ShamrockLee
hello-go: fix longDescription (GOOS and GOARCH specification)
cf563cf 
This reverts commit e53afdda6e01c4886d58e86c2f84bcccacf4a744.
@ShamrockLee
buildGoModule: instruct users to specify CGO_ENABLED as env.CGO_ENABLED
3b1ec75 
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
rl-2505.section.md: add release note entry about the buildGoModule en…
af99530 
…v.CGO_ENABLED changes
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Dec 18, 2024

Rebased onto the top of master and resolved merge conflicts.

@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Dec 18, 2024

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

@ShamrockLee ShamrockLee merged commit f75d144 into NixOS:master Dec 18, 2024
@nixos-discourse
Copy link

nixos-discourse commented Dec 18, 2024

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

@drupol
Copy link
Contributor

drupol commented Mar 18, 2025

Should we backport this?

@drupol drupol mentioned this pull request Mar 18, 2025
Merged
13 tasks
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Mar 19, 2025 •
edited
Loading

Should we backport this?

This PR is backward-incompatible.

If it is for the backporting of future PRs, some changes need to be re-implemented on the release branch. Considering the inherent compatibility between the env-environment-specifying interface with the previous unstructured attribute-to-environment-variable interface, such re-implementation would be quite hacky.

@ShamrockLee ShamrockLee mentioned this pull request Sep 10, 2025
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zowoq zowoq zowoq left review comments

@kalbasit kalbasit kalbasit approved these changes

@wolfgangwalther wolfgangwalther wolfgangwalther approved these changes

@doronbehar doronbehar doronbehar approved these changes

@katexochen katexochen katexochen approved these changes

@adamcstephens adamcstephens Awaiting requested review from adamcstephens

@Mic92 Mic92 Awaiting requested review from Mic92

@TomaSajt TomaSajt Awaiting requested review from TomaSajt

Assignees

No one assigned

Labels

6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 3+ This PR was reviewed and approved by three or more persons.

Projects

Go team
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.