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

x/build: TryBots should check go.mod files are tidy #31640

Open
jayconrod opened this issue Apr 23, 2019 · 5 comments
Open

x/build: TryBots should check go.mod files are tidy #31640

jayconrod opened this issue Apr 23, 2019 · 5 comments
Labels
Builders x/build issues (builders, bots, dashboards) modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@jayconrod
Copy link
Contributor

CL 172972 added a dependency on golang.org/x/sync in golang.org/x/tools. The go.mod and go.sum files were not updated.

There's nothing wrong with this dependency, but it should have been more noticeable during code review. If unexpected requirement were added to go.mod, human reviewers would see it.

TryBots should report an error if go.mod and go.sum change with go mod tidy. This will make sure dependency changes are explicit, and it will keep builds deterministic.

@jayconrod jayconrod added Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. modules labels Apr 23, 2019
@jayconrod jayconrod added this to the Unplanned milestone Apr 23, 2019
@bradfitz
Copy link
Contributor

Is there a flag to go mod tidy that'll let it report whether things are tidy without doing any network requests?

Because we're increasingly blocking outbound network access on builders.

But we do allow GOPROXY access, which we set, so if it needs that, I suppose that's okay.

@jayconrod
Copy link
Contributor Author

jayconrod commented Apr 23, 2019

There's not a simple way to do that, but it's a common feature request we should implement. I'm sure there is an issue filed somewhere.

go list -mod=readonly -test ./... would probably be good enough to check that we don't need to add new modules. Setting GOPROXY=off disables most requests, but the ?go-get=1 requests for resolving import paths don't go through the proxy, so those would still happen for unknown modules. Edit: resolving import paths actually does go through GOPROXY.

@bradfitz
Copy link
Contributor

There's not a simple way to do that, but it's a common feature request we should implement. I'm sure there is an issue filed somewhere.

If that's on the way, I'd rather wait for that.

Cross-reference the two bugs when you find it?

@jayconrod
Copy link
Contributor Author

#27005 would add a -check flag. go mod tidy would exit non-zero if changes are needed.

I don't think we'll have time to implement it before the freeze though.

@nmiyake
Copy link
Contributor

nmiyake commented May 23, 2019

Note that go list -mod=readonly -test ./... will only check that you don't need to add new modules given the build configuration of the environment that ran the command -- it will not flag any new modules that may be required when using different build tags (OS/arch/etc.). I believe that mod tidy -check (or an equivalent) is the only way to definitively guarantee that the go.mod and go.sum are in the proper state for all possible consumers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builders x/build issues (builders, bots, dashboards) modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants