From 3b523caf4145c2d915c5ead69440f9b890634587 Mon Sep 17 00:00:00 2001 From: Michael Matloob Date: Mon, 23 Aug 2021 14:51:39 -0400 Subject: [PATCH] [dev.cmdgo] cmd/go: clean up TODOWorkspaces instances Address some of the easier todos to address and remove the todos that have already been done and redundant todos. For #45713 Change-Id: I3fe4393168b10c6e005325258d9701713c92e9e4 Reviewed-on: https://go-review.googlesource.com/c/go/+/344491 Trust: Michael Matloob Run-TryBot: Michael Matloob TryBot-Result: Go Bot Reviewed-by: Jay Conrod --- src/cmd/go/internal/modcmd/download.go | 2 +- src/cmd/go/internal/modcmd/initwork.go | 4 +- src/cmd/go/internal/modget/get.go | 20 ++++----- src/cmd/go/internal/modload/build.go | 1 - src/cmd/go/internal/modload/buildlist.go | 8 ++-- src/cmd/go/internal/modload/init.go | 48 +++++++++------------ src/cmd/go/internal/modload/load.go | 3 +- src/cmd/go/internal/modload/modfile.go | 2 +- src/cmd/go/internal/modload/query.go | 53 ++++++++++++++---------- 9 files changed, 71 insertions(+), 70 deletions(-) diff --git a/src/cmd/go/internal/modcmd/download.go b/src/cmd/go/internal/modcmd/download.go index 6a99cb01e1e..ff56d05116f 100644 --- a/src/cmd/go/internal/modcmd/download.go +++ b/src/cmd/go/internal/modcmd/download.go @@ -97,7 +97,7 @@ func runDownload(ctx context.Context, cmd *base.Command, args []string) { modload.LoadModFile(ctx) // to fill MainModules if len(modload.MainModules.Versions()) != 1 { - panic(modload.TODOWorkspaces("TODO: multiple main modules not supported in Download")) + panic(modload.TODOWorkspaces("Support workspace mode in go mod download")) } mainModule := modload.MainModules.Versions()[0] diff --git a/src/cmd/go/internal/modcmd/initwork.go b/src/cmd/go/internal/modcmd/initwork.go index 30653503bcd..4182aa071d7 100644 --- a/src/cmd/go/internal/modcmd/initwork.go +++ b/src/cmd/go/internal/modcmd/initwork.go @@ -13,9 +13,9 @@ import ( "path/filepath" ) -var _ = modload.TODOWorkspaces("Add more documentation below.T hough this is" + +var _ = modload.TODOWorkspaces("Add more documentation below. Though this is" + "enough for those trying workspaces out, there should be more through" + - "documentation if the proposal is accepted.") + "documentation if the proposal is accepted and released.") var cmdInitwork = &base.Command{ UsageLine: "go mod initwork [moddirs]", diff --git a/src/cmd/go/internal/modget/get.go b/src/cmd/go/internal/modget/get.go index 3d831a14d80..37912ce8334 100644 --- a/src/cmd/go/internal/modget/get.go +++ b/src/cmd/go/internal/modget/get.go @@ -690,7 +690,7 @@ func (r *resolver) queryNone(ctx context.Context, q *query) { // However, neither of those behaviors would be consistent with the // plain meaning of the query. To try to reduce confusion, reject the // query explicitly. - return errSet(&modload.QueryMatchesMainModuleError{MainModule: v, Pattern: q.pattern, Query: q.version}) + return errSet(&modload.QueryMatchesMainModulesError{MainModules: []module.Version{v}, Pattern: q.pattern, Query: q.version}) } return pathSet{mod: module.Version{Path: q.pattern, Version: "none"}} @@ -703,7 +703,7 @@ func (r *resolver) queryNone(ctx context.Context, q *query) { } q.pathOnce(curM.Path, func() pathSet { if modload.HasModRoot() && curM.Version == "" && modload.MainModules.Contains(curM.Path) { - return errSet(&modload.QueryMatchesMainModuleError{MainModule: curM, Pattern: q.pattern, Query: q.version}) + return errSet(&modload.QueryMatchesMainModulesError{MainModules: []module.Version{curM}, Pattern: q.pattern, Query: q.version}) } return pathSet{mod: module.Version{Path: curM.Path, Version: "none"}} }) @@ -805,10 +805,10 @@ func (r *resolver) queryWildcard(ctx context.Context, q *query) { if modload.MainModules.Contains(curM.Path) && !versionOkForMainModule(q.version) { if q.matchesPath(curM.Path) { - return errSet(&modload.QueryMatchesMainModuleError{ - MainModule: curM, - Pattern: q.pattern, - Query: q.version, + return errSet(&modload.QueryMatchesMainModulesError{ + MainModules: []module.Version{curM}, + Pattern: q.pattern, + Query: q.version, }) } @@ -1760,10 +1760,10 @@ func (r *resolver) resolve(q *query, m module.Version) { } if modload.MainModules.Contains(m.Path) && m.Version != "" { - reportError(q, &modload.QueryMatchesMainModuleError{ - MainModule: module.Version{Path: m.Path}, - Pattern: q.pattern, - Query: q.version, + reportError(q, &modload.QueryMatchesMainModulesError{ + MainModules: []module.Version{{Path: m.Path}}, + Pattern: q.pattern, + Query: q.version, }) return } diff --git a/src/cmd/go/internal/modload/build.go b/src/cmd/go/internal/modload/build.go index 73b51c117a1..0efd84123af 100644 --- a/src/cmd/go/internal/modload/build.go +++ b/src/cmd/go/internal/modload/build.go @@ -218,7 +218,6 @@ func moduleInfo(ctx context.Context, rs *Requirements, m module.Version, mode Li Version: m.Version, Main: true, } - _ = TODOWorkspaces("handle rawGoVersion here") if v, ok := rawGoVersion.Load(m); ok { info.GoVersion = v.(string) } else { diff --git a/src/cmd/go/internal/modload/buildlist.go b/src/cmd/go/internal/modload/buildlist.go index 9989bb5b2a4..14379b4c3c1 100644 --- a/src/cmd/go/internal/modload/buildlist.go +++ b/src/cmd/go/internal/modload/buildlist.go @@ -139,7 +139,7 @@ func (rs *Requirements) initVendor(vendorList []module.Version) { } if MainModules.Len() != 1 { - panic("There should be exactly one main moudle in Vendor mode.") + panic("There should be exactly one main module in Vendor mode.") } mainModule := MainModules.Versions()[0] @@ -284,8 +284,10 @@ func readModGraph(ctx context.Context, depth modDepth, roots []module.Version) ( ) for _, m := range MainModules.Versions() { // Require all roots from all main modules. - _ = TODOWorkspaces("This isn't the correct behavior. " + - "Fix this when the requirements struct is updated to reflect the struct of the module graph.") + _ = TODOWorkspaces("This flattens a level of the module graph, adding the dependencies " + + "of all main modules to a single requirements struct, and losing the information of which " + + "main module required which requirement. Rework the requirements struct and change this" + + "to reflect the structure of the main modules.") mg.g.Require(m, roots) } diff --git a/src/cmd/go/internal/modload/init.go b/src/cmd/go/internal/modload/init.go index 896c61d19dd..ab6733830fc 100644 --- a/src/cmd/go/internal/modload/init.go +++ b/src/cmd/go/internal/modload/init.go @@ -123,8 +123,6 @@ func (mms *MainModuleSet) Contains(path string) bool { } func (mms *MainModuleSet) ModRoot(m module.Version) string { - _ = TODOWorkspaces(" Do we need the Init? The original modRoot calls it. Audit callers.") - Init() if mms == nil { return "" } @@ -143,8 +141,11 @@ func (mms *MainModuleSet) mustGetSingleMainModule() module.Version { panic("internal error: mustGetSingleMainModule called in context with no main modules") } if len(mms.versions) != 1 { - _ = TODOWorkspaces("Check if we're in workspace mode before returning the below error.") - panic("internal error: mustGetSingleMainModule called in workspace mode") + if inWorkspaceMode() { + panic("internal error: mustGetSingleMainModule called in workspace mode") + } else { + panic("internal error: multiple main modules present outside of workspace mode") + } } return mms.versions[0] } @@ -156,11 +157,7 @@ func (mms *MainModuleSet) GetSingleIndexOrNil() *modFileIndex { if len(mms.versions) == 0 { return nil } - if len(mms.versions) != 1 { - _ = TODOWorkspaces("Check if we're in workspace mode before returning the below error.") - panic("internal error: mustGetSingleMainModule called in workspace mode") - } - return mms.indices[mms.versions[0]] + return mms.indices[mms.mustGetSingleMainModule()] } func (mms *MainModuleSet) Index(m module.Version) *modFileIndex { @@ -363,7 +360,9 @@ func Init() { // We're in module mode. Set any global variables that need to be set. cfg.ModulesEnabled = true setDefaultBuildMod() - _ = TODOWorkspaces("ensure that buildmod is readonly") + _ = TODOWorkspaces("In workspace mode, mod will not be readonly for go mod download," + + "verify, graph, and why. Implement support for go mod download and add test cases" + + "to ensure verify, graph, and why work properly.") list := filepath.SplitList(cfg.BuildContext.GOPATH) if len(list) == 0 || list[0] == "" { base.Fatalf("missing $GOPATH") @@ -374,15 +373,14 @@ func Init() { } if inWorkspaceMode() { - _ = TODOWorkspaces("go.work.sum, and also allow modfetch to fall back to individual go.sums") - _ = TODOWorkspaces("replaces") var err error modRoots, err = loadWorkFile(workFilePath) if err != nil { base.Fatalf("reading go.work: %v", err) } + _ = TODOWorkspaces("Support falling back to individual module go.sum " + + "files for sums not in the workspace sum file.") modfetch.GoSumFile = workFilePath + ".sum" - // TODO(matloob) should workRoot just be workFile? } else if modRoots == nil { // We're in module mode, but not inside a module. // @@ -539,6 +537,7 @@ func (goModDirtyError) Error() string { var errGoModDirty error = goModDirtyError{} func loadWorkFile(path string) (modRoots []string, err error) { + _ = TODOWorkspaces("Clean up and write back the go.work file: add module paths for workspace modules.") workDir := filepath.Dir(path) workData, err := lockedfile.Read(path) if err != nil { @@ -661,8 +660,7 @@ func loadModFile(ctx context.Context) (rs *Requirements, needCommit bool) { // We don't need to do anything for vendor or update the mod file so // return early. - _ = TODOWorkspaces("don't worry about commits for now, but eventually will want to update go.work files") - return rs, false + return rs, true } mainModule := MainModules.mustGetSingleMainModule() @@ -761,7 +759,7 @@ func CreateModFile(ctx context.Context, modPath string) { MainModules = makeMainModules([]module.Version{modFile.Module.Mod}, []string{modRoot}, []*modfile.File{modFile}, []*modFileIndex{nil}) addGoStmt(modFile, modFile.Module.Mod, LatestGoVersion()) // Add the go directive before converted module requirements. - convertedFrom, err := convertLegacyConfig(modFile, modPath) + convertedFrom, err := convertLegacyConfig(modFile, modRoot) if convertedFrom != "" { fmt.Fprintf(os.Stderr, "go: copying requirements from %s\n", base.ShortPath(convertedFrom)) } @@ -1037,7 +1035,7 @@ func mustHaveCompleteRequirements() bool { // convertLegacyConfig imports module requirements from a legacy vendoring // configuration file, if one is present. -func convertLegacyConfig(modFile *modfile.File, modPath string) (from string, err error) { +func convertLegacyConfig(modFile *modfile.File, modRoot string) (from string, err error) { noneSelected := func(path string) (version string) { return "none" } queryPackage := func(path, rev string) (module.Version, error) { pkgMods, modOnly, err := QueryPattern(context.Background(), path, rev, noneSelected, nil) @@ -1050,10 +1048,7 @@ func convertLegacyConfig(modFile *modfile.File, modPath string) (from string, er return modOnly.Mod, nil } for _, name := range altConfigs { - if len(modRoots) != 1 { - panic(TODOWorkspaces("what do do here?")) - } - cfg := filepath.Join(modRoots[0], name) + cfg := filepath.Join(modRoot, name) data, err := os.ReadFile(cfg) if err == nil { convert := modconv.Converters[name] @@ -1166,7 +1161,8 @@ func findWorkspaceFile(dir string) (root string) { break } if d == cfg.GOROOT { - _ = TODOWorkspaces("Address how go.work files interact with GOROOT") + _ = TODOWorkspaces("If we end up checking in a go.work file to GOROOT/src," + + "remove this case.") return "" // As a special case, don't cross GOROOT to find a go.work file. } dir = d @@ -1345,7 +1341,7 @@ func commitRequirements(ctx context.Context, goVersion string, rs *Requirements) // We aren't in a module, so we don't have anywhere to write a go.mod file. return } - mainModule := MainModules.Versions()[0] + mainModule := MainModules.mustGetSingleMainModule() modFilePath := modFilePath(MainModules.ModRoot(mainModule)) modFile := MainModules.ModFile(mainModule) @@ -1398,12 +1394,6 @@ func commitRequirements(ctx context.Context, goVersion string, rs *Requirements) base.Fatalf("go: %v", err) } defer func() { - if MainModules.Len() != 1 { - panic(TODOWorkspaces("There should be exactly one main module when committing reqs")) - } - - mainModule := MainModules.Versions()[0] - // At this point we have determined to make the go.mod file on disk equal to new. MainModules.SetIndex(mainModule, indexModFile(new, modFile, mainModule, false)) diff --git a/src/cmd/go/internal/modload/load.go b/src/cmd/go/internal/modload/load.go index cb5a2d7a353..c9004ff7964 100644 --- a/src/cmd/go/internal/modload/load.go +++ b/src/cmd/go/internal/modload/load.go @@ -552,8 +552,7 @@ func resolveLocalPackage(ctx context.Context, dir string, rs *Requirements) (str // return an error. if len(mainModulePrefix) > len(pkgNotFoundLongestPrefix) { pkgNotFoundLongestPrefix = mainModulePrefix - pkgNotFoundErr = &PackageNotInModuleError{Mod: mainModule, Pattern: pkg} - + pkgNotFoundErr = &PackageNotInModuleError{MainModules: []module.Version{mainModule}, Pattern: pkg} } continue } diff --git a/src/cmd/go/internal/modload/modfile.go b/src/cmd/go/internal/modload/modfile.go index 664fc0f91bd..09e9c67659e 100644 --- a/src/cmd/go/internal/modload/modfile.go +++ b/src/cmd/go/internal/modload/modfile.go @@ -322,7 +322,7 @@ func replacement(mod module.Version, index *modFileIndex) (fromVersion string, t // If there is no replacement for mod, Replacement returns // a module.Version with Path == "". func Replacement(mod module.Version) (module.Version, string) { - _ = TODOWorkspaces("support replaces in the go.work file") + _ = TODOWorkspaces("Support replaces in the go.work file.") foundFrom, found, foundModRoot := "", module.Version{}, "" for _, v := range MainModules.Versions() { if index := MainModules.Index(v); index != nil { diff --git a/src/cmd/go/internal/modload/query.go b/src/cmd/go/internal/modload/query.go index 6d6bfe774cd..82979fbda12 100644 --- a/src/cmd/go/internal/modload/query.go +++ b/src/cmd/go/internal/modload/query.go @@ -632,18 +632,16 @@ func QueryPattern(ctx context.Context, pattern, query string, current func(strin if modOnly != nil { return nil, modOnly, nil } else if len(mainModuleMatches) != 0 { - _ = TODOWorkspaces("add multiple main modules to the error?") - return nil, nil, &QueryMatchesMainModuleError{ - MainModule: mainModuleMatches[0], - Pattern: pattern, - Query: query, + return nil, nil, &QueryMatchesMainModulesError{ + MainModules: mainModuleMatches, + Pattern: pattern, + Query: query, } } else { - _ = TODOWorkspaces("This should maybe be PackageNotInModule*s* error with the main modules that are prefixes of base") return nil, nil, &PackageNotInModuleError{ - Mod: MainModules.Versions()[0], - Query: query, - Pattern: pattern, + MainModules: mainModuleMatches, + Query: query, + Pattern: pattern, } } } @@ -695,7 +693,7 @@ func QueryPattern(ctx context.Context, pattern, query string, current func(strin }) if len(mainModuleMatches) > 0 && len(results) == 0 && modOnly == nil && errors.Is(err, fs.ErrNotExist) { - return nil, nil, &QueryMatchesMainModuleError{ + return nil, nil, &QueryMatchesMainModulesError{ Pattern: pattern, Query: query, } @@ -893,6 +891,7 @@ func (e *WildcardInFirstElementError) Error() string { // code for the versions it knows about, and thus did not have the opportunity // to return a non-400 status code to suppress fallback. type PackageNotInModuleError struct { + MainModules []module.Version Mod module.Version Replacement module.Version Query string @@ -900,11 +899,15 @@ type PackageNotInModuleError struct { } func (e *PackageNotInModuleError) Error() string { - if MainModules.Contains(e.Mod.Path) { - if strings.Contains(e.Pattern, "...") { - return fmt.Sprintf("main module (%s) does not contain packages matching %s", e.Mod.Path, e.Pattern) + if len(e.MainModules) > 0 { + prefix := "workspace modules do" + if len(e.MainModules) == 1 { + prefix = fmt.Sprintf("main module (%s) does", e.MainModules[0]) } - return fmt.Sprintf("main module (%s) does not contain package %s", e.Mod.Path, e.Pattern) + if strings.Contains(e.Pattern, "...") { + return fmt.Sprintf("%s not contain packages matching %s", prefix, e.Pattern) + } + return fmt.Sprintf("%s not contain package %s", prefix, e.Pattern) } found := "" @@ -1153,21 +1156,29 @@ func (rr *replacementRepo) replacementStat(v string) (*modfetch.RevInfo, error) return rev, nil } -// A QueryMatchesMainModuleError indicates that a query requests +// A QueryMatchesMainModulesError indicates that a query requests // a version of the main module that cannot be satisfied. // (The main module's version cannot be changed.) -type QueryMatchesMainModuleError struct { - MainModule module.Version - Pattern string - Query string +type QueryMatchesMainModulesError struct { + MainModules []module.Version + Pattern string + Query string } -func (e *QueryMatchesMainModuleError) Error() string { +func (e *QueryMatchesMainModulesError) Error() string { if MainModules.Contains(e.Pattern) { return fmt.Sprintf("can't request version %q of the main module (%s)", e.Query, e.Pattern) } - return fmt.Sprintf("can't request version %q of pattern %q that includes the main module (%s)", e.Query, e.Pattern, e.MainModule.Path) + plural := "" + mainModulePaths := make([]string, len(e.MainModules)) + for i := range e.MainModules { + mainModulePaths[i] = e.MainModules[i].Path + } + if len(e.MainModules) > 1 { + plural = "s" + } + return fmt.Sprintf("can't request version %q of pattern %q that includes the main module%s (%s)", e.Query, e.Pattern, plural, strings.Join(mainModulePaths, ", ")) } // A QueryUpgradesAllError indicates that a query requests