Converting this to a draft/wip so it isn't prematurely merged, I'll review in the next day or two.

A few comments:

• Needs to go to staging and not master due to the number of rebuilds
• These changes should also be applied to buildGoPackage as well to keep them in sync.
• Probably wait to merge this after 22.05 branch off so any problems with it don't interfere with the release or end up needing to be backported, etc.

cc @mmlb You may be interested in this?

• Needs to go to staging and not master due to the number of rebuilds

Done.

• These changes should also be applied to buildGoPackage as well to keep them in sync.

Doh! Done.

What's the actual (not just a hypothetical) use case for buildGo* to need checkFlagsArray? Really don't want to add it without very compelling reasons.

Adding it to just match stdenv isn't necessary, buildGo* is missing makeFlags* and others.

buildFlags* was problematic and mostly replaced by ldflags/tags, now it is kind of disabled and can only be used if set via preBuild, etc.

To make this easier to test I'd prefer to narrow it to just checkFlags in this PR, can you drop checkFlagsArray and return 0 please?

I'm happy do to return 0 myself in another PR and checkFlagsArray can be submitted in another PR if we really want to add it.

What's the actual (not just a hypothetical) use case for buildGo* to need checkFlagsArray? Really don't want to add it without very compelling reasons.

Identical to the reason makeFlagsArray. Shell makes it unbearably hard (mundane escaping) or impossible to pass parameters that include space, square brackets, etc.

I hit this while I was trying to skip some tests that triggered a local networking issue with OfBorg's Linux runners in another PR. The arguments I needed to pass were -run 'Test([^(Post)|^(NewDefaultTransport)])'. With checkFlagsArray it's possible, with just checkFlags the regular expression gets swallowed.

To make this easier to test I'd prefer to narrow it to just checkFlags in this PR, can you drop checkFlagsArray and return 0 please?

Done and updated the PR description too.

I'm happy do to return 0 myself in another PR and checkFlagsArray can be submitted in another PR if we really want to add it.

Understood. I'll send a return 0 PR later.

@grahamc Why was this merged?

Like seriously, why?

• Hadn't been reviewed/tested.

• labeled: work in progress

• this comment:

Probably wait to merge this after 22.05 branch off so any problems with it don't interfere with the release or end up needing to be backported, etc.

I don't know why I'm bothering being the maintainer/codeowner if someone else is just going to come along and hit the merge button without even asking first ...

By my read it was a good improvement to the readability and made the code more understandable. It also wasn't very invasive and looked like it was of low likelihood to cause real problems. Furthermore, it is from a contributor I think fairly highly of. If there are problems with it, we can go ahead and revert.

So why do we have maintainers and codeowners if you're going to do a drive by merge? I absolutely don't understand why you couldn't at least have asked first?

And yes, I do believe there will be some packages that have problems as a result of this.

Reverted and explained in e11568d

We got an eval diff that contains only revert of this PR, so that should be a good reference for what broke: https://hydra.nixos.org/eval/1762195#tabs-now-succeed

Thanks @vcunat for the revert, sorry for the issues. Nice that there is such a clear diff to look at!

@vcunat thanks for the revert, fixing build failures.

When we look at the build output though, we see what is currently in master does not actually run the tests in subdirectories. Builds succeed because, well, we don't run a good chunk of tests for most Go projects.

example: kompose

newly succeeding kompose build's log shows it's actually not running the failing tests.

running tests
?       github.com/kubernetes/kompose   [no test files]
?       github.com/kubernetes/kompose   [no test files]
?       github.com/kubernetes/kompose   [no test files]
?       github.com/kubernetes/kompose   [no test files]
?       github.com/kubernetes/kompose   [no test files]
?       github.com/kubernetes/kompose   [no test files]

Change in #171177 fixes running the tests in subdirectories.
Output from the failing kompose build shows:

running tests
=== RUN   TestParseHealthCheck
    compose_test.go:116: Test case: TCPSocket
    compose_test.go:116: Test case: Exec
    compose_test.go:116: Test case: HTTPGet
--- PASS: TestParseHealthCheck (0.00s)
[... cut ...]
FAIL
FAIL    github.com/kubernetes/kompose/pkg/transformer/openshift 0.016s
FAIL

example: delve

from newly succeeding delve build log:

running tests
no Go files in /build/source

from failing build with this change:

running tests
=== RUN   TestBuild
    dlv_test.go:93: makefile error: exit status 1
        output 2022/05/16 16:57:07 error getting build SHA via git: exec: "git": executable file not found in $PATH
        exit status 1
[... cut ...]
=== RUN   TestVersion
    dlv_test.go:226: go build -o /build/TestDlv1279996027/dlv.exe github.com/go-delve/delve/cmd/dlv: exit status 2
        # runtime/cgo
        In file included from /nix/store/hd3j5xfvffs4vaa74l1har94q10fpnm5-glibc-2.34-210-dev/include/bits/libc-header-start.h:33,
                         from /nix/store/hd3j5xfvffs4vaa74l1har94q10fpnm5-glibc-2.34-210-dev/include/stdlib.h:25,
                         from _cgo_export.c:3:
        /nix/store/hd3j5xfvffs4vaa74l1har94q10fpnm5-glibc-2.34-210-dev/include/features.h:412:4: error: #warning _FORTIFY_SOURCE requires compiling with optimization (-O) [-Werror=cpp]
          412 | #  warning _FORTIFY_SOURCE requires compiling with optimization (-O)
              |    ^~~~~~~
        cc1: all warnings being treated as errors
--- FAIL: TestVersion (0.48s)
=== RUN   TestStaticcheck
    dlv_test.go:1127: staticcheck not installed
--- SKIP: TestStaticcheck (0.00s)
FAIL
FAIL    github.com/go-delve/delve/cmd/dlv       38.581s
FAIL

So how should I proceed?
Maybe it's fair to say "we're in ZHF. it's not time to fix this" and move on.
Will that mean this will get merged after 22.05 is cut?

Surely we have a bunch of Go based packages that simply cannot pass unit tests. So another thing I can do is to go over the failing packages and disable their tests with a TODO for me and their maintainers.

I believe it'd be good to ship a correct goBuildModule and goBuildPackage with 22.05.

I'm relatively new to NixOS, and have little-to-no understanding of its release engineering practices. I'd love to hear your thoughts.

I think only a handful of Go based packages were flying under the radar before this change. You can check the build logs and search for "running tests" and see how no unit test under subdirectories are run because the same tests from top level are run again for each sub-directory

• kompose
• delve
• gitbatch
• go-mtpfs
• gucci (runs the top level tests twice)
• mmake
• skeema (runs the top level tests 8 times)
• wails

It would be ideal to fix most of these packages in the same pull request. Ping respective meta.maintainers (if any) for help.