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

Build failure: goModules using go workspaces with Go >= 1.22 #299096

Closed
nevivurn opened this issue Mar 26, 2024 · 5 comments · Fixed by #301928
Closed

Build failure: goModules using go workspaces with Go >= 1.22 #299096

nevivurn opened this issue Mar 26, 2024 · 5 comments · Fixed by #301928
Labels

Comments

@nevivurn
Copy link
Member

nevivurn commented Mar 26, 2024

Steps To Reproduce

Steps to reproduce the behavior:

  1. nix build github:NixOS/nixpkgs?rev=c726225724e681b3626acc941c6f95d2b0602087#talosctl.goModules --rebuild
  2. Build fails.

Build log

@nix { "action": "setPhase", "phase": "unpackPhase" }
Running phase: unpackPhase
unpacking source archive /nix/store/61c5gd4ifwiscggmksrgvcphaj2xslb8-source
source root is source
@nix { "action": "setPhase", "phase": "patchPhase" }
Running phase: patchPhase
@nix { "action": "setPhase", "phase": "updateAutotoolsGnuConfigScriptsPhase" }
Running phase: updateAutotoolsGnuConfigScriptsPhase
@nix { "action": "setPhase", "phase": "configurePhase" }
Running phase: configurePhase
@nix { "action": "setPhase", "phase": "buildPhase" }
Running phase: buildPhase
go: 'go mod vendor' cannot be run in workspace mode. Run 'go work vendor' to vendor the workspace or set 'GOWORK=off' to exit workspace mode.

Additional context

Starting with Go 1.22, go mod vendor fails when go.work is present and workspace mode is not explicitly disabled with GOWORK=off (upstream change). The affected packages already set GOWORK=off, but this is not passed down when building the goModules dependency.

As goModules is a fixed-output derivation, builds aren't failing in Hydra yet.

Affected packages (listed with a rudimentary grep -rF GOWORK and build confirmed to fail):

  • oneshot
  • opcr-policy
  • pop
  • talosctl
  • glauth
  • mcap-cli

Builds seem to succeed with Go 1.21, and overrideModAttrs = _: { env.GOWORK = "off"; }; also fixed the build, tested on talosctl.

Notify maintainers

Not sure who to ping, going with pkgs/build-support/go codeowners:
@kalbasit @Mic92 @zowoq

Metadata

Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.

$ nix-shell -p nix-info --run "nix-info -m"
 - system: `"x86_64-linux"`
 - host os: `Linux 6.7.10, NixOS, 23.11 (Tapir), 23.11.20240322.56528ee`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.18.1`
 - channels(root): `"nixos-23.11"`
 - nixpkgs: `/nix/store/cp5s6lac68bzypgc1chl8c0ss3rqxi27-source`

Add a 👍 reaction to issues you find important.

@nevivurn nevivurn added the 0.kind: build failure A package fails to build label Mar 26, 2024
@nevivurn
Copy link
Member Author

I think possible solutions include

  1. passing GOWORK=off to the goModules derivation
  2. not setting GOWORK=off, and using proxyVendor
  3. not setting GOWORK=off, and using go work vendor, added in 1.22

Things I noticed when testing w/ talosctl

  • vendoring w/ GOWORK=off (option 1) results in replaced modules in the talos repository appearing in the vendor directory
    • Ideally this should not be vendored, and it should be using $src/pkg/machinery/..., and not vendor/github.com/siderolabs/talos/pkg/machinery/...,
  • proxyVendor (option 2) results in a much larger goModules package, ~100MiB vs 500MiB. Unsure as to exactly why.
  • go work vendor (option 3) results in vendoring the packages used by eg. the modules in hack/ which are not actually required when building the main module.

@katexochen
Copy link
Contributor

proxyVendor (option 2) results in a much larger goModules package, ~100MiB vs 500MiB. Unsure as to exactly why.

If the number of affected packages is low, I'd say this could be a good solution, as it doesn't require changes to the builder. The size difference is interesting, I'd expect it to be smaller with proxyVendor. From my experience, using proxyVendor is faster in most cases (but I never checked size).

@zowoq
Copy link
Contributor

zowoq commented Mar 28, 2024

passing GOWORK=off to the goModules derivation

Very strongly suggest doing this.

The size difference is interesting, I'd expect it to be smaller with proxyVendor. From my experience, using proxyVendor is faster in most cases (but I never checked size).

I'd expect that proxyVendor would be both bigger and slower. The proxyVendor method used to be the default when buildGoModule was first added, then we switched to -mod=vendor and later proxyVendor was readded to handle edge cases.


I think this could work but not sure offhand if it might break something as the current assumption is that env isn't passed through to the vendor derivation.

diff --git a/pkgs/applications/networking/cluster/talosctl/default.nix b/pkgs/applications/networking/cluster/talosctl/default.nix
index ae3d300a9eb8..a8c326b288e2 100644
--- a/pkgs/applications/networking/cluster/talosctl/default.nix
+++ b/pkgs/applications/networking/cluster/talosctl/default.nix
@@ -15,7 +15,7 @@ buildGoModule rec {
 
   ldflags = [ "-s" "-w" ];
 
-  GOWORK = "off";
+  env.GOWORK = "off";
 
   subPackages = [ "cmd/talosctl" ];
 
diff --git a/pkgs/build-support/go/module.nix b/pkgs/build-support/go/module.nix
index 153b675d48ae..2aa589f6082b 100644
--- a/pkgs/build-support/go/module.nix
+++ b/pkgs/build-support/go/module.nix
@@ -78,6 +78,7 @@ let
     preBuild = args.preBuild or "";
     postBuild = args.modPostBuild or "";
     sourceRoot = args.sourceRoot or "";
+    env = args.env or null;
 
     impureEnvVars = lib.fetchers.proxyImpureEnvVars ++ [
       "GIT_PROXY_COMMAND"

@nevivurn
Copy link
Member Author

passing GOWORK=off to the goModules derivation

Very strongly suggest doing this.

I can test this over the weekend, but can I ask why this is the preferred option?
My concern is that we're ignoring go.work despite the upstream projects clearly intending to build with Go workspaces by providing that file. Now that it's become easier to vendor workspace dependencies with go work vendor, shouldn't we start supporting it in buildGoModule?

See also #291409 where it overrides the vendor command to go work vendor, referencing this upstream comment saying they don't support go mod vendor.

@katexochen
Copy link
Contributor

My concern is that we're ignoring go.work despite the upstream projects clearly intending to build with Go workspaces by providing that file.

I didn't take a look at the concrete example of talosctl, but I wouldn't assume that a go.work file suggest you to build binaries in workspace mode in any case. Usually it is used to simplify editor support (and committed for easier developer setup). I'm doing the same for the multi-module repos I'm maintaining.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants