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: add mod tidy -check #27005

Open
fishy opened this issue Aug 15, 2018 · 34 comments
Open

cmd/go: add mod tidy -check #27005

fishy opened this issue Aug 15, 2018 · 34 comments
Assignees
Milestone

Comments

@fishy
Copy link

@fishy fishy commented Aug 15, 2018

Please answer these questions before submitting your issue. Thanks!

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

go version go1.11rc1 darwin/amd64

Does this issue reproduce with the latest release?

N/A

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

GOARCH="amd64"
GOOS="darwin"

What did you do?

  • Remove lines from go.mod and go.sum files
  • Run go mod tidy to add them back

What did you expect to see?

go mod tidy should return non-zero as it made changes. This way it could be integrated to tools like pre-commit hooks to make sure that we don't commit untidied go.mod and go.sum files into version control

What did you see instead?

go mod tidy returned zero

@mvdan
Copy link
Member

@mvdan mvdan commented Aug 15, 2018

Non-zero exit codes are usually for errors, so having it behave like this by default would be counter-intuitive.

Perhaps it could have something similar to gofmt, which has the -l and -d flags to tell if any changes were made.

@swtch1
Copy link

@swtch1 swtch1 commented Aug 15, 2018

If the files are successfully added why wouldn't you want to see a 0 status code? This means that the following script would fail:

#!/usr/bin/env bash
set -e
go mod tidy  # <-- non 0 return code
echo 'we never get here'
@fishy
Copy link
Author

@fishy fishy commented Aug 15, 2018

That makes sense. How about maybe make it optional? e.g. when it's executed with a flag it should return non-zero when it made any changes.

@thepudds
Copy link

@thepudds thepudds commented Aug 15, 2018

@gopherbot, please add label modules

@rsc rsc changed the title modules: go mod tidy should return non-zero when it made changes to go.mod and/or go.sum files cmd/go: add mod tidy -check Aug 18, 2018
@rsc
Copy link
Contributor

@rsc rsc commented Aug 18, 2018

We could think about adding a flag that means "don't do the update, but fail if one is needed".
Not sure exactly what to call it but I put -check in the description. Needs more thought.

@rsc rsc added this to the Go1.12 milestone Aug 18, 2018
@bcmills bcmills modified the milestones: Go1.12, Go1.13 Oct 24, 2018
radeksimko added a commit to vancluever/terraform-provider-acme that referenced this issue Feb 19, 2019
 - 'go mod verify' is not that useful and isn't equivalent to govendor status.
 - 'go mod tidy -v' does not return non-zero exit code when dependencies aren't tidy
 - We might add 'go mod tidy -check' at some point, when it becomes a thing.
   See golang/go#27005
   Until then it's better to keep provider setup aligned though.
@nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Feb 25, 2019

@rsc regarding your comment in #26850

You could use 'go list all -mod=readonly' to check that at least for the current GOOS/GOARCH no changes are needed.

go list all -mod=readonly causes changes for me to go.sum without erroring.

Is this a bug?

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Apr 24, 2019

