Rules_go: Gazelle should recognize testdata directories that contain code

[jayconrod]

golang.org/x/crypto/ssh/testdata is a Go package that contains Go code. Gazelle treats this as a directory containing data files, which breaks tests that depend on it as a normal package.

Gazelle should check whether testdata directories contain Go code. If they do, it should treat them as normal directories. If they don't, it should continue treating them as data.

[jayconrod]

@ains pointed out this issue also affects github.com/golang/protobuf.

[jayconrod]

I have a draft fix for this, but unfortunately, it breaks golang.org/x/tools. This is harder than it looks initially.

x/tools has a lot of programs that operate on .go files, so it's natural that there are .go files in testdata directories that are not buildable. Gazelle doesn't have a good way to distinguish these from source files, and when it attempts to generate BUILD files, it errors out.

500 resolves one of these errors. I'm thinking about making more errors non-fatal. I still need to analyze some of the errors to determine if we can do some things better.

[kevinburkeotto]

I'm still running into this issue.

[jayconrod]

What repo are you seeing this issue in?

[kevinburkeotto]

Yikes, apologies - see the other issue I commented on, I think that's the right place to discuss this and the right problem I was seeing.

[ash2k]

Any known workarounds?

bazel run //:gazelle-fix
INFO: Analysed target //:gazelle-fix (0 packages loaded).
INFO: Found 1 target...
Target //:gazelle-fix up-to-date:
  bazel-bin/gazelle-fix_script.bash
  bazel-bin/gazelle-fix
INFO: Elapsed time: 0.642s, Critical Path: 0.04s
INFO: Build completed successfully, 1 total action

INFO: Running command line: bazel-bin/gazelle-fix
gazelle: /bla/vendor/golang.org/x/tools/cmd/fiximports/testdata/src/old.com/bad/bad.go: error reading go file: /bla/vendor/golang.org/x/tools/cmd/fiximports/testdata/src/old.com/bad/bad.go:2:43: expected 'package', found 'EOF'
gazelle: found packages a (a.go) and b (b.go) in /bla/vendor/golang.org/x/tools/go/gcimporter15/testdata
gazelle: found packages pointer (pointer.go) and complexnums (complexnums.go) in /bla/vendor/golang.org/x/tools/go/internal/gccgoimporter/testdata
gazelle: /bla/vendor/golang.org/x/tools/go/loader/testdata/badpkgdecl.go: error reading go file: /bla/vendor/golang.org/x/tools/go/loader/testdata/badpkgdecl.go:1:34: expected 'package', found 'EOF'
gazelle: found packages c (c_test.go) and a (a_test.go) in /bla/vendor/golang.org/x/tools/go/ssa/interp/testdata

[jayconrod]

@ash2k You can add a comment # gazelle:exclude testdata to the top of the build file in the parent directory. That will block Gazelle from traversing the directory.

I'll close this issue because it seems like this is working correctly now.

[ash2k]

@jayconrod This does not work for me unfortunately - stuff is in vendor folder which I don't check in. And gazelle:exclude does not apply to the subtree.

[jayconrod]

@ash2k So how would you want this to work? There are testdata directories that contain Go code that should be built, and testdata directories that contain fake code for testing that cannot be built. We can't automatically tell which is which, so there needs to be some directive that says what to do.

# gazelle:exclude will prevent the testdata subtree from being traversed. I think it makes the most sense for that directive to be written in the build file where it applies; it could be very confusing if files are ignored due to a directive in some other build file.

[ains]

We see this issue too (they are just warnings, but we do get concerned messages occasionally)

Perhaps a .gazelleignore file would work?

[jayconrod]

@ains What's the advantage of .gazelleignore over # gazelle:exclude testdata?

[ains]

Ah I may have misunderstood what can be done with # gazelle:exclude ideally I don't want to need to a persistent modification to the vendor dir (it's managed by dep and will be wiped on ensure).

However if a # gazelle:exclude can be placed in a root BUILD file referencing a subdir within vendor that works.

[ash2k]

ideally I don't want to need to a persistent modification to the vendor dir (it's managed by dep and will be wiped on ensure).

Same here, dep manages my vendor dir so everything will be wiped on dep ensure.

However if a # gazelle:exclude can be placed in a root BUILD file referencing a subdir within vendor that works.

This will work for me too. Maybe # gazelle:exclude can support relative paths, not just directory/file names? i.e. in the root BUILD.bazel I could say

# gazelle:exclude vendor/golang.org/x/tools/cmd/fiximports/testdata
# gazelle:exclude vendor/golang.org/x/tools/go/gcimporter15/testdata

@jayconrod WDYT?

[jayconrod]

I have some concerns about adding that; not entirely opposed, but unsure. I think it will be confusing when files are ignored in one package due to directives in a parent package. It does make sense because of dep's habit of deleting build files, but what if other customizations are needed?

Maybe it would be better to have a wrapper script which runs dep ensure, runs Gazelle in vendor/, then applies a patch to the build files in vendor? That would be a more general solution.

Re-opening for continuing discussion.

[ash2k]

I think it will be confusing when files are ignored in one package due to directives in a parent package.

This is how # gazelle:proto disable works and it is convenient IMHO - I was able to work around https://github.com/bazelbuild/rules_go/issues/900 by putting this directive into the root build file.

It does make sense because of dep's habit of deleting build files, but what if other customizations are needed?

dep and probably other vendor-managing tools assume they fully control and own the directory. I think it is fair. It's their job to ensure that the contents of the directory exactly match the desired state. Why would they leave any extra files there? This is, of course, in conflict with how Bazel build files are used. For example, to do extra patching I need to workaround https://github.com/kubernetes/kubernetes/issues/37598 I just cp the fixed file into the vendor directory to overwrite the broken one. Not great.

Maybe it would be better to have a wrapper script which runs dep ensure, runs Gazelle in vendor/, then applies a patch to the build files in vendor? That would be a more general solution.

As you can see from the error message above, Gazelle fails because some testdata .go files are not valid Go programs. So it is impossible to patch build files after Gazelle ran - presumably it terminates abruptly in such situations. Right now I rm the offending directories after running dep ensure and before running Gazelle:

rm -rf vendor/golang.org/x/tools/cmd/fiximports/testdata \
    vendor/golang.org/x/tools/go/internal/gccgoimporter/testdata \
    vendor/golang.org/x/tools/go/gcimporter15/testdata \
    vendor/golang.org/x/tools/go/loader/testdata \
    vendor/golang.org/x/tools/go/ssa/interp/testdata

This works for me.

[jayconrod]

All right, I'm convinced. Patching after Gazelle runs is more trouble than I thought. I'll # gazelle:exclude to accept paths in subdirectories.