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

proposal: cmd/go: lazy module loading #36460

Open
bcmills opened this issue Jan 8, 2020 · 16 comments
Open

proposal: cmd/go: lazy module loading #36460

bcmills opened this issue Jan 8, 2020 · 16 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 8, 2020

I propose to change cmd/go to avoid loading transitive module dependencies
that have no observable effect on the packages to be built.

The key insights that lead to this approach are:

  1. If no package in a given dependency module is ever (even transitively)
    imported by any package loaded by an invocation of the go command, then an
    incompatibility between any package in that dependency and any other package
    has no observable effect in the resulting program(s). Therefore, we can
    safely ignore the (transitive) requirements of any module that does not
    contribute any package to the build.

  2. We can use the explicit requirements of the main module as a coarse filter
    on the set of modules relevant to the main module and to previous
    invocations of the go command.

Based on those insights, I propose to change the go command to retain more
transitive dependencies in go.mod files and to avoid loading go.mod files
for “irrelevant” modules, while still maintaining high reproducibility for build
and test operations.

Design doc: CL 220080.

@bcmills bcmills added this to the Go1.15 milestone Jan 8, 2020
@gopherbot gopherbot added the Proposal label Jan 8, 2020
@bcmills bcmills closed this Jan 8, 2020
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 3, 2020

Change https://golang.org/cl/217557 mentions this issue: cmd/go: add a regression test for package import cycles guarded by build tags

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 19, 2020

Change https://golang.org/cl/220080 mentions this issue: design/36460: add design for lazy module loading

@bcmills bcmills removed the WaitingForInfo label Feb 19, 2020
@bcmills bcmills reopened this Feb 19, 2020
@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Feb 19, 2020

A detailed design is in CL 220080.

(You can view the rendered Markdown by opening the .md file at the latest patch set and clicking the gitiles link.)

@bcmills bcmills added this to Incoming in Proposals Feb 19, 2020
@ChrisHines

This comment has been minimized.

Copy link
Contributor

@ChrisHines ChrisHines commented Feb 20, 2020

For modules that are tidy:

The module versions recorded in the go.mod file would be exactly those listed in vendor/modules.txt, if present.

👍 I think that is a nice property to have. I have some projects for which go mod vendor is sane, but go mod tidy updates the go.mod file in undesirable ways due to test-of-test dependencies. If I understand this proposal correctly that problem would go away.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Feb 20, 2020

I added a few examples (in our usual cmd/go script test format) to the design doc.

@ChrisHines, the third example explicitly covers the behavior for test-of-test dependencies.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Feb 21, 2020

CC @julieqiu since this may affect how we display dependencies on pkg.go.dev.

CC @heschik, @dmitshur because this may affect the package search algorithms used by goimports and godoc.

CC @bep because I seem to recall that Hugo is doing something unusual with module dependencies independent of Go package imports. (But note that we are unlikely to actively support the use of Go modules for purposes other than compiling Go source code.)

@bep

This comment has been minimized.

Copy link
Contributor

@bep bep commented Feb 21, 2020

But note that we are unlikely to actively support the use of Go modules for purposes other than compiling Go source code.

