Problematic behavior of buildGoModule in conjunction with vendorSha256

[Ma27]

Describe the bug
I figured that it may be better to move further discussion about the recent buildGoModule-changes (#86376 (comment)) to a new issue.

I see the following problems with the switch to -mod=vendor:

• A lot of packages[1] now have to override dependencies using overrideModAttrs. In my opinion this is a serious regression since we now push the task of solving dependency problems to our package maintainers, previously the build-script was able to do this automatically.

• I also don't understand why those huge vendor.patch-files are needed, in that case I think that it's even easier to just use a generated deps.nix. To be fair, this seems to be only relevant for documize-community atm where I was able to remove it during the last update[2].

I'd like to know why those hacks are even needed and discuss about better alternatives (AFAICS modSha256 will be removed in the upcoming release and we now seem to prefer buildGoModule over buildGoPackage).

Also relevant: #86282, #89128, #52469 & #84826

Notify folks who are probably interested/involved into this topic

cc @Mic92 @c00w @nlewo @zimbatm @doronbehar @kalbasit @emilazy

---

[1] When grepping for overrideModAttrs, I found the following packages:

• mautrix-whatsapp
• tailscale
• blockbook
• go-ethereum
• gobetween
• tinygo
• saml2aws
• hugo
• aerc

[2] 7857634

[doronbehar]

I'd like to know why those hacks are even needed and discuss about better alternatives (AFAICS modSha256 will be removed in the upcoming release and we now seem to prefer buildGoModule over buildGoPackage).

Maybe you'd be interested in https://discourse.nixos.org/t/creating-vendor-directories-directly-in-srcs-of-go-and-rust-packages-so-fixed-output-derivations-wont-be-needed/7367/6

[c00w]

Derp just saw this - I dropped a massive response in #86376 (comment)

A lot of packages[1] now have to override dependencies using overrideModAttrs. In my opinion this is a serious regression since we now push the task of solving dependency problems to our package maintainers, previously the build-script was able to do this automatically.

I think I wrote almost all of those patches and it wasn't too bad. Mostly we're copying a couple more files into the vendor directories (which would have been impossible with the go module approach).

tinygo, jenkinsx and one other package (that I can't remember) were really painful. Most packages (300+ I think) were actually automatically migrated + mostly just worked.

Most overrideModAttrs options are pretty small (and maybe I should go see if I can make them top line options).

I also don't understand why those huge vendor.patch-files are needed, in that case I think that it's even easier to just use a generated deps.nix. To be fair, this seems to be only relevant for documize-community atm where I was able to remove it during the last update[2].

They shouldn't be?

I thought @Mic92 had me drop almost all of these. My memory is that one binary used gomock which didn't build, so I did a patch on the source. When I get some time I'll see if I can go find the vendor patches and see what the big ones are + why they are present.

I don't think anyone has objections to deps.nix, just a lack of time to implement it :)

I'd like to know why those hacks are even needed and discuss about better alternatives (AFAICS modSha256 will be removed in the upcoming release and we now seem to prefer buildGoModule over buildGoPackage).

In general these patches are needed because go mod vendor doesn't work cleanly on upstream :)

[c00w]

I'm not seeing vendor.patch files?

% git show && grep vendor.patch . -r                                                                       ~/nixpkgs
commit 467ce5a9f45aaf96110b41eb863a56866e1c2c3c (HEAD -> master, origin/master, origin/HEAD)
Merge: aabcd40fdc2 5f2eb9ae102
Author: Lassulus <[email protected]>
Date:   Tue Jun 2 20:00:15 2020 +0200

    Merge pull request #88929 from geistesk/hash_extender-20200324
    
    hash_extender: 2017-04-10 -> unstable-2020-03-24

./pkgs/tools/misc/peep/default.nix:  cargoPatches = [ ./0001-Add-Cargo.lock-by-running-cargo-vendor.patch ];
Binary file ./.git/index matches

[Ma27]

Thanks a lot for taking the time to respond to my criticism, I read both of your comments. Please be patient until tomorrow, I'll need to sleep now :)

[c00w]

A lot of packages[1] now have to override dependencies using overrideModAttrs. In my opinion this is a serious regression since we now push the task of solving dependency problems to our package maintainers, previously the build-script was able to do this automatically.

With #89453 we now have 4 instances of overrideModAttrs out of 465 packages, which is pretty good

grep overrideModAttrs . -r                                              ~/nixpkgs
./pkgs/applications/networking/mailreaders/aerc/default.nix:  overrideModAttrs = (_: {
./pkgs/development/go-modules/generic/old.nix:, overrideModAttrs ? (_oldAttrs : {})
./pkgs/development/go-modules/generic/old.nix:  args = removeAttrs args' [ "overrideModAttrs" "modSha256" "disabled" ];
./pkgs/development/go-modules/generic/old.nix:  ) // overrideModAttrs modArgs);
./pkgs/development/go-modules/generic/default.nix:, overrideModAttrs ? (_oldAttrs : {})
./pkgs/development/go-modules/generic/default.nix:  args = removeAttrs args' [ "overrideModAttrs" "vendorSha256" "disabled" ];
./pkgs/development/go-modules/generic/default.nix:  ) // overrideModAttrs modArgs) else "";
./pkgs/development/compilers/tinygo/default.nix:  overrideModAttrs = (_: {
./pkgs/servers/mautrix-whatsapp/default.nix:  overrideModAttrs = _: {
./pkgs/servers/tailscale/default.nix:  overrideModAttrs = (_: {

grep buildGoModule pkgs -r | wc -l                                      ~/nixpkgs
465

[Mic92]

@nlewo also made some progress on getting vgo2nix to work nicely with buildGoModule: nlewo@09ac010#diff-5cc2ec5eab07fa4ee4e3ba57846f5134R53 This means we no longer have evil fixed-input derivations.

[Ma27]

Hi! First of all sorry for the delay, I really intended to respond earlier. I'll try to respond to the statements made during the last week in a more or less chronological order.

It's actually a strength of the new approach that you can just use plain text patches to fix issues :)

Can you elaborate this, please? Can't you patch e.g. the go.mod of a repo for modifications? Or are you explicitly talking about sources within vendor/? (fair point then - didn't know that).

However I really just want to get rid of buildGoPackages as much as possible, so I'm doing this to try and get #86282 merged.

We still have the issue with the fixed-output derivation here (but we discussed about this in the past IIRC and I acknowledge that we have different opinions - also, we should discuss this in the corresponding PR then).

I'm confused - buildGoModule never supported deps.nix.

That's right: I'm talking about convenience: buildGoModule used to be (or is) something which takes care of pulling the proper dependencies which is why people occasionally prefer it over buildGoPackage.

I only observed the need to fix some packages manually in a post-fetch hook after your patch which reminded me of the deps.nix approach where you had to create your own build-structure to generate a deps.nix.

That's the next complaint to fix, either by using deps.nix to generate the vendor folder, or by writing a go module proxy which can be configured via deps.nix.

I see. My posts weren't meant as complaint or something, I mainly want to understand the motivation and technical reasons :)

Most overrideModAttrs options are pretty small (and maybe I should go see if I can make them top line options).

That's true, but it still means that you have to dive into the dependency tree in some cases.

I'm not seeing vendor.patch files?

Yeah, I removed the last one from documize-community a while ago. I thought I've seen it somewhere else as well, but I may be wrong. In that case, we can consider this discussion-point resolved I guess :)

also made some progress on getting vgo2nix to work nicely with buildGoModule: nlewo/nixpkgs@09ac010#diff-5cc2ec5eab07fa4ee4e3ba57846f5134R53 This means we no longer have evil fixed-input derivations.

Oh that's nice!

---

Apart from that, thanks a lot for working so hard on this. As far as I understand this now, with some pending PRs getting merged, we have a fairly stable improvement for buildGoModule that only causes issues for packages with incompatible or problematic project-setups (i.e. approaches that are disregarded by the Go community?)

[c00w]

Can you elaborate this, please? Can't you patch e.g. the go.mod of a repo for modifications? Or are you explicitly talking about sources within vendor/? (fair point then - didn't know that).

No one patched the results of downloading go modules because it was ... hard enough to be functionally impossible without writing code. We also had a gloriously hard time making it deterministic (I think it took 3 passes and an upstream go issue).

People are patching the sources within vendor, which is great :) I should probably do a review of people using this besides me, to see what they're doing.

[stale]

I marked this as stale due to inactivity. → More info

[Mic92]

Can we close this?

[zimbatm]

Yep