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: create VERSION file for cmd/dist to read #24754

Open
mvdan opened this issue Apr 7, 2018 · 12 comments
Open

x/build: create VERSION file for cmd/dist to read #24754

mvdan opened this issue Apr 7, 2018 · 12 comments
Labels
Builders x/build issues (builders, bots, dashboards) Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Apr 7, 2018

https://build.golang.org/log/89d71a5b0369a691899c3435abc7df3d4abd4074

##### API check
Error running API checker: exit status 1
Go version is "builder host-linux-armel-cross", ignoring -next /home/builder/stage0scratch/workdir/go/api/next.txt
[ a lot of +pkg ... lines ]
exit status 1
2018/04/07 13:27:59 Failed: exit status 1
2018/04/07 13:28:04 FAILED

The fact that the version does not contain devel implies that +pkg lines, i.e. new APIs, are a test failure:

fail = !compareAPI(bw, features, required, optional, exception,
        *allowNew && strings.Contains(runtime.Version(), "devel"))

The fact that no other builder hits this tells me that the version in this ARM builder is wrong; it should contain devel.

@mvdan mvdan added Testing An issue that has been verified to require only test changes, not just a test failure. Builders x/build issues (builders, bots, dashboards) labels Apr 7, 2018
@mvdan mvdan added this to the Go1.11 milestone Apr 7, 2018
@mvdan
Copy link
Member Author

mvdan commented Apr 7, 2018

/cc @zeebo @bradfitz

@bradfitz
Copy link
Contributor

bradfitz commented Apr 9, 2018

Well, this part of the failure log (https://build.golang.org/log/89d71a5b0369a691899c3435abc7df3d4abd4074) is interesting:

warning: changing VERSION from "devel 3e31eb6b84d923adb764fa4af920cec5902eae60" to "builder host-linux-armel-cross"
Building Go toolchain1 using /go1.4.
warning: changing VERSION from "devel 3e31eb6b84d923adb764fa4af920cec5902eae60" to "builder host-linux-armel-cross"
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
warning: changing VERSION from "devel 3e31eb6b84d923adb764fa4af920cec5902eae60" to "builder host-linux-armel-cross"

The build system is explicitly writing out a VERSION file, and then something is changing it. Why?

@rsc added that in b09e2de where he says:

+			// Some builders cross-compile the toolchain on linux-amd64
+			// and then copy the toolchain to the target builder (say, linux-arm)
+			// for use there. But on non-release (devel) branches, the compiler
+			// used on linux-amd64 will be an amd64 binary, and the compiler
+			// shipped to linux-arm will be an arm binary, so they will have different
+			// content IDs (they are binaries for different architectures) and so the
+			// packages compiled by the running-on-amd64 compiler will appear
+			// stale relative to the running-on-arm compiler. Avoid this by setting
+			// the version string to something that doesn't begin with devel.
+			// Then the version string will be used in place of the content ID,
+			// and the packages will look up-to-date.
+			// TODO(rsc): Really the builders could be writing out a better VERSION file instead,
+			// but it is easier to change cmd/dist than to try to make changes to
+			// the builder while Brad is away.
+			if strings.HasPrefix(b, "devel") {
+				if hostType := os.Getenv("META_BUILDLET_HOST_TYPE"); strings.Contains(hostType, "-cross") {
+					fmt.Fprintf(os.Stderr, "warning: changing VERSION from %q to %q\n", b, "builder "+hostType)
+					b = "builder " + hostType
+				}
+			}

So, uh, what should the VERSION file be, @rsc?

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/106118 mentions this issue: dashboard: skip some slow/broken tests on arm5 builder

gopherbot pushed a commit to golang/build that referenced this issue Apr 10, 2018
Updates golang/go#24754

Change-Id: Ie5e94a8fd0b76e6394249a734d8fc1a61650dbad
Reviewed-on: https://go-review.googlesource.com/106118
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
@mvdan
Copy link
Member Author

mvdan commented Apr 13, 2018

@bradfitz the builder is still seeing the same failures. Do we require manual action to apply your dashboard fix above?

@bradfitz
Copy link
Contributor

I haven't deployed it

@mvdan
Copy link
Member Author

mvdan commented Apr 13, 2018

Ah, apologies for the noise - I thought this was somehow automated.

@bradfitz
Copy link
Contributor

bradfitz commented Jun 5, 2018

Ping @rsc. What do you think the VERSION file should contain in these cases?

@ianlancetaylor
Copy link
Contributor

@bradfitz The reason for the change is, essentially, to avoid having the string "devel" appears in the third field of the output of go tool compile -V=full. When the string "devel" appears there, the code in cmd/go/internal/work/buildid.go, function (*Builder).toolID, uses a hash of the compiler binary for the build ID. When "devel" does not appear, (*Builder).toolID just hashes the version string itself. The effect is that for released compilers we get the same build ID regardless of whether we are using a cross-compiler or not.

The output of go tool compile -V=full is generated by cmd/internal/objabi/flag.go based on the value of version in cmd/internal/objabi/zbootstrap.go. That file is generated by mkzbootstrap in cmd/dist/buildruntime.go, where version is set to the result of findgoversion, which is the function you are asking about.

So this all boils down to: findgoversion can return anything that doesn't start with "devel". That will ensure that the build ID of the packages built on the cross-compiler host will be the same as the build ID of the packages on the target, and will thus avoid rebuilding lots of packages on the target.

Other than not starting with "devel" I don't think it really matters what we put in the VERSION file on the builders. It could be more or less anything, since the files don't escape the builders anyhow as far as I know.

@ianlancetaylor ianlancetaylor changed the title cmd/api: failing on linux-arm builder due to non-devel Go version x/build: create VERSION file for cmd/dist to read Jun 29, 2018
@bradfitz
Copy link
Contributor

@ianlancetaylor, one medium-term goal for the builders is to use cmd/go's build+test cache infrastructure across builds, at least for trybots. If we put the git commit hash in the VERSION, doesn't that get mixed in to the action ID / cache keys? Can I just use a static string for all builds, like "golang-x-build"?

@ianlancetaylor
Copy link
Contributor

I don't understand how we can safely use the cache across builds across CLs. If a CL changes the compiler, then we definitely do not want to pick up an cached build result built by an earlier version of the compiler.

@bradfitz
Copy link
Contributor

@ianlancetaylor, I assumed the digest of the compiler/linker/assembler bits were part of the cache. I just don't want some arbitrary string to be part of the digest unnecessarily.

If a given commit only touches a comment in some test file, I want to have an identical compiler/linker/assembler from the previous commit, and then reuse all the built packages & test results.

@ianlancetaylor
Copy link
Contributor

If go tool compile -V=full prints a string that contains the string "devel", then the digest of the compiler will be part of the cache. If it does not, then only the -V=full output will be part of the cache. If there is a VERSION file, then the string "devel" will be printed if and only if the VERSION file contains the string "devel". So we could perhaps use "devel golang-x-build".

At least, that is my understanding.

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) 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

6 participants