buildGoModule: Fix overriding with overlay-style stdenv

[ShamrockLee]

Description of changes

The Go package builder buildGoModule is refactored to adopt the overlay-style stdenv.mkDerivation (#119942) and to fix overriding. vendorHash can now be overridden directly using overrideAttrs.

Attribute overriding can now be done with simple overrideAttrs. The overrideModAttrs and mod*Phase attributes are kept as syntax sugar only. This implementation is already a drop-in replacement to the original buildGoModule. (Addressing #86349 (comment))

The go-modules, vendorHash and vendorSha256 attributes are moved out of passthru as they now already exposed as attributes of stdenv.mkDerivation. They can still be accessed as, say, pet.vendorHash, but no longer pet.passthru.vendorHash. The builder now only adds go to passthru.

Fixes #86349

Comparison to #171586:

#171586 makes use of the overlay-style stdenv to fix the vendorHash overriding, but the builder (buildGoModule) still accept attribute set only insted of a fixed-point function.

This PR enables support to fixed-point function input. That is, one can write

buildGoModule (finalAttrs:
{
  # ...
})

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/)
• 23.05 Release Notes (or backporting 22.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
• Fits CONTRIBUTING.md.

[zowoq]

It may not be a breaking change but I think we're to close to branch off for this so I'm a very strongly against including it in 23.05.

Also, I am planning to make a few changes to buildGoModule after the branch, e.g. drop vendorSha256 and only have vendorHash, it'll be easier and make this PR simpler if that happens first.

cc @NixOS/golang

[ShamrockLee]

It may not be a breaking change but I think we're too close to branch off for this so I'm strongly against including it in 23.05.

Agree. Should I drop change to the the release note?

I place them there just because the new release note hasn't come out yet.

[ShamrockLee]

Would it be okay to just remove those mod*Phase attributes? We alredy have overrideModAttrs, which itself will also become a syntax sugar after this chage. Exposing those mod*Phase would be confusing.

[zowoq]

Should I drop change to the the release note?

I place them there just because the new release note hasn't come out yet.

Fine to leave them there for now, can be moved once the new release note is created after branch off.

Would it be okay to just remove those mod*Phase attributes? We alredy have overrideModAttrs, which itself will also become a syntax sugar after this chage. Exposing those mod*Phase would be confusing.

Maybe, have to give it some thought first.

---

Also, I'd appreciate it if you would revert all of the unrelated formatting changes, it makes the diff much harder to review.

[ShamrockLee]

Also, I'd appreciate it if you would revert all of the unrelated formatting changes, it makes the diff much harder to review.

It turns out that the indentation difference is not just caused by nixpkgs-fmt, but the inherent difference of two structures.

Even then, I managed to make them align by making reasonable adjustment to the original expression (mainly by simplifying the go-modules part before running nixpkgs-fmt). The adjustment (packed inside the first 2 commits) doesn't bring any rebuild. Maybe I could file another PR containing them to make this one easier to review.

The last commit contains the actual change. The relation between the original implementation and the new implementation can be seen from the diff. The expected attributes are organized at the beginning of the expression, and the corresponding comment documentation are prefixed with ATTRIBUTES:.

[zowoq]

@ShamrockLee I'm switching this back to draft as it isn't going to be merged until after branch off.

[zowoq]

#225371

Still needs more testing but I've opened a PR to (mostly) get rid of vendorSha256.

[ShamrockLee]

Go packages built with this buildGoModule implementation seems to take noticable longer to evaluate. Not sure if it also happens to #171586.

How can we benchmark the evaluation time?

[ShamrockLee]

(Just to add some documentation and correct the commit message. Sorry for another force-push.)

[ShamrockLee]

Facing the errors below when trying to build pet and md2man

       last 9 log lines:
       > unpacking sources
       > unpacking source archive /nix/store/ncp60ky4x5jiqsw5mbl0lwhvw2xw351a-source
       > source root is source
       > patching sources
       > configuring
       > building
       > Building subPackage ./.
       > malformed import path "-p": leading dash
       > package 16 is not in GOROOT (/nix/store/2r0y9hgc3gma3rfn0r1f7i104g485nk2-go-1.20.3/share/go/src/16)
       For full logs, run 'nix log /nix/store/irb53w655wz443cn8drmdrsi57386jgl-pet-0.4.0.drv'.

It is probably caused by the flags+=("-p" "$NIX_BUILD_CORES") part, which intends to specify the jobs to use to build in parallel, but is somehow interpreted as two import paths when running go install.

The buildPhase implementation

  buildPhase = previousAttrs.buildPhase or (''
      runHook preBuild
  exclude='\(/_\|examples\|Godeps\|testdata'
  if [[ -n "$excludedPackages" ]]; then
    IFS=' ' read -r -a excludedArr <<<$excludedPackages
    printf -v excludedAlternates '%s\\|' "''${excludedArr[@]}"
    excludedAlternates=''${excludedAlternates%\\|} # drop final \| added by printf
    exclude+='\|'"$excludedAlternates"
  fi
  exclude+='\)'

  buildGoDir() {
    local cmd="$1" dir="$2"

    . $TMPDIR/buildFlagsArray

    declare -a flags
    flags+=($buildFlags "''${buildFlagsArray[@]}")
    flags+=(''${tags:+-tags=${lib.concatStringsSep "," finalAttrs.tags}})
    flags+=(''${ldflags:+-ldflags="$ldflags"})
    flags+=("-p" "$NIX_BUILD_CORES")

    if [ "$cmd" = "test" ]; then
      flags+=(-vet=off)
      flags+=($checkFlags)
    fi

    local OUT
    if ! OUT="$(go $cmd "''${flags[@]}" $dir 2>&1)"; then
      if ! echo "$OUT" | grep -qE '(no( buildable| non-test)?|build constraints exclude all) Go (source )?files'; then
        echo "$OUT" >&2
        return 1
      fi
    fi
    if [ -n "$OUT" ]; then
      echo "$OUT" >&2
    fi
    return 0
  }

  getGoDirs() {
    local type;
    type="$1"
    if [ -n "$subPackages" ]; then
      echo "$subPackages" | sed "s,\(^\| \),\1./,g"
    else
      find . -type f -name \*$type.go -exec dirname {} \; | grep -v "/vendor/" | sort --unique | grep -v "$exclude"
    fi
  }

  if (( "''${NIX_DEBUG:-0}" >= 1 )); then
    buildFlagsArray+=(-x)
  fi

  if [ ''${#buildFlagsArray[@]} -ne 0 ]; then
    declare -p buildFlagsArray > $TMPDIR/buildFlagsArray
  else
    touch $TMPDIR/buildFlagsArray
  fi
  if [ -z "$enableParallelBuilding" ]; then
      export NIX_BUILD_CORES=1
  fi
  for pkg in $(getGoDirs ""); do
    echo "Building subPackage $pkg"
    buildGoDir install "$pkg"
  done
'' + lib.optionalString (stdenv.hostPlatform != stdenv.buildPlatform) ''
  # normalize cross-compiled builds w.r.t. native builds
  (
    dir=$GOPATH/bin/${go.GOOS}_${go.GOARCH}
    if [[ -n "$(shopt -s nullglob; echo $dir/*)" ]]; then
      mv $dir/* $dir/..
    fi
    if [[ -d $dir ]]; then
      rmdir $dir
    fi
  )
'' + ''
  runHook postBuild
'');

  

I'm still building the previous commit on staging before this patch. (It takes a long time to build packages on the staging branch on my laptop.) If the build also fails with similar error message, maybe the version of `go` on staging no longer support this flag.

[ShamrockLee]

It turns out that the dummy value of buildFlags and buildFlagsArrary should be null instead of "".

It doesn't cause problem in the original implementation because it is never passed as an argument to stdenv.mkDerivation -- it is the default value of the function argument, and it won't be inside args' in most cases since users will remove them upon seeing the warning. In this implementation, however, I need to choose a dummy value and pass it so that finalAttrs.buildFlags can be used to warn users when they intended to use them, no matter if it appears inside the initial attribute set or the overrideAttrs attribute set. Passing it as null prevents it from becoming a variable in the build commands.

[ShamrockLee]

I found a way to preserve the input arguments. This should make reviewing easier.

I manually outdent the input argument set to make the diff cleaner, and then format it back in the following commit.

[ShamrockLee]

(Sorry for pressing the wrong button).

[zowoq]

I've moved remove input argument "tags" to a separate PR we can merge first: #234537

[flokli]

What's the status of this? Could this be rebased?

[zowoq]

What's the status of this? Could this be rebased?

My understanding is that this PR as it is currently is superseded by a generic implementation in #234651 and that once that PR is merged this PR will be changed to use the generic implementation.