x/exp/cmd/gorelease: avoid using -mod=mod and support lazy module loading #41456

jayconrod · 2020-09-17T15:38:07Z

gorelease uses go list $modpath/... to build a list of packages it needs to load and compare (where $modpath is the path of the module being released). Go 1.16 has -mod=readonly on by default (#40728), so if the module is missing one or more sums, this may result in an error like

go: updates to go.sum needed, disabled by -mod=readonly

We can work around this temporarily by explicitly using -mod=mod.

Go 1.16 will also support lazy module loading (#36460). This may require changes to the way gorelease generates temporary go.mod files for loading packages in downloaded modules, since nothing is imported in those temporary modules. We can work around this temporarily by using go 1.16 in temporary go.mod files.

A possible solution to both problems is to replace the go list call with a walk of the directory tree, listing non-main, non-internal packages (importable by other modules). Once we have such a list, we can generate a temporary .go file that imports those packages, then run go get -d on that temporary package. That will ensure it's safe to call packages.Load later.

cc @jadekler @bcmills

The text was updated successfully, but these errors were encountered:

gopherbot · 2020-09-17T15:46:08Z

Change https://golang.org/cl/255597 mentions this issue: cmd/gorelease: use -mod=mod when listing packages

bcmills · 2020-09-17T15:48:04Z

FWIW, I think the temporary-go.mod approach will continue to work after lazy loading. (The dependencies of the target module will still all be present in the deepening scan, as if the synthetic module imported something from the target module in some package that we just don't happen to be looking at right now.)

bcmills · 2020-09-17T15:52:08Z

I think you can fix the -mod=readonly issue by running go get $modpath@$version before go list, unless $modpath is a local replacement (in which case you can copy over its go.sum file).

bcmills · 2020-09-17T15:56:30Z

Ah, but I guess the go.sum file for the module itself may be incomplete if it has any replace directives. 😞

go mod download should do the trick, though! (It just might download more than is strictly necessary.)

jayconrod · 2020-09-17T15:57:09Z

I thought about go get $modpath@$version; that would add the sum for the module itself, but we can get that already from go mod download -json, which runs earlier. We copy over the go.sum file already as well.

The bigger problem is if the module's go.sum file is missing or incomplete. We can't use go list -mod=readonly $modpath/... to make a list of importable packages in that case. go mod tidy won't work in the temporary module since there are no imports. go get $modpath/... would have other side effects if there are nested modules.

bcmills · 2020-09-17T15:59:44Z

Yeah, considering all the alternatives I suspect that the go mod tidy approach you describe in the CL is probably the most robust. (It's also the best approximation of what real users of the module-to-be-released will do, so a more faithful test in that regard.)

jeanbza · 2020-09-17T18:15:20Z

Quick check - is this discussion centered around making gorelease fetch to file modules that are in the dependency graph but not yet downloaded to file? (so that gorelease can go inspect them)

I ask because I was wondering why we're talking about go.mod / go.sum changing - seems like things that shouldn't happen. And, if it does happen, seems like things that cause gorelease to emit a requirements incomplete message.

(sorry for the potentially unintelligent question, just catching up :) )

This fixes test failures when gorelease is used with the tip Go 1.16. For golang/go#41456 Change-Id: I9a1452518113f595d9c94fb394f0aecbcc80b95f Reviewed-on: https://go-review.googlesource.com/c/exp/+/255597 Trust: Jay Conrod <jayconrod@google.com> Trust: Jean de Klerk <deklerk@google.com> Run-TryBot: Jay Conrod <jayconrod@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Jean de Klerk <deklerk@google.com>

jayconrod · 2020-09-17T18:52:43Z

@jadekler Right, this is mainly about gorelease fetching modules that are in the dependency graph but not in go.sum. It also happens due to the way we load the base module (we create a go.mod in a temp temporary, then require the base module version), though we could work around that.

With -mod=mod (the default until now), when we load packages with go list (either directly or via packages.Load), the go command will update go.sum automatically, and we can check whether it changed after the fact.

With -mod=readonly (the new default in Go 1.16), the go command reports a hard error if something is missing from go.sum and doesn't load anything. We can still print a diagnostic in that case, but we won't be able to report API changes.

jeanbza · 2020-09-17T19:43:07Z

Ahhh. Thank you! This helps greatly.

gopherbot · 2020-10-16T23:57:20Z

Change https://golang.org/cl/263178 mentions this issue: x/exp/cmd/gorelease: avoid using -mod=mod and support lazy module loading

…ding Details: - Replaces `go list -mod=mod` with `go list -mod=readonly`. - Switches to `go get -d` to download dependencies rather than relying on `go list` doing so. - Missing go.sum/go.mod data is now an error. This CL will be followed by a CL that ensures the filepath.Walk stops at submodules, and a test asserting that. I figured it'd be good to do separately since this CL is getting to be hefty. Give a shout if you prefer otherwise! Updates golang/go#41456 Change-Id: I66f12fe0325e4fed432d8b10c1fe977068e4a030 Reviewed-on: https://go-review.googlesource.com/c/exp/+/263178 Run-TryBot: Jean de Klerk <deklerk@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com> Trust: Jean de Klerk <deklerk@google.com>

jeanbza · 2021-01-26T17:17:51Z

Quick note: I intend on finishing this out by adding a bit more logic+test around not walking into submodules when considering which packages to import in prepareLoadDir.

gopherbot · 2021-01-29T17:13:43Z

Change https://golang.org/cl/287972 mentions this issue: x/exp/cmd/gorelease: add test for tidy submodule

Details: - Adds a test for the missing req submodule case. However, it turns out we don't need any code in the filepath.Walk to handle submodules, since earlier in copyModuleToTempDir we only copy the module's contents (not any submodule contents) with zip.CreateFromDir. This means that prepareLoadDir is never going to consider any submodule content, and we don't have to build logic for that. The test will maintain that guarantee. - Remove some extraneous go.mod dupes from other tests. - Remove a TODO related to filepath.Walk submodule checking which is no longer needed. Fixes golang/go#41456 Change-Id: Ib0bac2fb227d4175c31603880b629374991b8bde Reviewed-on: https://go-review.googlesource.com/c/exp/+/287972 Run-TryBot: Jean de Klerk <deklerk@google.com> TryBot-Result: Go Bot <gobot@golang.org> Trust: Jean de Klerk <deklerk@google.com> Reviewed-by: Jay Conrod <jayconrod@google.com>

jayconrod added NeedsFix modules Tools labels Sep 17, 2020

jayconrod added this to the Unreleased milestone Sep 17, 2020

jayconrod modified the milestones: Unreleased, gorelease Oct 14, 2020

jeanbza self-assigned this Jan 26, 2021

gopherbot closed this as completed in golang/exp@d352d2d Feb 1, 2021

golang locked and limited conversation to collaborators Feb 1, 2022

gopherbot added the FrozenDueToAge label Feb 1, 2022

rsc unassigned jeanbza Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/exp/cmd/gorelease: avoid using -mod=mod and support lazy module loading #41456

x/exp/cmd/gorelease: avoid using -mod=mod and support lazy module loading #41456

jayconrod commented Sep 17, 2020

gopherbot commented Sep 17, 2020

bcmills commented Sep 17, 2020

bcmills commented Sep 17, 2020

bcmills commented Sep 17, 2020

jayconrod commented Sep 17, 2020

bcmills commented Sep 17, 2020

jeanbza commented Sep 17, 2020 •

edited

Loading

jayconrod commented Sep 17, 2020

jeanbza commented Sep 17, 2020

gopherbot commented Oct 16, 2020

jeanbza commented Jan 26, 2021

gopherbot commented Jan 29, 2021

x/exp/cmd/gorelease: avoid using -mod=mod and support lazy module loading #41456

x/exp/cmd/gorelease: avoid using -mod=mod and support lazy module loading #41456

Comments

jayconrod commented Sep 17, 2020

gopherbot commented Sep 17, 2020

bcmills commented Sep 17, 2020

bcmills commented Sep 17, 2020

bcmills commented Sep 17, 2020

jayconrod commented Sep 17, 2020

bcmills commented Sep 17, 2020

jeanbza commented Sep 17, 2020 • edited Loading

jayconrod commented Sep 17, 2020

jeanbza commented Sep 17, 2020

gopherbot commented Oct 16, 2020

jeanbza commented Jan 26, 2021

gopherbot commented Jan 29, 2021

jeanbza commented Sep 17, 2020 •

edited

Loading