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: go list should default to not modify go.mod and not exit non-zero when skipping go.mod update #41328

Closed
maruel opened this issue Sep 10, 2020 · 12 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@maruel
Copy link
Contributor

maruel commented Sep 10, 2020

What version of Go are you using (go version)?

go 1.15.2

What operating system and processor architecture are you using (go env)?

debian derived with GO111MODULE="on"

What did you do?

$ go list

inside a directory containing a go.mod that refers to packages that have known newer versions available.

What did you expect to see?

  1. ./go.mod is not touched.
  2. No error message.
  3. Exit code is unaffected by go.mod not being up to date.

A naive view would be that go list is a pure read-only operation. Seeing other go list related feature requests on GitHub, I realize this is not a shared expectation.

What did you see instead?

Using go list causes it to touch ./go.mod.

Using go list -mod=readonly stops touching ./go.mod but:

  • It exits 1 when it would normally have updated go.mod
  • It prints an error message which is noise to the user.

Workaround

Use the flag -mod=readonly at each invocation but:

  • Neither go help list nor go list -help document this flag, which hinders the UX.
  • go help modules does but go help modules | wc -w is more than 3200 words.

It can be "permanently" set as the default via:

go env -w GOFLAGS="-mod=readonly"

That's even less discoverable though, as one has to know about GOFLAGS and it has whole system impact.

Proposed change

My take would be:

  1. Have go list -mod=readonly exit 0 when it skips updating go.mod and not print an error message.
  2. Have go list default to -mod=readonly since it's a querying function.

More context for googlers: fxb/59330

@maruel
Copy link
Contributor Author

maruel commented Sep 11, 2020

I understand the second proposed change is potentially more controversial, but the first is hopefully reasonable.
@bcmills I hope you won't mind me paging you.

@bcmills
Copy link
Contributor

bcmills commented Sep 14, 2020

go list -e should certainly exit with status code 0 when the -e flag is set. However, it probably should not default to that behavior when -e is not set, since it would not necessarily be possible to actually build the packages in the listed configuration.

@bcmills
Copy link
Contributor

bcmills commented Sep 14, 2020

Defaulting to -mod=readonly is included in proposal #40728.

@maruel
Copy link
Contributor Author

maruel commented Sep 14, 2020

Since Russ wrote #40728, I'm hopeful this will be fixed in 1.16.

I still did analysis as this means the situation won't be better for another 6 months. None of the flag combination helps, they all misbehave in a way or another on go1.15.2. So even if the other issue is fixed, there should be better flag coherency in go list.

The following commands were run within fuchsia.git/tools:

  • ./affected_targets is a directory that does not contain .go files
  • ./artifactory is a normal go package
  • ./non_existent doesn't exist on disk

Summary

  • without flag: errors out without go file or invalid path; modifies go.mod
  • with -e: prints outputs even if package doesn't exist; modifies go.mod
  • with -mod=readonly: no output, always an error; no modification
  • with -e -mod=readonly: no output, always an error; no modification (-e has no effect)

None of the behavior is satisfactory.

Raw data

Without flag:

$ go list ./affected_targets; echo $?
no Go files in /.../fuchsia/tools/affected_targets
1
$ git status --porcelain; git reset --hard -q
 M tools/go.mod

$ go list ./artifactory; echo $?
go.fuchsia.dev/fuchsia/tools/artifactory
0
$ git status --porcelain; git reset --hard -q
 M tools/go.mod

$ go list ./non_existent; echo $?
stat /.../fuchsia/tools/non_existent: directory not found
1
$ git status --porcelain; git reset --hard -q
 M tools/go.mod

With -e only:

$ go list -e ./affected_targets; echo $?
./affected_targets
0
$ git status --porcelain; git reset --hard -q
 M tools/go.mod

$ go list -e ./artifactory; echo $?
go.fuchsia.dev/fuchsia/tools/artifactory
0
$ git status --porcelain; git reset --hard -q
 M tools/go.mod

