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

Jump to bottom

cmd/go: do not update 'go.mod' automatically if the changes are only cosmetic #34822

Closed
bcmills opened this issue Oct 10, 2019 · 11 comments
Closed

cmd/go: do not update 'go.mod' automatically if the changes are only cosmetic #34822

bcmills opened this issue Oct 10, 2019 · 11 comments
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.14

Comments

@bcmills
Copy link
Contributor

bcmills commented Oct 10, 2019

#29452, #33779, and #31870 all describe issues due to cosmetic changes in the go.mod file (specifically, newlines). CL 186557 changed the go command to properly omit versions already implied by the module graph.

It is possible that future changes to the go command will similarly add or remove redundant or non-essential information from the go.mod file. (For example, #25898 (comment) suggests adding a comment for pseudo-versions indicating the corresponding non-semver tag, if any.)

It is annoying and sometimes intrusive for the go command to make unnecessary edits to the go.mod file.

go mod tidy should apply cosmetic edits to tidy up the file: that is part of its purpose. However, for other commands, I suggest:

  • The go command should not overwrite the go.mod file if it contains only formatting changes (spacing, newlines, ordering, grouping, etc.) or removes redundant-but-not-misleading information (such as a require line that specifies an indirect dependency already implied by some other indirect dependency).

  • If -mod=readonly is set, the go command should not fail if the go.mod file contains additional comment-only changes (such as the addition or removal of an // indirect comment), or omits helpful-but-unnecessary information (such as a require line for a direct dependency whose version is already implied by an indirect one, but contrast cmd/go: update to '@latest' when promoting an indirect dependency to a direct one? #34534).

CC @jayconrod
@bcmills bcmills added NeedsFix The path to resolution is known, but the work has not been done. modules labels Oct 10, 2019
@bcmills bcmills added this to the Go1.14 milestone Oct 10, 2019
This was referenced Oct 10, 2019
Closed
Closed
@dmitshur
Copy link
Contributor

dmitshur commented Oct 11, 2019 •
edited

Would it be a reasonable analogy to say this is like running the go build command on poorly formatted Go code. The go build command successfully builds such code but does not automatically apply formatting changes to it. One needs to use gofmt (or equivalent) separately to format Go code.

@bcmills
Copy link
Contributor Author

bcmills commented Oct 11, 2019 •
edited

Yes, except that the go command (unlike the compiler) does automatically update the go.mod file as needed to get things to build.

@nd
Copy link
Contributor

nd commented Oct 12, 2019

The analogy above doesn't work: compiler doesn't change the code in case of compilation errors even if they are obvious; go build doesn't reformat the code: one has to run a dedicated command for that explicitly.

IMHO it is hard to integrate with tools behaving like this. Every client will have to check and handle go.mod changes and roll them back if they are undesired. The go.mod file might not be under my control. If I want to list modules I will have to revert changes in go.mod again and again. I understand the intention of this behavior: to keep go.mod in a good shape and to add mandatory entries there, it is totally reasonable to have this behavior by default, but it would be great to have a mode with this logic disabled.

@tv42
Copy link

tv42 commented Oct 18, 2019 •
edited

but it would be great to have a mode with this logic disabled.

That'd be -mod=readonly, I believe.

@gopherbot
Copy link

Change https://golang.org/cl/204521 mentions this issue: cmd/go: default to mod=readonly when the go.mod file is read-only

gopherbot pushed a commit that referenced this issue Nov 1, 2019
cmd/go: default to mod=readonly when the go.mod file is read-only
2c8529c 
Updates #30185
Updates #33326
Updates #34822

Change-Id: Ie13651585898d1bbbf4f779b97ee50b6c7e7ad50
Reviewed-on: https://go-review.googlesource.com/c/go/+/204521
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/204821 mentions this issue: modfile: ensure that Cleanup removes extraneous entries after SetRequire

gopherbot pushed a commit to golang/mod that referenced this issue Nov 1, 2019
modfile: ensure that Cleanup removes extraneous entries after SetRequire
a222b96 
Updates golang/go#34822

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

Change https://golang.org/cl/204877 mentions this issue: cmd/go.mod: upgrade x/mod to pull in CL 204821

@gopherbot
Copy link

Change https://golang.org/cl/204878 mentions this issue: cmd/go: make commands other than 'tidy' prune go.mod less agressively

gopherbot pushed a commit that referenced this issue Nov 5, 2019
cmd/go.mod: upgrade x/mod to pull in CL 204821
8550a58 
Updates #34822

Change-Id: I189d93ebd3ce6cd1b8f1e29336876fd82a7cfff7
Reviewed-on: https://go-review.googlesource.com/c/go/+/204877
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/205497 mentions this issue: modfile: prefer to add a new 'go' directive after the 'module' directive

gopherbot pushed a commit to golang/mod that referenced this issue Nov 5, 2019
modfile: prefer to add a new 'go' directive after the 'module' directive
c90efee 
The previous behavior put the new 'go' directive at the end of the
file, which is not where anybody actually puts it, and that in turn
complicates the regression tests for golang/go#34822.

Updates golang/go#34822

Change-Id: Ic275eb6999fc0db7e72e2d9636b77c9e3d919539
Reviewed-on: https://go-review.googlesource.com/c/mod/+/205497
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
gopherbot pushed a commit that referenced this issue Nov 5, 2019
cmd: update x/mod to CL 205497
649f341 
Also revert an incidental 'gofmt' of a vendored file from CL 205240.

Updates #34822

Change-Id: I82a015d865db4d865b4776a8013312f25dbb9181
Reviewed-on: https://go-review.googlesource.com/c/go/+/205539
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/205539 mentions this issue: cmd: update x/mod to CL 205497

@bcmills bcmills mentioned this issue Nov 26, 2019
Closed
@bcmills bcmills mentioned this issue Dec 17, 2019
Closed
@gopherbot
Copy link

Change https://golang.org/cl/219237 mentions this issue: cmd/go: eliminate empty '()' when passing -mod=readonly explicitly to 'go list'

gopherbot pushed a commit that referenced this issue Feb 12, 2020
cmd/go: eliminate empty '()' when passing -mod=readonly explicitly to…
363bcd0 
… 'go list'

Discovered while investigating #37197.

Updates #33326
Updates #34822

Change-Id: I38b136a4ee762a580a554125066b9778491295f2
Reviewed-on: https://go-review.googlesource.com/c/go/+/219237
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@sthagen sthagen mentioned this issue Feb 12, 2020
Merged
@codyoss codyoss mentioned this issue Jul 21, 2020
Closed
@golang golang locked and limited conversation to collaborators Feb 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
5 participants
@nd @tv42 @dmitshur @bcmills @gopherbot