Our unusual usage is fairly well anchored in the "Go project". That said, I have read your design document twice, and I think this is a good thing (even for our unusual use) -- but it would be easier to determine with a prototype (I find this topic to be a little bit of a mind twister and I don't fully understand the examples in the document).

gopherbot pushed a commit that referenced this issue Feb 21, 2020
…ild tags

I've been thinking about the relationship between the package import
graph and the module import graph, and realized that the package
import graph is not always acyclic. (The package import graph must be
acyclic given a specific set of build tags, but the 'mod' subcommands
intentionally ignore build tags.)

I'm not sure whether we have any existing regression tests that cover
this sort of cycle, so I'm adding one now. Thankfully, it passes!

Updates #36460

Change-Id: I7679320994ee169855241efa51cd45f71315f7f5
Reviewed-on: https://go-review.googlesource.com/c/go/+/217557
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Feb 24, 2020

@bcmills This is excellent, thank you.

I think I follow the detail, but have a couple of questions which might well prove otherwise 😄

Since the all pattern is based on package imports (more-or-less independent of module dependencies), this change should be independent of the go version specified in the go.mod file.

I'm not sure what this sentence means. In the context of changing the definition of the all package pattern, does this not imply the change to the definition is contingent on the go version specified in the go.mod file? Indeed, doesn't the next paragraph go on to say exactly that, in addition to saying the cmd/go version is also relevant?

Again related to the proposed change in the definition of the all package pattern, given the proposed changes to maintenance via go mod tidy of the main module's go.mod file, does this mean that go test all (assuming the previous, i.e. current, definition of all for one second) is no longer reproducible in the case of incomplete go.mod files? Do we need to retain a special pattern for the previous definition of package all? I'm not sure what the main driver was for the original all definition so can't really offer much other than vague questions here.

What about go.mod files that are left intentionally untidy? Are there any situation where this is still required/desirable? If so, how does that influence (or not influence) the proposed (behaviour) changes?

Regarding the transition to use the new behaviour. People will, per your proposal, need to specify go 1.15 in their go.mod files. I'm not currently aware of any side effects of making that change (from an earlier version), but perhaps that would be worth confirming via an FAQ?

Thinking about the compatibility section and CI use, if I've understood things correctly, running go mod tidy with go1.13.x on a go.mod file that was otherwise stable with go1.15.x will result in changes, but running with go1.14.x will not. I guess that's another potential FAQ entry for people who will likely add go1.15beta1 to their build matrix and then start seeing surprising diffs post the go mod tidy and go mod verify steps of their CI build?

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Feb 25, 2020

In the context of changing the definition of the all package pattern, does this not imply the change to the definition is contingent on the go version specified in the go.mod file?

Not really, no: the all package pattern (both before and after this proposal) is a function of the import graph, and only interacts with modules (and go.mod files) to the extent that those modules provide imported packages.

But not that the all module pattern does depend on the version, and for a go 1.15 module, the all module pattern as defined in Go 1.14 does not necessarily cover the all package pattern as defined in Go 1.14.

@bcmills bcmills closed this Feb 25, 2020
@bcmills bcmills reopened this Feb 25, 2020
@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Feb 25, 2020

What about go.mod files that are left intentionally untidy?

Don't do that? 😅

Are there any situation where this is still required/desirable? If so, how does that influence (or not influence) the proposed (behaviour) changes?

I would argue that there weren't any situations where that was ever required (or particularly desirable). But if you have specific examples, I'd like to look into them in more depth.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Feb 26, 2020

People will, per your proposal, need to specify go 1.15 in their go.mod files. I'm not currently aware of any side effects of making that change (from an earlier version), but perhaps that would be worth confirming via an FAQ?

Currently the only other effect on module-mode semantics is that the transition from go 1.13 or lower to go 1.14 or higher also enables automatic use of -mod=vendor when the main module has a vendor subtree. Beyond that, the new language features that come along with the version are all strict additions so far.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Feb 26, 2020

Thinking about the compatibility section and CI use, if I've understood things correctly, running go mod tidy with go1.13.x on a go.mod file that was otherwise stable with go1.15.x will result in changes, but running with go1.14.x will not.

go mod tidy with any older version, including 1.14, will demolish the go 1.15 invariants.

The difference between 1.13 and 1.14 is that 1.14 will not demolish those invariants unless you invoke go mod tidy explicitly.

Note that we have fixed bugs in go mod tidy in nearly every release so far, so I hope that by now folks have realized that go mod tidy-ness is version-dependent, much like go fmt cleanness. (And note that go fmt with an older Go version may choke on new language features, so it, too, is not something you want to run with older releases in CI.)

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Feb 26, 2020

I would argue that there weren't any situations where that was ever required (or particularly desirable). But if you have specific examples, I'd like to look into them in more depth.

This was the comment I had in mind when asking:

https://go-review.googlesource.com/c/tools/+/184018/2#message-7612508883ea3ee1f11a807241422e9b4e138da6

which refers through to #27899. I've not followed that in detail, so just raising for completeness; the point may now be moot.

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Feb 26, 2020

Currently the only other effect on module-mode semantics is that the transition from go 1.13 or lower to go 1.14 or higher also enables automatic use of -mod=vendor when the main module has a vendor subtree.

Very much a tangent, but are these changes documented somewhere centrally? That would be an incredibly useful guide to start and maintain (vs having the details scattered in various parts of the docs).

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Feb 26, 2020

This was the comment I had in mind when asking: …

Ah, now I remember. The untidiness there was “larger than tidy” rather than “smaller than tidy”, and I think we have basically settled on the // +build approach for that use-case.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Feb 26, 2020

are these [go version] changes documented somewhere centrally?

Not yet. That's arguably part of #30791...

@rsc rsc moved this from Incoming to Active in Proposals Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.