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/internal/modload: refactor global state into exported struct type #40775

Open
jayconrod opened this issue Aug 13, 2020 · 31 comments
Open

cmd/go/internal/modload: refactor global state into exported struct type #40775

jayconrod opened this issue Aug 13, 2020 · 31 comments

Comments

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Aug 13, 2020

The modload package has a lot of global state. In general, variables that are set once during initialization and global caches are fine. But buildList and loader in particular cause problems, since most exported functions either read values out of them or replace and overwrite them. It's frequently not obvious when this is happening.

This leads to some unfortunate situations. For example, see get.go in CL 228383. If modload.ListModules is called before modload.WriteGoMod, the go.mod file will contain spurious // indirect comments. This happens because WriteGoMod uses loaded.direct to decide whether to mark a requirement as indirect, but ListModules calls LoadBuildList, which replaces loaded, so it appears that nothing is directly required.

I think these side effects should be more obvious to callers, and the loaded and buildList variables should be removed. Instead, the loader struct type should be exported, and exported functions that create, read, or write it should be converted to methods.

@jayconrod
Copy link
Contributor Author

@jayconrod jayconrod commented Aug 14, 2020

Another problem to add: PackageModuleInfo returns a new *modinfo.ModulePublic on every call. It would be nice for packages from the same module to share the same instance. This would require adding state to loaded, which I'd rather not do until this is improved.

Loading

@jayconrod
Copy link
Contributor Author

@jayconrod jayconrod commented Aug 19, 2020

Another problem: WriteGoMod is highly sensitive to global state in modload. We should be careful not to call WriteGoMod more than once, since calling it multiple times can result in cosmetic differences in go.mod files, based on which changes were made when.

CL 237017 was a step in this direction, but we should confirm other commands like go get only call WriteGoMod once at the end.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 19, 2020

Change https://golang.org/cl/249398 mentions this issue: cmd/gorelease: fix brittle test broken by CL 237017

Loading

gopherbot pushed a commit to golang/exp that referenced this issue Aug 19, 2020
CL 237017 caused 'go list' to call modload.WriteGoMod once instead of
twice, which changed the order of requirements in a temporary go.mod
file created by gorelease. Previously, WriteGoMod was called once after
the go version was added, then again after packages were loaded. The
first call did not sort requirements, since packages hadn't been
loaded. The second call didn't write go.mod since no requirements were
missing and go.mod was not considered dirty.

This change makes it so gorelease will only report a diagnostic when
requirements are actually missing. the order of requirements no longer
matters.

Updates golang/go#40775

Change-Id: I5b42104207fbd88dd849b68fb2b5d1ab50ea48c7
Reviewed-on: https://go-review.googlesource.com/c/exp/+/249398
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@bcmills
Copy link
Member

@bcmills bcmills commented Aug 20, 2020

Unfortunately, the buildList and loader variables basically need to be global as long as GOPATH mode continues to exist: they carry implicit state that would not be practical to thread through the GOPATH-mode loader. (The coupling between the GOPATH load package and the module-mode modload package occurs through callback functions, not explicitly-plumbed variables. Which is maybe fine, because in practice we ~never want two distinct build lists or package graphs.)

In the meantime, it should be possible to address the other symptoms you report without eliminating the globals. (For example, LoadBuildList should probably preserve, rather than discard, the existing direct markings — but I think that particular function will go away with lazy loading anyway.)

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 18, 2020

Change https://golang.org/cl/255938 mentions this issue: cmd/go/internal/modget: consolidate Load entrypoints

Loading

gopherbot pushed a commit that referenced this issue Sep 22, 2020
This change replaces ImportPaths, ImportPathsQuiet, LoadALL, and
LoadVendor with a single LoadPackages function, with a LoadOpts struct
that more clearly documents the variations in behavior.

It also eliminates the cmd/go/internal/load.ImportPaths function,
which was undocumented and had only one call site (within its own
package).

The modload.LoadTests global variable is subsumed by a field in the
new LoadOpts struct, and is no longer needed for callers that invoke
LoadPackages directly. It has been (temporarily) replaced with a
similar global variable, load.ModResolveTests, which can itself be
converted to an explicit, local argument.

For #37438
For #36460
Updates #40775
Fixes #26977

Change-Id: I4fb6086c01b04de829d98875db19cf0118d40f8c
Reviewed-on: https://go-review.googlesource.com/c/go/+/255938
Trust: Bryan C. Mills <bcmills@google.com>
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 12, 2020

Change https://golang.org/cl/261640 mentions this issue: cmd/go/internal/modload: avoid using the global build list in QueryPattern

Loading

gopherbot pushed a commit that referenced this issue Oct 16, 2020
…ttern

The Query function allows the caller to specify the current version of
the requested module, but the QueryPattern function is missing that
parameter: instead, it always assumes that the current version is the
one selected from the global build list.

