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: add misc-compile TryBots for x/ repos #58163

Closed
bcmills opened this issue Jan 31, 2023 · 17 comments
Closed

x/build: add misc-compile TryBots for x/ repos #58163

bcmills opened this issue Jan 31, 2023 · 17 comments
Assignees
@mknyszek
Labels
Builders x/build issues (builders, bots, dashboards) FeatureRequest Issues asking for a new feature that does not need a proposal. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. new-builder
Milestone
Unreleased

Comments

@bcmills
Copy link
Contributor

bcmills commented Jan 31, 2023

Various recent changes in the x/tools have broken the build for various second-class ports (aix, plan9, solaris) without being detected in TryBot runs.

Given that tests can be cross-compiled using go test -c (albeit currently a bit awkwardly because of #58162), we should enable the misc-compile TryBots for the x repos for all platforms that support internal linking.

(The platforms that require external linking do not currently have misc-compile builders at all — that's #25963.)
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Jan 31, 2023
@gopherbot gopherbot added this to the Unreleased milestone Jan 31, 2023
@bcmills bcmills modified the milestones: Unreleased, Backlog Jan 31, 2023
@bcmills
Copy link
Contributor Author

bcmills commented Jan 31, 2023

(CC @golang/release)

@dmitshur dmitshur added FeatureRequest Issues asking for a new feature that does not need a proposal. new-builder labels Jan 31, 2023
@mknyszek mknyszek self-assigned this Jan 31, 2023
@mknyszek mknyszek added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 31, 2023
@mknyszek
Copy link
Contributor

Since I'm currently on rotation I'll try to get to this this week, but no promises since I'm working on the release.

@mknyszek
Copy link
Contributor

Got ready for the release early. Looking at this now.

The logic around misc-compile builders is unfortunately kind of hairy. There's every indication that it should be already running for x/ repo trybots (albeit failing by trying to run buildall.bash), but there's basically a deeply buried exception for any builder that uses buildall.bash. Sigh.

I think I figured out how to fix that (just get rid of the exception), then the coordinator just needs to notice the CompileOnly flag on the builder and do the right thing. There's one remaining question after that and it's how to plumb the build targets through. Currently buildall.bash just has it installed as one of its arguments, but we can't do that for go test -c. We need to invoke it multiple times with a different GOOS and GOARCH. I guess that just means another field to the builder configuration?

@dmitshur
Copy link
Contributor

dmitshur commented Jan 31, 2023 •
edited
Loading

The logic around misc-compile builders is unfortunately kind of hairy. There's every indication that it should be already running for x/ repo trybots (albeit failing by trying to run buildall.bash), but there's basically a deeply buried exception for any builder that uses buildall.bash.

Yep.

I think I figured out how to fix that (just get rid of the exception), then the coordinator just needs to notice the CompileOnly flag on the builder and do the right thing.

Yeah, in runSubrepoTests.

There's one remaining question after that and it's how to plumb the build targets through. [...] I guess that just means another field to the builder configuration?

This question really comes up because misc-compile trybots are the only builder types that try to multiple GOOS/GOARCH builds in one. In other ways, BuildConfig is always one test execution of an environment, so its env []string (one environment) field is enough. But here, you really want multiple environments. To illustrate this point—consider that if the misc-compile x repo trybots were to handle one GOOS/GOARCH port, it'd be sufficient to just set GOOS/GOARCH env vars in the BuildConfig.env field.

To keep the multiple-builds-per-one-BuildConfig property in the x repo CompileOnly trybots, I think adding another field to the builder configuration is reasonable.

Then runSubrepoTests can notice it in addition to CompileOnly and perform multiple GOOS/GOARCH compiles in one. You could technically avoid new fields by instead using a special env var, for example, GO_TEST_COMPILE_ONLY_PORTS=linux/ppc64,linux/ppc64le,aix/ppc64 for runSubrepoTests to interpret, but I doubt that's better than a new field since it still needs to be handled specially.

@prattmic
Copy link
Member

prattmic commented Jan 31, 2023 •
edited
Loading

There's one remaining question after that and it's how to plumb the build targets through. Currently buildall.bash just has it installed as one of its arguments, but we can't do that for go test -c.

You also need extra work to make sure any nested modules are built. e.g., ./... or golang.org/x/tools/... won't build golang.org/x/tools/gopls because it is a nested module. You either need to run go test inside each module, or create a workspace including all nested modules (go work use -r .).

Edit: the testRuns part of runSubrepoTests mostly handles this, provided it gets hooked up properly.

@mknyszek
Copy link
Contributor

I think everything is hooked up properly, except go test -c can't handle multiple packages. #15513 actually totally solves this by allowing for go test -c -o /dev/null but although it's been accepted, it hasn't been implemented. :(

Would it be sufficient to just say go build -o /dev/null for now, since that should just work? It doesn't test that test packages compile, which is unfortunate, but the workaround is considerably more complex, since every package needs to be manually listed with a separate go test -c invocation. (I can make this work if it's critical, but it adds more complexity to the coordinator.)

@mknyszek
Copy link
Contributor

Also @dmitshur, as discussed offline, I did actually break up the misc-compile builders as well. Still not totally sure it's desirable because of the extra VMs started (although they're each shorter-lived than before), but it does make the code a little easier to reason about.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/463777 mentions this issue: dashboard: split misc-compile builders and support subrepo builds

@mknyszek
Copy link
Contributor

mknyszek commented Feb 1, 2023

@prattmic pointed out to me that go test -c alone might not be sufficient either, because it'll miss packages that don't have tests. go test kind of has that problem in general.

As a workaround to the fact that go test -c only works with a single package, I also considered go test -exec where we pass some dummy script that never actually invokes the binary.

@mknyszek
Copy link
Contributor

