Skip to content

treewide: go-modules -> goModules#242905

Merged
Artturin merged 1 commit intoNixOS:stagingfrom
Artturin:gomodu1
Jul 13, 2023
Merged

treewide: go-modules -> goModules#242905
Artturin merged 1 commit intoNixOS:stagingfrom
Artturin:gomodu1

Conversation

@Artturin
Copy link
Member

In 787af0f #241707
I had to change ${go-modules} to $goModules to allow overrideAttrs to work; However, env vars cannot contain -, so i had to change go-modules too. This in turn broke nix-update because it uses the go-modules attr.

Instead of making nix-update more complicated, make go-modules naming match cargoDeps.

fd --type f | xargs sd '\bgo-modules\b' 'goModules' and revert change to pkgs/applications/misc/dstask/default.nix

release note added

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog This PR adds or changes release notes 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. labels Jul 11, 2023
@Artturin Artturin force-pushed the gomodu1 branch 2 times, most recently from 1436ce3 to c9b8d09 Compare July 11, 2023 19:34
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 1001-2500 This PR causes many rebuilds on Darwin and should most likely target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. labels Jul 11, 2023
@Artturin Artturin changed the base branch from master to staging July 12, 2023 15:01
@Artturin Artturin force-pushed the gomodu1 branch 3 times, most recently from c69f955 to 406c439 Compare July 13, 2023 17:54
In 787af0f
I had to change ${go-modules} to $goModules to allow overrideAttrs to work;
However, env vars cannot contain -, so  i had to change go-modules too.
This in turn broke nix-update because it uses the go-modules attr.

Instead of making nix-update more complicated, make go-modules naming match cargoDeps.

`fd --type f | xargs sd '\bgo-modules\b' 'goModules'`
and revert change to pkgs/applications/misc/dstask/default.nix
and pkgs/servers/http/dave/default.nix
and pkgs/os-specific/darwin/plistwatch/default.nix

release note added
@Artturin Artturin merged commit c831e79 into NixOS:staging Jul 13, 2023
@Artturin Artturin deleted the gomodu1 branch July 13, 2023 22:36
@vcunat
Copy link
Member

vcunat commented Jul 23, 2023

There are various go packages that broke due to this merge. (They build again when I revert this merge. I'm looking at staging-next branch.) For example, k2tf or kuma-cp. For more it's possible to look at https://hydra.nixos.org/eval/1797817?compare=1797813#tabs-now-fail

/cc PR #244111

EDIT: more packages I've noticed regressing because of this: goofys, tinygo

@Artturin
Copy link
Member Author

Weird that a attr name change would affect the build
Can take a look in a few hours

@Artturin
Copy link
Member Author

Artturin commented Jul 23, 2023

I get a failure on master too

nix build ".#k2tf.go-modules" --rebuild

so the issue is not related to this change, instead the issue has been there for maybe a long time and just hasn't shown because the go-modules is a FOD.

The name change causes a rebuild but not a hash change

@Artturin
Copy link
Member Author

Artturin commented Jul 23, 2023

We can work around the issue by not causing a rebuild

diff --git a/pkgs/build-support/go/module.nix b/pkgs/build-support/go/module.nix
index ced7873e6a2b..6c2284a7a98d 100644
--- a/pkgs/build-support/go/module.nix
+++ b/pkgs/build-support/go/module.nix
@@ -54,7 +54,7 @@ let
 
   goModules = if (vendorHash == null) then "" else
   (stdenv.mkDerivation {
-    name = "${name}-goModules";
+    name = "${name}-go-modules";
 
     nativeBuildInputs = (args.nativeBuildInputs or [ ]) ++ [ go git cacert ];

@Artturin
Copy link
Member Author

the last message from go mod vendor is

k2tf> go: replacement path ./vendor/k8s.io/cli-runtime/pkg/kustomize/k8sdeps/transformer inside vendor directory

@Artturin
Copy link
Member Author

@kalbasit @Mic92 @zowoq @qbit

@Artturin
Copy link
Member Author

Artturin commented Jul 23, 2023

tinygo.goModules fails with

error: illegal path references in fixed-output derivation '/nix/store/06v7rn03bgsnzvv89dn8i2a6kap1fijl-tinygo-0.26.0-goModules.drv'

on master and staging-next

This PR didn't cause any issues, but instead exposed buggy behavior in new go versions

vcunat pushed a commit that referenced this pull request Jul 24, 2023
This is a workaround to avoid exposing non-reproducible .goModules
At least for now.
#242905 (comment)
@vcunat
Copy link
Member

vcunat commented Jul 24, 2023

OK, I took your workaround to avoid immediate breakage for now. (7da39a7)

@Artturin
Copy link
Member Author

Artturin commented Jul 24, 2023

Will need to add snippets like this

   broken = true; # vendor isn't reproducible with go > X.XX: nix-build -A $name.goModules --check

But it can wait.
Many of the replacements in this PR were in those kinds of comments.

go vendoring is buggy :(

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Jul 24, 2023

We shouldn't have to revert this change. (Only changing the name is smart move without causing more regressions in things that already adopted the new variable name.) Everything mentioned that broke with it, was hanging on a thin string and only worked as long as the FOD worked. Those packages have been only hold together by the FOD and aren't properly maintained.

But it can wait.

This situation has been like this at least since the go bump from 1.19 to 1.20 and probably before. We should mark all those packages broken to finally get some maintainer attention, so that they fix their packages.

go vendoring is buggy :(

Go vendor is first a bitch and if anything is off, it breaks.

@SuperSandro2000
Copy link
Member

Will need to add snippets like this

We already have some of those from previous go bumps and I am creating a PR right now to add more of those, so that people that are interested in those softwares speak up and fix them.

@SuperSandro2000
Copy link
Member

I have created #245159 with all failing builds I found in the history of this and want to get some awareness and attention to fix those.

@Artturin
Copy link
Member Author

Thanks Sandro

@reckenrode reckenrode mentioned this pull request Jul 25, 2023
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 1001-2500 This PR causes many rebuilds on Darwin and should most likely target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants