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: do not update 'go.mod' automatically if the changes are only cosmetic #34822

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

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

bcmills opened this issue Oct 10, 2019 · 10 comments
Labels
Milestone

Comments

@bcmills
Copy link
Member

@bcmills 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 #34534).

CC @jayconrod

@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Oct 11, 2019

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

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Oct 11, 2019

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

@nd

This comment has been minimized.

Copy link
Contributor

@nd 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

This comment has been minimized.

Copy link

@tv42 tv42 commented Oct 18, 2019

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

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 31, 2019

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
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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 1, 2019

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
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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 1, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 1, 2019

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
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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 5, 2019

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
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
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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 5, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.