go-modules: simplify overriding

[tjni]

This is currently a proof of concept, pending a resolution to the discussion in #119942.

Please feel free though to offer feedback or test this out. I extracted a bunch of logic in default.nix into a separate go-modules.nix, hence the number of lines changed.

Description of changes

The aim of this patch is to enable overriding attributes of Go modules,
such as src and vendorSha256, like:

  myGoModule.overrideAttrs (oldAttrs: {
    src = fetchFromGitHub {
      ...
    };

    vendorSha256 = "...";
  });

which is more idiomatic and what a novice Nix user would expect to work.

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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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/)
• 22.05 Release Notes (or backporting 21.11 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
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[zowoq]

I extracted a bunch of logic in default.nix into a separate go-modules.nix

I'd very strongly prefer that we don't do this, I'd rather keep this as a single file.

[tjni]

I think this will lead to a lot of rebuilds because vendorSha256 was previously left out of the package derivation's attribute set.

[tjni]

I extracted a bunch of logic in default.nix into a separate go-modules.nix

I'd very strongly prefer that we don't do this, I'd rather keep this as a single file.

I only have a slight preference to keeping them separate, so I'll change that since you feel strongly about it.

[zowoq]

Not sure I understand why only some of these are final/finalAttrs but not others?

[tjni]

Not sure I understand why only some of these are final/finalAttrs but not others?

Great question!

If an attribute is derived from another attribute (that could come from a rec, for instance), it should now depend on finalAttrs. If its value comes from somewhere else, such as an argument, it should not.

Some attributes, like vendorSha256, are only used by the go-modules derivation, but need to be set on the package derivation so that they can be overridden. These are then accessed by go-modules under finalAttrs since they now depend on those attributes from the package's derivation.

I hope this makes sense!

[zowoq]

I'll be more specific, why allowGoReference and tags?

[tjni]

The intention behind finalAllowGoReference and finalTags was to use the default values of the arguments when they are not overridden in the expressions that follow. Otherwise, they would be null if referenced via finalAttrs.

[zowoq]

Maybe I'm just too tired and I'm not seeing it but neither are used in go-modules derivation so I'm not really following.

While I realise some people want a way of being able to override this TBH I don't think is a good idea. Rather than introducing more complexity to address the non-default use case maybe we should try to find another approach that would hopefully be simpler even if it is a rewrite of buildGoModule (which I've been thinking about doing anyway).

[tjni]

Maybe I'm just too tired and I'm not seeing it but neither are used in go-modules derivation so I'm not really following.

No worries. finalTags and finalAllowGoReference specifically are not used in go-modules, but they are used when constructing the attributes needed by the mkDerivation call for the package itself. For instance:

GOFLAGS = lib.optionals (!finalProxyVendor) [ "-mod=vendor" ]
      ++ lib.optionals (!finalAllowGoReference) [ "-trimpath" ];

and

... ''${tags:+-tags=${lib.concatStringsSep "," finalTags}} ...

Rather than introducing more complexity to address the non-default use case maybe we should try to find another approach that would hopefully be simpler even if it is a rewrite of buildGoModule

I don't mind that but don't have any ideas. The standard way to override a derivation via an overlay is:

myPackage = prev.myPackage.overrideAttrs(oldAttrs: {
  src = ...;
  ...
});

which breaks completely when myPackage is a Go module and src and vendorSha256 need to be overridden. It's possible to introduce new arguments to buildGoModule new attributes on the derivation that are the override hooks for these, but to me there is a desirable simplicity in overriding these packages through overrideAttrs like all other Nix packages.

[Artturin]

btw
maintainability > rebuilds

[zowoq]

btw maintainability > rebuilds

Don't understand what this means?

[ShamrockLee]

Hi! Are you still working on this!

The current buildGoModule overriding simply doesn't work (unless heavily hacked). I'm looking forward for this PR to get merged!

[Aleksanaa]

This is closed since #225051 containing a similar approach is merged. If you have any optimization on top of it, feel free to reopen and reuse this.