$ go list -e ./non_existent; echo $?
./non_existent
0
$ git status --porcelain; git reset --hard -q
 M tools/go.mod

Wth -mod=readonly only:

$ go list -mod=readonly ./affected_targets; echo $?
go: updates to go.mod needed, disabled by -mod=readonly
1
$ git status --porcelain; git reset --hard -q

$ go list -mod=readonly ./artifactory; echo $?
go: updates to go.mod needed, disabled by -mod=readonly
1
$ git status --porcelain; git reset --hard -q

$ go list -mod=readonly ./non_existent; echo $?
go: updates to go.mod needed, disabled by -mod=readonly
1
$ git status --porcelain; git reset --hard -q

Both -e -mod=readonly:

$ go list -e -mod=readonly ./affected_targets; echo $?
go: updates to go.mod needed, disabled by -mod=readonly
1
$ git status --porcelain; git reset --hard -q

$ go list -e -mod=readonly ./artifactory; echo $?
go: updates to go.mod needed, disabled by -mod=readonly
1
$ git status --porcelain; git reset --hard -q

$ go list -e -mod=readonly ./non_existent; echo $?
go: updates to go.mod needed, disabled by -mod=readonly
1
$ git status --porcelain; git reset --hard -q

@bcmills
Copy link
Contributor

bcmills commented Sep 14, 2020

@maruel, if you want fine-grained control over the outputs of go list in case of unresolved dependencies, you need to use the -e flag in conjunction with the -f flag. (-e will suppress the default error behavior, and -f will allow you to supply your own formatting string, which can be used to suppress output of packages with errors.)

@bcmills
Copy link
Contributor

bcmills commented Sep 14, 2020

Also note that the -e -mod=readonly behavior has already changed in Go 1.16 (as of CL 251445).

@bcmills bcmills changed the title go list should default to not modify go.mod and not exit non-zero when skipping go.mod update cmd/go: go list should default to not modify go.mod and not exit non-zero when skipping go.mod update Sep 14, 2020
@bcmills
Copy link
Contributor

bcmills commented Sep 14, 2020

@maruel, can you try a go build from head and see whether you can get go list -e -mod=readonly -f … to work with some suitable template as the -f flag argument?

@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Sep 14, 2020
@bcmills bcmills added this to the Unplanned milestone Sep 14, 2020
@maruel
Copy link
Contributor Author

maruel commented Sep 14, 2020

$ go version
go version devel +506eb0a9b1 Mon Sep 14 21:33:53 2020 +0000 linux/amd64
$ go list -e -mod=readonly -f '{{.ImportPath}}' ./artifactory
go: updates to go.mod needed, disabled by -mod=readonly
$ echo $?
1

@bcmills
Copy link
Contributor

bcmills commented Sep 15, 2020

Ok. So what changes to the go.mod file occur if you pass -mod=mod instead?
(Is the problem that the existing go.mod file is initially in an inconsistent state?)

In general -mod=readonly expects that the go.mod file is consistent and complete, and that the go.sum file contains checksums for all modules relevant to the command.

We can reasonably make -e report package errors for packages missing from the go.mod file, but I don't think we should allow it to bypass a go.mod file that explicitly requires a version lower than what is implied by the module's transitive dependencies.

We could perhaps allow -e to report package errors for packages whose module is missing from the go.sum file, but that seems like a fairly invasive change, and given the ease of committing a valid go.sum file I don't think it would be a top priority at the moment.

@bcmills bcmills added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Sep 15, 2020
@maruel
Copy link
Contributor Author

maruel commented Sep 15, 2020

$ git reset --hard -q
$ go version
go version devel +506eb0a9b1 Mon Sep 14 21:33:53 2020 +0000 linux/amd64
$ ll go.*
-rw-r----- 1 maruel primarygroup  2395 2020-09-15 14:01 go.mod
-rw-r----- 1 maruel primarygroup 23003 2020-09-11 21:00 go.sum

