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

std,cmd: add test to ensure that go.mod and vendor are in sync #36852

Closed
dmitshur opened this issue Jan 28, 2020 · 12 comments
Closed

std,cmd: add test to ensure that go.mod and vendor are in sync #36852

dmitshur opened this issue Jan 28, 2020 · 12 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@dmitshur
Copy link
Contributor

We should make it so that trybots catch and report when the src/go.mod and src/vendor directories are not in sync, to prevent them from going out of sync without anyone noticing.

See #36851 for more context.

/cc @golang/osp-team

@dmitshur dmitshur 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. FeatureRequest labels Jan 28, 2020
@dmitshur dmitshur added this to the Unreleased milestone Jan 28, 2020
@bcmills bcmills added the Testing An issue that has been verified to require only test changes, not just a test failure. label Jan 31, 2020
@dmitshur
Copy link
Contributor Author

@bcmills I think this issue is the same as issue #36907 (which was created slightly later), and can be considered resolved via CL 217218. Do you agree?

@dmitshur
Copy link
Contributor Author

Oh, I misread, this is about divergence between go.mod and vendor. That issue was about divergence between two different go.mods.

@dmitshur
Copy link
Contributor Author

dmitshur commented Feb 21, 2020

That said, the test in CL 217218 seems to check for this issue too, does it not?

Edit: It only checks for missing vendored packages, not that the vendored packages are identical.

@bcmills
Copy link
Contributor

bcmills commented Feb 24, 2020

Correct. The new test only checks that vendor/modules.txt is consistent with go.mod: it does not verify that the actual vendored source code is unchanged.

@dmitshur
Copy link
Contributor Author

dmitshur commented May 5, 2020

@bcmills I have some questions about this, since you have more context and might be able to answer more easily.

Do you think this can be implemented by adding a test that does go mod vendor in the two modules and does the equivalent of git diff? Does go mod vendor need internet beyond accessing the module mirror? Do you know of any precedent for doing the "equivalent of git diff" in tests in the standard library? Final question, would it be sufficient to do this check on one or two configurations (e.g., linux/amd64), or can you think of a reason to do it on more/all builder types?

@dmitshur
Copy link
Contributor Author

dmitshur commented May 5, 2020

I've confirmed there isn't an existing test (on master) that catches this issue by making a trivial edit in src/cmd/vendor and running src/all.bash; it reported "ALL TESTS PASSED".

@bcmills
Copy link
Contributor

bcmills commented May 5, 2020

Do you think this can be implemented by adding a test that does go mod vendor in the two modules and does the equivalent of git diff?

Yes.

Does go mod vendor need internet beyond accessing the module mirror?

It should need only the module mirror.

Do you know of any precedent for doing the "equivalent of git diff" in tests in the standard library?

There are a few tests that check that generated source files are clean (such as cmd/go.TestDocsUpToDate), but to my knowledge none that rely on git (or on being able to write to GOROOT).

Another approach might be to set up an overlay of the std module in a temporary directory and run go mod vendor within that overlay, then diff the resulting trees (using diff -r or an equivalent Go function, perhaps implemented in terms of filepath.Walk).

The misc/reboot test illustrates how to make an overlay of GOROOT/src in order to test commands that mutate GOROOT. (Unfortunately, due to the fact that the misc repo does not have a fully-qualified import path in GOPATH mode, that part of the repo contains many duplicate copies of the file overlaydir_test.go that constructs the actual overlay.)

Final question, would it be sufficient to do this check on one or two configurations (e.g., linux/amd64), or can you think of a reason to do it on more/all builder types?

go mod vendor is intended to be platform-independent, so running the test on one or two configurations should suffice.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/255859 mentions this issue: misc/update: add program to update golang.org/x dependencies

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/256357 mentions this issue: cmd/updatestd: add program to maintain standard library modules

@dmitshur
Copy link
Contributor Author

dmitshur commented Sep 24, 2020

Thanks for answering @bcmills. Based on those answers (and our other discussion of this topic), I think we want a normal test that is skipped in -short test mode (so it only runs on longtest builders or when executed locally manually).

As long as it's possible to implement a reasonably fast/efficient test without needing to rely on git, I don't see a reason for the test to be implemented out of tree; that would make it difficult to reproduce the result of builders locally, and it would contribute negatively to #39054. The cmd/internal/moddeps, misc/reboot, and test/winbatch.go tests may be good sources of inspiration. (To avoid needing git, a temporary directory can be used to compute the expected result, and check the tree against that.)

So, I'll retitle this to be less about x/build and builders and more about investigating adding a test in the main repo. Once a test is added, it will be automatically executed by longtest builders (and via SlowBots). (If it turns out to be infeasible to add as a normal test in-tree, we can reconsider.)

@dmitshur dmitshur changed the title x/build: add check to trybot to catch src/go.mod and src/vendor divergence std,cmd: add test to ensure that go.mod and vendor are in sync Sep 24, 2020
@dmitshur dmitshur removed the Builders x/build issues (builders, bots, dashboards) label Sep 24, 2020
@dmitshur
Copy link
Contributor Author

Managing backports can become much more expensive if skew is introduced in a new major Go release. We've been lucky that go.mod and vendor skew hasn't made it into any major Go release so far (although it did happen on tip, e.g., #36851), but I think it's important we have an automated measure so we can be confident it can't happen due to human error.

I'm going to make this a release-blocker for Go 1.16: we should either resolve this issue by adding a test, or if there isn't enough time, we must manually verify that there is no skew introduced in Go 1.16 release candidates and the final release.

CC @golang/release.

@dmitshur dmitshur modified the milestones: Unreleased, Go1.16 Oct 22, 2020
@dmitshur dmitshur self-assigned this Dec 5, 2020
@toothrot toothrot added okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 and removed okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 labels Dec 10, 2020
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/283643 mentions this issue: cmd/internal/moddeps: check content of all modules in GOROOT

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 16, 2021
@golang golang locked and limited conversation to collaborators Jan 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

5 participants
@toothrot @dmitshur @bcmills @gopherbot and others