mknyszek commented Feb 1, 2023

@prattmic points out I can just pass -exec /bin/true. I think that actually fully works around the go test -c issue.

@mknyszek
Copy link
Contributor

mknyszek commented Feb 1, 2023

OK actually, it looks like go test -exec /bin/true is sufficient. Even packages without tests are built and vetted.

@dmitshur

This comment was marked as resolved.

Sign in to view
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/464955 mentions this issue: dashboard: make misc-compile builders run make.bash

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/464957 mentions this issue: dashboard: add test post-submit misc-compile builders for subrepos

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/464956 mentions this issue: dashboard: remove StopAfterMake from misc-compile builders

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/464958 mentions this issue: dashboard: make post-submit subrepo misc-compile builders into trybots

gopherbot pushed a commit to golang/build that referenced this issue Feb 3, 2023
@mknyszek
dashboard: split misc-compile builders
1f0362e 
This change splits up the misc-compile builders to only build for one
platform each. This is the first step in a sequence of CLs to simplify
misc-compile builders by eliminating the !SplitMakeRun paths, of which
misc-compile builders are the only ones.

The end result will be the support of misc-compile builders for
subrepos.

Splitting up misc-compile builders has the potential to increase VM
demand, but each build also takes less time to run. The main way is
could increase overall VM resource usage is the overhead of setting up
and tearing down a VM. The other downside is more visual noise, though
this is fairly minor.

Note that this change also passes GOOS, GOARCH, and GOARM environment
variables to buildall.sh. The goal is to eventually remove buildall.sh
by just doing make.bash. (Right now buildall.sh will essentially ignore
those variables and just set them itself.)

For golang/go#58163.

Change-Id: Ia48f6a16d080e9e078b2959dea3b68fe43def8ec
Reviewed-on: https://go-review.googlesource.com/c/build/+/463777
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Michael Pratt <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Run-TryBot: Michael Knyszek <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
gopherbot pushed a commit to golang/build that referenced this issue Feb 3, 2023
@mknyszek
dashboard: make misc-compile builders run make.bash
5fd7435 
This change removes misc-compile builders' dependence on buildall.bash
and switches to make.bash instead, since they now each build for exactly
one platform.

This change then also removes SplitMakeRun and any paths derived from
that in order to simplify the code.

For golang/go#58163.

Change-Id: I16704cc553c0f6e2c6c34994999693b6d44adb88
Reviewed-on: https://go-review.googlesource.com/c/build/+/464955
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Michael Knyszek <[email protected]>
gopherbot pushed a commit to golang/build that referenced this issue Feb 3, 2023
@mknyszek
dashboard: remove StopAfterMake from misc-compile builders
0962551 
The previous change added a StopAfterMake requirement so that the
misc-compile builders would only ever run make.bash. However, this
is a fairly big hammer as it skips everything after make.bash. Coming
up, we're going to want to runSubrepoTests.

However, we can't call into runTests because of golang/go#58297, so we
need a special exception for that. This change thus also adds the notion
of IsCrossCompileOnly defined by whether the host arch doesn't line up
with the target arch, and adds a test to make sure this only applies to
the misc-compile builders.

This change also removes some hard-coding of the misc-compile prefix in
favor of this new IsCrossCompileOnly notion.

For golang/go#58163.

Change-Id: I40ce91e13b45e8bbbb607aedd302f0ec0fbd608a
Reviewed-on: https://go-review.googlesource.com/c/build/+/464956
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Run-TryBot: Michael Knyszek <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
@dmitshur dmitshur moved this to In Progress in Go Release Feb 3, 2023
gopherbot pushed a commit to golang/build that referenced this issue Feb 3, 2023
@mknyszek
dashboard: add test post-submit misc-compile builders for subrepos
a695691 
This change adds experimental post-submit misc-compile builders for
subrepos only.

For golang/go#58163.

Change-Id: I05e78a3aeefb62669afc364ae04c67d358af80c3
Reviewed-on: https://go-review.googlesource.com/c/build/+/464957
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Michael Knyszek <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Release Feb 3, 2023
@dmitshur dmitshur mentioned this issue Jun 6, 2023
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/513755 mentions this issue: src/buildall.bash: mention no longer being used by Go build system

gopherbot pushed a commit that referenced this issue Jul 27, 2023
@dmitshur @gopherbot
src/buildall.bash: mention no longer being used by Go build system
77863e4 
The buildall.bash script was initially added in 2015 (in CL 9438),
documented as used in the implementation of the new compile-only
builders at the time.

That description was updated as the builder implementation changed
from "linux-amd64-compilesmoke" to "all-compile" and most recently
to "misc-compile", which it still mentions today.

The build system stopped using it in CL 464955 and there are no plans
to use it again in the future, so update the description so that it's
not misleading. Notably, adding additional checks to this script does
not mean they will be caught by builders.

Updates #31916.
Updates #58163.

Change-Id: I17558b1c150a3ad95105de14511c51791287991b
Reviewed-on: https://go-review.googlesource.com/c/go/+/513755
Reviewed-by: Dmitri Shuralyov <[email protected]>
Run-TryBot: Dmitri Shuralyov <[email protected]>
Auto-Submit: Dmitri Shuralyov <[email protected]>
Run-TryBot: Dmitri Shuralyov <[email protected]>
Reviewed-by: Michael Knyszek <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
@bcmills bcmills mentioned this issue Aug 10, 2023
Closed
@golang golang locked and limited conversation to collaborators Jul 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mknyszek mknyszek

Labels
Builders x/build issues (builders, bots, dashboards) FeatureRequest Issues asking for a new feature that does not need a proposal. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. new-builder
Projects
Go Release
Archived in project
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@mknyszek @prattmic @dmitshur @bcmills @gopherbot