$ go list -mod=mod ./artifactory; echo $?
go.fuchsia.dev/fuchsia/tools/artifactory
0
$ git diff
diff --git tools/go.mod tools/go.mod
index 2e515f4fb5a..38a9a1cd2e8 100644
--- tools/go.mod
+++ tools/go.mod
@@ -19,9 +19,9 @@ require (
        go.fuchsia.dev/fuchsia/garnet v0.0.0-20200821151753-3226fa91b98e
        go.fuchsia.dev/fuchsia/src v0.0.0-20200821151753-3226fa91b98e
        golang.org/x/crypto v0.0.0-20200820211705-5c72a883971a
-       golang.org/x/net v0.0.0-20200813134508-3edf25e44fcc
+       golang.org/x/net v0.0.0-20200822124328-c89045814202
        golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208
-       golang.org/x/sys v0.0.0-20200821140526-fda516888d29
+       golang.org/x/sys v0.0.0-20200828081204-131dc92a58d5
        golang.org/x/text v0.3.3 // indirect
        golang.org/x/tools v0.0.0-20200821151209-74543c403428 // indirect
        google.golang.org/api v0.30.0 // indirect

$ git reset --hard -q
$ go list -mod=readonly ./artifactory; echo $?
go: updates to go.mod needed, disabled by -mod=readonly
1

Important note: go.sum is not tracked by git; I added it to .gitignore since we use a meta-checkout tool and use replace statements. We use replace statements to use the local versions: https://fuchsia.googlesource.com/fuchsia/+/master/tools/go.mod

That said the only thing we want is that go list stops updating minor version in go.mod unintentionally. It messes up the developer flow for people that open VS Code with the Go language server setup, or use vim-go.

I don't think -e should be used. I think the fix should be that -mod=readonly should not complain if a newer minor version is available.

@bcmills
Copy link
Contributor

bcmills commented Sep 15, 2020

That said the only thing we want is that go list stops updating minor version in go.mod unintentionally.

I think the fix should be that -mod=readonly should not complain if a newer minor version is available.

This has nothing to do with whether a newer minor version is available. (The point of Go Modules is to provide reproducibility, not recency: go list will not update the version of anything implicitly unless it need to do so in order to resolve an imported package that is not already provided by any module in the module graph.)

I have a guess: you say you “use a meta-checkout tool and use replace statements”. Does the meta-checkout tool invoke go list and/or go mod tidy in order to ensure that its interpretation of the module requirements is consistent with the go tool's interpretation? That may be the bug here: this third-party tool (or perhaps a hand-edit) is generating an inconsistent go.mod file, and then go list is telling you (in an admittedly roundabout way) that the go.mod file does not mean what it superficially appears to mean.


Removing the replace directives and running go list -mod=mod -e ./... (to get to a consistent state) and go mod graph | grep c89045 (to see where one of the upgraded dependencies comes from), I see:

~/src/go.fuchsia.dev/fuchsia/tools$ go mod graph | grep c89045814202
go.fuchsia.dev/fuchsia/tools golang.org/x/net@v0.0.0-20200822124328-c89045814202
cloud.google.com/go@v0.65.0 golang.org/x/net@v0.0.0-20200822124328-c89045814202

That is: go list is telling you that the explicit requirement on golang.org/x/net v0.0.0-20200813134508-3edf25e44fcc contradicts the transitive requirement through cloud.google.com/go v0.65.0.


So the easy fix here is to check in a go.mod file that specifies a consistent set of versions, such that no dependency requires a higher version than what is listed in your go.mod file. You can obtain such a file by running go mod tidy, go list -mod=mod -m all, go get -d all, or various other commands.

@bcmills bcmills added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Sep 23, 2020
@maruel
Copy link
Contributor Author

maruel commented Oct 23, 2020

Our meta checkout tool does not invoke any go tool, since we build the Go toolchain ourselves; Fuchsia support hasn't been upstreamed yet.

Will close, my expectation for now is that we'll stay in a non optimal state for the foreseeable future.

@maruel maruel closed this as completed Oct 23, 2020
@golang golang locked and limited conversation to collaborators Oct 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

3 participants