When this is implemented, we should run this on the TryBots (#31640).

Ideally go mod tidy -check would not access the network, since it will commonly be run in CI environments where network access is restricted. We might want to do so to provide diagnostics though. GOPROXY=off is still a good signal that we shouldn't access the network.

@nmiyake
Copy link

@nmiyake nmiyake commented May 23, 2019

This is a feature that we would like to see for our CI environment as well (we want to verify that go.mod and go.sum are in the proper state for all submissions for all possible build constraints).

I echo the sentiment expressed by @jayconrod that it would be ideal if this check could be performed without network access -- is it feasible for the -check mode to verify only that no change needs to be made without making network queries? My intuition is that the information provided by the source code, go.mod and go.sum should be sufficient to make this determination, but I'm not completely sure about this.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@detailyang
Copy link

@detailyang detailyang commented Jul 22, 2019

It's would be useful to reduce git conflict in lint scenario

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@mcandre
Copy link

@mcandre mcandre commented Nov 7, 2019

Networkless operation is a nice to have. Dry-run flags and intuitive exit codes are perhaps a more immediate concern.

@leighmcculloch
Copy link
Contributor

@leighmcculloch leighmcculloch commented Dec 12, 2019

How about maybe make it optional? e.g. when it's executed with a flag it should return non-zero when it made any changes.

This sounds to me like the solution that is most similar to how other tools solve this problem. golint uses this pattern already, and so do other tools like git diff. I'd prefer this over a -check command that alters the other behaviors of the tool because by altering the behavior of the tool there is more to learn and understand.

It'd be good to surface an exit code mode consistently across all the Go tool commands that we routinely use as checks in CI pipelines.

Today running checks for go mod, go fmt, and golint all use unique and unintuitive patterns:

OUTPUT=$(gofmt -d .)
if [[ $OUTPUT ]]; then
    exit 1
fi
golint -set_exit_status
go mod tidy
git diff --exit-code -- go.mod go.sum

We could reuse the pattern demonstrated by golint:

golint -set_exit_status
go fmt -set_exit_status
go mod tidy -set_exit_status

Or, we could take the opportunity to use a term that is used by other tools, like -exit-code:

golint -exit-code
go fmt -exit-code
go mod tidy -exit-code
@mvdan
Copy link
Member

@mvdan mvdan commented Jan 14, 2021

To recap: We seem to have consensus that this should be behind a flag.

On yesterday's tools call, @jayconrod wondered if it would be helpful to print more user-friendly output, like a diff. @bcmills says we should make a decision on that at the same time we decide on the flag name. It would make little sense for an -exit-code flag to also print a diff, for example.

Personally, I think we should just name the flag -check and have it return a short error, telling the user that the module is not tidy and how to fix it. The user will most likely have to run go mod tidy to fix the problem anyway, so I don't think a diff will be very helpful. Moreover, the diff would likely include go.sum too, so the size could get out of hand quickly with large projects, making the command tricky to use in CI.

Edit: To clarify, go mod tidy -check would not modify the go.mod and go.sum files. It would simply check that they are tidy. This is helpful for many cases where one just wants to run a sanity check without potentially changing the module or losing changes, like in CI.

Please give a thumbs up if you agree, or a thumbs down if you don't - along with a comment explaining why :) Thanks!

@iand
Copy link
Contributor

@iand iand commented Jan 14, 2021

I think it would be consistent to use -mod=readonly instead of -check. It was raised in #26850 which defers to this issue.

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Jan 14, 2021

None of the go mod subcommands accept -mod right now, and -mod=readonly will be the default for build commands in 1.16. I think it would be surprising to use -mod=readonly here; -check seems more natural to me.

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 14, 2021

We could also consider naming the flag -n, analogous to go build -n, and have it print a summary of the requirements that would be added and removed, but then a nonzero exit code probably wouldn't be appropriate.

@leighmcculloch
Copy link
Contributor

@leighmcculloch leighmcculloch commented Jan 14, 2021

Outputting something like a diff of what would change would be very useful, and would make it easier to understand why the command failed.

@mvdan
Copy link
Member

@mvdan mvdan commented Jan 14, 2021

Did you see my reasoning above against diffs? It's not uncommon for large projects to have go.sum files in the thousands of lines, for example. A diff output could get large to the point that the end user can't even see what command failed due to pagination/truncation. It's also not clear that a diff would help that much; a message like "your module is not tidy, run go mod tidy" should be plenty.

@rsc
Copy link
Contributor

@rsc rsc commented Jan 19, 2021

It seems like go mod tidy -diff would print useful information in a variety of contexts, and the exit code would get the boolean that people want. Adding -check instead would only provide one bit of information, which is less useful.

Let's add -diff.

@rsc
Copy link
Contributor

@rsc rsc commented Jan 19, 2021

(And if the diff is huge, all the more reason to see it!)

@Helcaraxan
Copy link
Contributor

@Helcaraxan Helcaraxan commented Jan 19, 2021

The key point for me with the huge diffs is that their relevance is extremely variable depending on the context. Within automation (e.g CI), the people writing the scripts and their end-users tend to only care about the binary value provided by the exit-code. Add to that as @mvdan puts it that many of those CI systems will truncate the huge diff anyway rendering it useless despite all its good intent.

It's only when manually invoked that you tend to potentially be interested in the content of that huge diff. But in those cases, on your local workstation, you can get the same result already by running go mod tidy and using your VCS's built-in diff functionality. Hence I do not necessarily see the value add of a go mod tidy -diff over the proposed go mod tidy -check combined with other one-line commands that give the same result.

The one thing where I could see the value add of go mod tidy -diff is if its not a literal diff but rather a summarised report over what changed and why. That would however be a much more complex diagnostic tool to write than a simple check as is the aim of this issue.

@mvdan
Copy link
Member

@mvdan mvdan commented Jan 19, 2021

I still don't see the benefit to -diff over -check - I share @Helcaraxan's concerns. Another potential issue is consistency; gofmt -d prints a diff but the exit code does not correlate with the printing of a non-empty diff. There have been a few issues in the past to ask gofmt -d to return a non-exit status when code would be changed by gofmt, but as far as I know all such suggestions have been rejected.

@mvdan
Copy link
Member

@mvdan mvdan commented Jan 19, 2021

All that said, I think we really need this feature so I won't insist on the details. I just wanted to make my position clear that I don't see how we're getting to "print a diff".

@leighmcculloch
Copy link
Contributor

@leighmcculloch leighmcculloch commented Jan 19, 2021

I think what is described with -diff is great 🎉. Diffs are useful because it means the developer doesn't need to do anything other than look at CI to know what's wrong. I use CI scripts to output diffs even if the tool doesn't support them, for example:
https://github.com/stellar/go/blob/9791cbf/gomod.sh#L4-L6

If someone doesn't want the diff and just the error code they can always pipe stdout to /dev/null.

-diff would do what I want, but these could also be composed:

  • -check set exit code to non-zero if there's a diff
  • -diff outputs diff

It's also worth noting there is some inconsistency with other tools such as git diff and golint which won't return a non-zero exit code unless parameters are passed (e.g. #27005 (comment)) and consistency could be nice for familiarity moving between tools.

@rsc If we go with -diff would go mod tidy still update the files as go mod tidy does today? Or would it not?

@Helcaraxan
Copy link
Contributor

@Helcaraxan Helcaraxan commented Jan 19, 2021

@leighmcculloch regarding your CI story I understand and agree with the general perspective that diffs can be useful. However in my experience the usefulness correlates strongly with the information contained in the diff being relevant to the user action that needs to follow. This happens usually when there are multiple courses of action available with the developer having to use their knowledge to decide what to do.

In the case that we are discussing here the only possible user action that follows the CI failure is to run go mod tidy and push the result to the branch. There is no need for "developer knowledge" to decide what to do. It's a simple one rule action with no conditionality. Indicating to a user that's the thing to do would actually be obscured by a long diff and would instead benefit from a short and clean error message instead.

@rsc
Copy link
Contributor

@rsc rsc commented Jan 19, 2021

In the case that we are discussing here the only possible user action that follows the CI failure is to run go mod tidy and push the result to the branch.

@Helcaraxan It matters why go mod tidy is complaining and what changes it wants to make. It sounds like you are saying that users shouldn't even bother looking at the changes being made by go mod tidy to go.mod and go.sum, that they should just blindly accept the results. If that's really what you are saying, that's not good advice. In contrast, go mod tidy -diff to show exactly what it would do is very helpful for evaluating whether you want those changes or not.

Part of the reason we rejected gofmt -d setting an exit code is that CI systems should not be breaking due to non-semantic things like code having a stray trailing space. In contrast, go mod tidy is concerned with quite important semantic issues, and having go mod tidy -diff set the same exit code as diff(1) (0 for no changes, 1 for changes) is fine with me. Then if CI systems use go mod tidy -diff as the condition, you get both the exit code and information about what is different. That's exactly what users should want.

To the extent that the arguments for go mod tidy -check seem to encourage a mindset of "go.sum and go.mod are a black box that no one understands and you just run go mod tidy a bunch and pay no attention to exactly what is in those files", I am strongly opposed to that.

@Helcaraxan
Copy link
Contributor

@Helcaraxan Helcaraxan commented Jan 20, 2021

@Helcaraxan It matters why go mod tidy is complaining and what changes it wants to make. It sounds like you are saying that users shouldn't even bother looking at the changes being made by go mod tidy to go.mod and go.sum, that they should just blindly accept the results. If that's really what you are saying, that's not good advice. In contrast, go mod tidy -diff to show exactly what it would do is very helpful for evaluating whether you want those changes or not.

@rsc I fully agree on that point. My intent was and is not to say that people should ignore the changes induced by running go mod tidy, on the contrary. I am sorry if that's how my phrasing has been interpreted. The core of my argument is about the manner in which those changes are being presented to users. And I'm arguing there that presenting a raw diff will, in my opinion and experience, have either a neutral or even a negative effect in the longer run.

If our goal is to make developers understand the induced changes then they need more than a diff. Hence my earlier suggestion about an alternative (but more complex) behaviour for the -diff flag.

The one thing where I could see the value add of go mod tidy -diff is if its not a literal diff but rather a summarised report over what changed and why.

The behaviour of go mod tidy is unintuitive for most users as it goes beyond managing direct or indirect dependencies and it considers the entire dependency graph before applying MVS, not just the build list. This is something that comes up more often than not in my day-to-day experience at my place of work, on issues on OSS projects and on the Go Slack as an element that is not understood at all. With the exception being the very few who have been actively working on Go modules itself or related tooling over the past year(s).

As a result the current pragmatic realisation is that the average, or even more advanced, Go developer is not (yet) likely to understand the why of the changes just based on a raw diff. Simply presenting a diff will de facto result in people continuing to consider modules, the behaviour of go mod tidy as well as the content of go.sum (and to a lesser degree go.mod) as black boxes. It actually can disengage developers from learning to understand the modules system better as their default experience would be to be faced by a wall of text with no clue on how to make sense of it.

Put differently: Getting a raw diff is already possible and easy with existing tooling (go mod tidy && git diff --exit-code or equivalent), I'd rather have an actual go mod tidy -diff aspire to give higher level information than an external diff tool given that it's part of the language's own toolchain. If the choice is between it outputting a raw diff or not implementing any check/diff feature at all I'd probably vote for the latter.

@miquella
Copy link

@miquella miquella commented Jan 20, 2021

I wanted to offer an observation in hopes that it may be helpful to this discussion. Changes to the go.mod file are reasonably well understood, as they're usually in direct response to adding or updating a module. However, this isn't always true of go.sum changes.

Occasionally, changes to .go files in our code base have caused the go.sum file to become untidy (without changes to the go.mod file). Although I didn't investigate at the time, I would guess this is because we started relying on a new package that we hadn't previously that already existed somewhere in the module graph.

Furthermore, while valuable, the data in go.sum is difficult to verify or update directly. So developers typically just run go mod tidy to update go.sum to be whatever it needs to be. Knowing that it's out of date is very useful, but I would argue that the contents of the diff matter less (at least in the common case).

So, I would argue that there is tremendous value in showing the diff of the go.mod file. But perhaps there is less value in showing the diff of the go.sum file, although it should probably still be indicated when it is out of date.

I suppose this begs the question: when go mod tidy is run, will it change the digest of a module without complaining? (i.e. if a particular version already existed in go.sum, but when calculated locally it was different, would it complain rather than updating it?)


Aside: our CI system currently has a 64 line Bash function to flag untidy modules that would largely be supplanted by go mod tidy -diff (coupled with defaulting to -mod=readonly), so this seems very useful. A good portion of that function is ensuring that the diff gets output so we know what got missed.

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Jan 20, 2021

My 2¢: go mod tidy -diff should print a diff and exit non-zero if there are any.

I don't think have separate -check and -diff flags is necessary unless the output is qualitatively different. If the user just wants the exit code, the output can be piped to /dev/null.

If our goal is to make developers understand the induced changes then they need more than a diff. Hence my earlier suggestion about an alternative (but more complex) behaviour for the -diff flag.

@Helcaraxan This is interesting, but there may be a lot here. Some possible changes go mod tidy can make:

  • Formatting, sorting, removing duplicates, and other non-semantic changes in go.mod and go.sum
  • Add go directive
  • Add missing require directives
  • Remove unnecessary require directives
  • Update inconsistent require directives (when the version actually used is higher)
  • Canonicalize versions (replace versions like devbranch with a pseudo-version corresponding to that commit)
  • Add missing sums in go.sum
  • Delete unused sums in go.sum

Describing all those changes in a friendly way may be more in the wheelhouse of gorelease. There is a bit of overlap here though.

I suppose this begs the question: when go mod tidy is run, will it change the digest of a module without complaining? (i.e. if a particular version already existed in go.sum, but when calculated locally it was different, would it complain rather than updating it?)

@miquella go mod tidy (and all other module commands) reports a security error if the locally calculated sum of a downloaded module is different than an existing sum in go.sum. Usually that means a semver tag was moved to a different commit, but it could also mean the module code was tampered with.

@leighmcculloch
Copy link
Contributor

@leighmcculloch leighmcculloch commented Jan 20, 2021

@rsc If we go with -diff would go mod tidy still update the files as go mod tidy does today? Or would it not?

I think my question got missed. Is the intention with -diff that it changes status code and outputs a diff in addition to mutating the files, or when used would it not mutate the files?

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Jan 20, 2021

I think my question got missed. Is the intention with -diff that it changes status code and outputs a diff in addition to mutating the files, or when used would it not mutate the files?

I think the intent is for -diff (or -check) not to mutate files. It would be analogous to -mod=readonly in other commands: fail if any changes are needed.

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 20, 2021

That suggests an alternate spelling: go mod diff -tidy, with a corresponding go mod diff that could report only needed additions and cleanups (like go list -mod=mod -test -deps all).

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 20, 2021

(To be clear, I have no particular preference between go mod tidy -diff and go mod diff -tidy.)

@Helcaraxan
Copy link
Contributor

@Helcaraxan Helcaraxan commented Jan 21, 2021

@Helcaraxan This is interesting, but there may be a lot here. Some possible changes go mod tidy can make:

  • Formatting, sorting, removing duplicates, and other non-semantic changes in go.mod and go.sum
  • Add go directive
  • Add missing require directives
  • Remove unnecessary require directives
  • Update inconsistent require directives (when the version actually used is higher)
  • Canonicalize versions (replace versions like devbranch with a pseudo-version corresponding to that commit)
  • Add missing sums in go.sum
  • Delete unused sums in go.sum

Describing all those changes in a friendly way may be more in the wheelhouse of gorelease. There is a bit of overlap here though.

@jayconrod agreed that there's quite a lot of different things that can happen for various reasons and it would go beyond the current request of having a simple -check command.

I am a bit hesitant about considering this part of gorelease. Such diff explanations have no particular link with releasing as they are part of a general Go-based workflow. Hence why I'd argue that putting it in the go command makes sense. It boils down to adding user-visible annotations to logic that the toolchain already has.

This is also another reason why I'd continue to argue against the -diff proposal. If we implement that one now than what would it look like to add this more complex diff-behaviour? Do we add another flag that has exactly the same semantics as the -diff one but with just a different output (that seems confusing)? Do we consider changing the output of -diff (which would be a breaking change)? The best option I can think of is to use something akin to the -x flag on other commands that increases output verbosity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet