Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: ignore 'go.mod' files in 'testdata' subdirectories when computing module boundaries #27852

Open
bcmills opened this issue Sep 25, 2018 · 12 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 25, 2018

Some existing tests of Go tools make use of testdata directories containing a self-contained GOPATH

For those tests to work as expected in module mode, the testdata directories may need to contain go.mod files, but in order for the tests to pass when run from within other modules, those go.mod files should be included in the same module as the test — they should not actually define separate modules.

(There's just one caveat that I know of: at the moment, adding a go.mod file is the easiest way to prune out files whose paths are invalid in the module zip format (see #26672).)

CC: @rsc @myitcv @thepudds

¹ e.g., x/tools/cmd/fiximports/testdata

@bcmills bcmills added this to the Go1.12 milestone Sep 25, 2018
@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Sep 25, 2018

but in order for the tests to pass when run from within other modules, those go.mod files should be included in the same module as the test — they should not actually define separate modules.

This bit I don't quite understand, specifically "should be included in the same module as the test". Could you clarify?

That said, with tests that do require a "full setup" (i.e. a go.mod etc) is it not just as easy to create a temporary directory and programmatically create a go.mod within it? Seems a bit easier/cleaner than offering an exemption to the rule?

Or just use _testdata? (I realise this might be contrary to suggested best practice, but again maybe easier to offer a variation on this best practice with the introduction of modules?)

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Sep 25, 2018

This bit I don't quite understand, specifically "should be included in the same module as the test". Could you clarify?

If I add a require golang.org/x/tools to my go.mod file and then run go test golang.org/x/tools/cmd/fiximports, the test should pass. That implies that the source directory within the module cache must include the entire testdata directory.

That said, with tests that do require a "full setup" (i.e. a go.mod etc) is it not just as easy to create a temporary directory and programmatically create a go.mod within it?

That is a straightforward workaround, but it splits the test input across two places (the testdata directory and the code to generate the go.mod files), and makes it so that you can't just cd testdata and export GOPATH=$(pwd) to investigate the (accurate) contents of the test directory.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 26, 2018

Change https://golang.org/cl/137815 mentions this issue: all: set GO111MODULE=off for tests that use GOPATHs in testdata.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Sep 27, 2018

Note from @rsc: if we do this, we should also reject module paths that include a testdata component.

gopherbot pushed a commit to golang/tools that referenced this issue Oct 3, 2018
Some users may set GO111MODULE=on, and we will eventually want to be able to
build x/tools itself in module mode.

Updates golang/go#27858
Updates golang/go#27852

Change-Id: Iaf488b2a89e6526471530245cb580f1f0391a770
Reviewed-on: https://go-review.googlesource.com/137815
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
@bcmills bcmills modified the milestones: Go1.12, Go1.13 Nov 14, 2018
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 20, 2018

I don't know that we should introduce a special case here. We have a very simple rule at the moment. Maybe we should figure out how to work within that rule.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Feb 22, 2019

Maybe we should figure out how to work within that rule.

The stack of changes starting at https://golang.org/cl/163209 works within that rule, but it's tedious (because we have to explicitly copy the modules in each test) and doesn't interoperate well with IDEs (because the import paths used by the files in testdata don't match the import paths that will be implied by the injected go.mod file).

Plus, the tests end up a lot less efficient because we have to copy the entire contents of the directory: os.Symlink is not portable to plan9, and module testdata directories are often read-only (see also #28387).

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Feb 22, 2019

Plus, the tests end up a lot less efficient

I spoke too soon: it's easy enough to fall back to copying if os.Symlink is not supported. So we end up needing to copy the branching structure of the directory tree and make some symlinks, but not copy individual files.

@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Feb 23, 2019

I don't know that we should introduce a special case here.

cmd/go documentation says the following about "testdata":

The go tool will ignore a directory named "testdata", making it available to hold ancillary data needed by the tests.

Directory and file names that begin with "." or "_" are ignored by the go tool, as are directories named "testdata".

If creating a go.mod file inside a "testdata" directory defines a new module and removes files from the original module, isn't that a case of the go tool not ignoring the "testdata" directory fully?

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Feb 23, 2019

I just realized that #30240 may require a variation on this proposal: if we want module-enabled builds from within the module cache to work with vendor trees, then we will need to be careful not to truncate the vendor tree when downloading the associated modules.

On the other hand, if we are ok with entries in the module cache only working for builds outside the module (which may be the case anyway due to file-based replace directives), then we should also treat the vendor root itself as a module boundary (and prune it out during module extraction).

Both of those changes would require a migration strategy in order not to break existing go.sum entries. I have an idea for that and will post a proposal.

@jayconrod

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Feb 25, 2019

@dmitshur That documentation isn't telling the whole story. go build and go list ignore those directories when expanding ... wildcards. You can still build packages with those names explicitly, and you can import them from other packages. And people do build test packages (and especially examples) that use these names. testdata isn't technically that much of a special case right now.

@jayconrod

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Feb 25, 2019

I think I agree with @rsc's earlier comment. It would be better to keep the rules simple and not add a special case.

Adding a special case means more logic in cmd/go. Anything that processes module paths also needs to know about this rule (proxy implementations, goimports, ...). It's weird that testdata is valid as part of a package path but not a module path.

This may make tests for tools a little more complicated, but there are tools like golang.org/x/tools/go/packages/packagestest that should make it easier.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 18, 2019

Change https://golang.org/cl/196297 mentions this issue: cmd/go/internal/modload: do not prune the module root when walking directories

gopherbot pushed a commit that referenced this issue Sep 18, 2019
…rectories

When walking filesystem paths to locate packages, we normally prune
out subdirectories with names beginning with ".", "_", or equal to
"testdata". However, we should not prune out such a directory if it is
at or above the module root, since its name is not part of the package
path.

Fixes #28481
Updates #27852

Change-Id: Ice82b1f908afaab50f5592f6c38ca6a0fe911edf
Reviewed-on: https://go-review.googlesource.com/c/go/+/196297
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.