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.

Sign up for GitHub

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

x/build/cmd/golangbuild: run subrepo tests from outside their repositories #34352

Closed
bcmills opened this issue Sep 17, 2019 · 6 comments
Closed

x/build/cmd/golangbuild: run subrepo tests from outside their repositories #34352

bcmills opened this issue Sep 17, 2019 · 6 comments
Assignees
@dmitshur
Labels
Builders x/build issues (builders, bots, dashboards) FeatureRequest modules NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Unreleased

Comments

@bcmills
Copy link
Contributor

bcmills commented Sep 17, 2019

(This is essentially the golang.org/x/[…] version of #30316.)

go.mod files in testdata directories cut off the directory from the rest of the module (see #27852). As a result, the corresponding tests are likely to fail.

Unfortunately, at least one of the golang.org/x repos already includes such files: I am aware of some in golang.org/x/tools/go/packages/testdata (CC @heschik, @matloob), and there may be others as well.

Rather than expecting all Go developers to remember to avoid adding extraneous go.mod files, we should configure builders and TryBots to run the tests for golang.org/x modules outside the module, either in addition to or instead of running them from within a cloned repository.

(The simplest alternative I am aware of is to construct a symlink overlay within a temporary directory, as is done in various misc/cgo tests.)

CC @dmitshur @toothrot @bradfitz
@gopherbot gopherbot added this to the Unreleased milestone Sep 17, 2019
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Sep 17, 2019
@bcmills bcmills added Testing An issue that has been verified to require only test changes, not just a test failure. and removed Builders x/build issues (builders, bots, dashboards) labels Sep 17, 2019
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Sep 17, 2019
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 17, 2019
@bcmills bcmills mentioned this issue Sep 17, 2019
Closed
@gopherbot
Copy link

Change https://golang.org/cl/196258 mentions this issue: go/packages: remove go.mod files

gopherbot pushed a commit to golang/tools that referenced this issue Sep 18, 2019
@heschi
go/packages: remove go.mod files
68f92b6 
Files named go.mod define a module boundary and punch a hole in the
repository when the module is fetched with go get. We had a couple in
testdata. Get rid of them.

In one case the changes I made to produce a module cache in packagestest
were enough. In the other, the test needs multiple minor/patch versions
of the same module, which we have no provision for. Rename them to
"definitelynot_go.mod" in the repo and fix it up in the test.

Updates golang/go#34352

Change-Id: I284578b3aebb0f1fec3ddb4bef0df24f050d0636
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196258
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
clintjedwards pushed a commit to clintjedwards/tools that referenced this issue Sep 19, 2019
@heschi @clintjedwards
go/packages: remove go.mod files
472ad66 
Files named go.mod define a module boundary and punch a hole in the
repository when the module is fetched with go get. We had a couple in
testdata. Get rid of them.

In one case the changes I made to produce a module cache in packagestest
were enough. In the other, the test needs multiple minor/patch versions
of the same module, which we have no provision for. Rename them to
"definitelynot_go.mod" in the repo and fix it up in the test.

Updates golang/go#34352

Change-Id: I284578b3aebb0f1fec3ddb4bef0df24f050d0636
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196258
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@dmitshur
Copy link
Contributor

/cc @ianthehat FYI. This is the issue I mentioned to you in person just now.

@dmitshur dmitshur changed the title x/build: run subrepo tests from outside their repositories x/build/cmd/coordinator: run subrepo tests from outside their repositories Oct 11, 2021
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. FeatureRequest and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 31, 2022
@dmitshur dmitshur mentioned this issue Jun 7, 2023
Closed
@dmitshur dmitshur self-assigned this Jun 7, 2023
@dmitshur
Copy link
Contributor

This is implemented in the new build system (LUCI); see #60666 (comment). At this point I'm confident we won't go back and implement this in x/build/cmd/coordinator, so closing as wontfix.

@dmitshur dmitshur closed this as not planned Won't fix, can't repro, duplicate, stale Jul 25, 2023
@bcmills bcmills mentioned this issue Dec 14, 2023
Closed
@bcmills
Copy link
Contributor Author

bcmills commented Jan 24, 2024 •
edited

This is implemented in the new build system (LUCI)

The current implementation in LUCI does not appear to have high enough fidelity to catch the sort of problems that motivated this issue. For example, #65258 is not currently caught by the LUCI builders for the x/telemetry subrepo.

Perhaps the tests are currently run from outside their module, but without cutting off the module contents at module boundaries?

At any rate: reopening as “not actually fixed on LUCI”.

@bcmills bcmills reopened this Jan 24, 2024
@bcmills bcmills changed the title x/build/cmd/coordinator: run subrepo tests from outside their repositories x/build: run subrepo tests from outside their repositories Jan 24, 2024
@bcmills bcmills mentioned this issue Jan 24, 2024
Open
@dmitshur
Copy link
Contributor

dmitshur commented Jan 24, 2024 •
edited

The reason #65258 is not currently caught by the LUCI builders is because the current implementation requires there not to be local replace directives, and x/telemetry is one of the 3 repos that does, and their owners have decided to opt out of this behavior. See here.

If the local replace directive is removed and x/telemetry owners would like this check to apply, it's a matter of removing "telemetry" from keepNestedModsInsideRepo, but to my knowledge they had no such plans when I last asked about 6 months ago.

@dmitshur dmitshur mentioned this issue Jan 24, 2024
Open
@dmitshur dmitshur changed the title x/build: run subrepo tests from outside their repositories x/build/cmd/golangbuild: run subrepo tests from outside their repositories Jan 24, 2024
@dmitshur
Copy link
Contributor

This feature was implemented in LUCI and is working for the repos that haven't opted out.
I filed #65267 to track a way of improving it that I'm aware of.

Retitling for LUCI, and closing as fixed via crrev.com/c/4598622 (and crrev.com/c/4651891).
Please file a new issue if something else comes up, and refer to this one if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dmitshur dmitshur

Labels
Builders x/build issues (builders, bots, dashboards) FeatureRequest modules NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
Go Release
Status: Done
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@dmitshur @bcmills @gopherbot