This change removes that assumption, instead adding a callback
function to determine the current version. (The callback is currently
invoked once per candidate module, regardless of whether that module
exists, but in a future change we can refactor it to invoke the
callback only when needed.)

For #36460
For #40775

Change-Id: I001a4a8ab24f5b4fcc66a670d9bd305b47e948ba
Reviewed-on: https://go-review.googlesource.com/c/go/+/261640
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 20, 2020

Change https://golang.org/cl/272129 mentions this issue: cmd/go/internal/modload: remove the Reqs function

Loading

gopherbot pushed a commit that referenced this issue Nov 21, 2020
The Reqs function returns an mvs.Reqs implemention for the global
build list. The API that it presents assumes that the build list is
globally consistent (problematic for #40775) and readily available
(problematic for #36460).

Fortunately, it is no longer used outside of the modload package.
We can instead use individual instances of the unexported mvsReqs
struct, making the dependency on the global build list more explicit.

For #36460
For #40775

Change-Id: I8674442f2a86416b0bf9c3395cb591c1e724c9d2
Reviewed-on: https://go-review.googlesource.com/c/go/+/272129
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 18, 2021

Change https://golang.org/cl/293689 mentions this issue: cmd/go/internal/modload: replace the global buildList with structured requirements

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 23, 2021

Change https://golang.org/cl/303869 mentions this issue: cmd/go: attribute direct imports from indirect dependencies to the importing package

Loading

gopherbot pushed a commit that referenced this issue Mar 25, 2021
… requirements

This is intended to be a pure-refactoring change, with very little
observable change in the behavior of the 'go' command. A few error
messages have prefixes changed (by virtue of being attached to
packages or modules instead of the build list overall), and
'go list -m' (without arguments) no longer loads the complete module
graph in order to provide the name of the (local) main module.

The previous modload.buildList variable contained a flattened build
list, from which the go.mod file was reconstructed using various
heuristics and metadata cobbled together from the original go.mod
file, the package loader (which was occasionally constructed without
actually loading packages, for the sole purpose of populating
otherwise-unrelated metadata!), and the updated build list.

This change replaces that variable with a new package-level variable,
named "requirements". The new variable is structured to match the
structure of the go.mod file: it explicitly specifies the roots of the
module graph, from which the complete module graph and complete build
list can be reconstructed (and cached) on demand. Similarly, the
"direct" markings on the go.mod requirements are now stored alongside
the requirements themselves, rather than side-channeled through the
loader.

The requirements are now plumbed explicitly though the modload
package, with accesses to the package-level variable occurring only
within top-level exported functions. The structured requirements are
logically immutable, so a new copy of the requirements is constructed
whenever the requirements are changed, substantially reducing implicit
communication-by-sharing in the package.

For #36460
Updates #40775

Change-Id: I97bb0381708f9d3e42af385b5c88a7038e1f0556
Reviewed-on: https://go-review.googlesource.com/c/go/+/293689
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
gopherbot pushed a commit that referenced this issue Mar 25, 2021
…porting package

For #36460
Updates #40775

Change-Id: I833ee8ee733151aafcff508bf91d0e6e37c50032
Reviewed-on: https://go-review.googlesource.com/c/go/+/303869
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 14, 2021

Change https://golang.org/cl/310089 mentions this issue: cmd/go/internal/modload: fix truncated error message from goModDirtyError

Loading

gopherbot pushed a commit that referenced this issue Apr 14, 2021
…rror

The 'go mod tidy' hint was truncated due to a typo in CL 293689,
and that particular case was not covered by any existing test.

Updates #36460
Updates #40775

Change-Id: Ib6fa872a9dfdafc4e9a112e8add2ff5aecd2dbd0
Reviewed-on: https://go-review.googlesource.com/c/go/+/310089
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 14, 2021

Change https://golang.org/cl/310181 mentions this issue: cmd/go: make Tidy an option in PackageOpts rather than a separate call

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 15, 2021

Change https://golang.org/cl/310669 mentions this issue: cmd/go/internal/load: convert two global flags to an options struct

Loading

gopherbot pushed a commit that referenced this issue Apr 16, 2021
PackageOpts is a new struct type accepted by package loading
functions. It initially has two fields: IgnoreImports, and
ModResolveTests. Previously, these were global variables set by
clients. We'll add more to this in the future.

For #40775

Change-Id: I6956e56502de836d3815ce788bdf16fc5f3e5338
Reviewed-on: https://go-review.googlesource.com/c/go/+/310669
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Apr 21, 2021
This eliminates some awkwardly-stateful outside calls to
modload.{Disallow,Allow,}WriteGoMod.

Perhaps more importantly, it gives the loader the opportunity to
reload packages and revise dependencies after the tidied requirements
are computed. With lazy loading, dropping an irrelevant requirement
from the main module's go.mod file may (rarely) cause other test
dependencies for packages outside the main module to become
unresolved, which may require the loader to re-resolve those
dependencies, which may in turn add new roots and increase the
selected versions of modules providing other packages.

This refactoring allows the loader to iterate between tidying the
build list and reloading packages as needed, making the exact
sequencing of loading and tidying an implementation detail of the
modload package.

For #36460
For #40775

Change-Id: Ib6da3672f32153d5bd7d653d85e3672ab96cbe36
Reviewed-on: https://go-review.googlesource.com/c/go/+/310181
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented May 24, 2021

Change https://golang.org/cl/322369 mentions this issue: cmd/go: don't let 'go mod download' save sums for inconsistent requirements

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 2, 2021

Change https://golang.org/cl/332571 mentions this issue: cmd/go/internal/modload: remove ImportMap and PackageDir

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 20, 2021

Change https://golang.org/cl/336151 mentions this issue: [dev.cmdgo] cmd/go: make fewer 'go mod' commands update go.mod

Loading

gopherbot pushed a commit that referenced this issue Jul 27, 2021
'go mod graph', 'go mod vendor', 'go mod verify', and 'go mod why'
will no longer edit go.mod or go.sum.

'go mod graph', 'go mod verify', and 'go mod why' may still fetch
files and look up packages as if they were able to update
go.mod. They're useful for debugging and should still work when go.mod
is a little broken.

This is implemented in modload.setDefaultBuildMod based on command
name for now. Super gross. Sorry. This should be fixed with a larger
refactoring for #40775.

Fixes #45551

Change-Id: If5f225937180d32e9a5dd252c78d988042bbdedf
Reviewed-on: https://go-review.googlesource.com/c/go/+/336151
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 4, 2021

Change https://golang.org/cl/339895 mentions this issue: [dev.cmdgo] cmd/go/internal/modload: define empty Opts and State types

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 12, 2021

Change https://golang.org/cl/341933 mentions this issue: cmd/go: make fewer 'go mod' commands update go.mod

Loading

gopherbot pushed a commit that referenced this issue Aug 13, 2021
'go mod graph', 'go mod vendor', 'go mod verify', and 'go mod why'
will no longer edit go.mod or go.sum.

'go mod graph', 'go mod verify', and 'go mod why' may still fetch
files and look up packages as if they were able to update
go.mod. They're useful for debugging and should still work when go.mod
is a little broken.

This is implemented in modload.setDefaultBuildMod based on command
name for now. Super gross. Sorry. This should be fixed with a larger
refactoring for #40775.

Fixes #45551

Change-Id: If5f225937180d32e9a5dd252c78d988042bbdedf
Reviewed-on: https://go-review.googlesource.com/c/go/+/336151
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/341933
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 25, 2021

Change https://golang.org/cl/345172 mentions this issue: [dev.cmdgo] cmd/go: remove cfg.ModulesEnabled

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 25, 2021

Change https://golang.org/cl/345169 mentions this issue: [dev.cmdgo] cmd/go/internal/modload: move ForceUseModules to Opts

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 25, 2021

Change https://golang.org/cl/345170 mentions this issue: [dev.cmdgo] cmd/go/internal/modload: move RootMode into Opts

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 25, 2021

Change https://golang.org/cl/345171 mentions this issue: [dev.cmdgo] cmd/go/internal/modload: move BuildMod into State

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 25, 2021

Change https://golang.org/cl/345173 mentions this issue: [dev.cmdgo] cmd/go: move modload.BinDir to internal/load.defaultBinDir

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 25, 2021

Change https://golang.org/cl/345169 mentions this issue: [dev.cmdgo] cmd/go/internal/modload: move ForceUseModules to Opts

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 1, 2021

Change https://golang.org/cl/347155 mentions this issue: cmd/go/internal/modload: move RootMode into Opts

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 1, 2021

Change https://golang.org/cl/347153 mentions this issue: cmd/go/internal/modload: define empty Opts and State types

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 1, 2021

Change https://golang.org/cl/347157 mentions this issue: cmd/go: remove cfg.ModulesEnabled

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 1, 2021

Change https://golang.org/cl/347156 mentions this issue: cmd/go/internal/modload: move BuildMod into State

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 1, 2021

Change https://golang.org/cl/347154 mentions this issue: cmd/go/internal/modload: move ForceUseModules to Opts

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 1, 2021

Change https://golang.org/cl/347158 mentions this issue: cmd/go: move modload.BinDir to internal/load.defaultBinDir

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 2, 2021

Change https://golang.org/cl/347230 mentions this issue: cmd/go/internal/modload: move workspace initialization into Init

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 22, 2021

Change https://golang.org/cl/351468 mentions this issue: cmd/go: test that graph, verify, and why don't write go.mod or go.sum

Loading

gopherbot pushed a commit that referenced this issue Sep 24, 2021
They should also not report an error if these files need to be
updated. These commands are used for debugging, so it's important that
they still work when go.mod and go.sum are incomplete.

For #40775

Change-Id: I1b731599e5a4510f47827b9812525636a7402bf4
Reviewed-on: https://go-review.googlesource.com/c/go/+/351468
Trust: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants