x/build: make GOROOT read-only before running tests #30316

bcmills · 2019-02-19T20:28:38Z

When #28387 is fixed, we should make sure that it doesn't regress.

Can we configure the builders to make GOROOT read-only before they invoke run.bash, and perhaps restore write permissions after the tests have finished?

(CC @bradfitz @dmitshur)

The text was updated successfully, but these errors were encountered:

dmitshur · 2019-02-19T20:40:29Z

Is it better to catch regressions to #28387 by making builders have a read-only GOROOT and hope that tests fail if they try to write, or by having a writeable GOROOT and try to verify (after tests have finished running) that GOROOT hasn't been modified?

I'm not sure if it's viable to do the latter, but I wanted to ask so that we consider it.

bradfitz · 2019-02-19T20:44:26Z

and perhaps restore write permissions after the tests have finished?

After the tests have finished we destroy the VM, so not point restoring write access moments before death.

bcmills · 2019-02-19T20:59:03Z

@dmitshur, we could do both, but it is more important to run with a read-only GOROOT and check for failing tests: tests that leave changes behind are easy for developers to detect (via git status), so they slip by much less frequently than tests that make and then unmake ephemeral changes.

The latter category is still harmful, since they will fail when users run go test all (a command intended to become more useful in module mode) with a distro-packaged copy of the Go toolchain.

Ephemeral changes can also interfere with proper cache invalidation, so we should avoid them even if GOROOT is writable.

bcmills · 2019-02-19T21:00:57Z

(It occurs to me, though, that we already have a “files opened” check in the cache subsystem. Probably we should check for — and reject — file writes in GOROOT explicitly there.)

bradfitz · 2019-02-21T16:23:10Z

The buildlet already has a recursive file list endpoint, so the build system could also just look at all files before & after a test run to see if there are any differences. This is what x/build/cmd/gomote does to decide how to incrementally push files from developer machines to buildlets.

That might be faster in that it's only doing read operations and not writes.

bcmills · 2019-02-21T17:32:25Z

As noted above, merely looking at the files before and after the test run is not sufficient: we don't want tests to make ephemeral changes, even if they back them out before exiting.

bradfitz · 2019-02-21T17:50:01Z

Ah, sorry, missed that comment.

gopherbot · 2019-02-22T20:17:52Z

Change https://golang.org/cl/163477 mentions this issue: cmd/dist: make GOROOT unwritable on builders

Updates #30316 Change-Id: I57e489f6bbe4a3b39c907dabe5ac41fb9939cdb4 Reviewed-on: https://go-review.googlesource.com/c/go/+/163477 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>

gopherbot · 2019-08-08T23:20:28Z

Change https://golang.org/cl/189537 mentions this issue: cmd/release: create release after make.bash and before all.bash

Lekensteyn · 2019-08-13T16:06:22Z

At least the contents of go1.13beta1.linux-amd64.tar.gz tarball from https://golang.org/dl/ is read-only, is that an intentional side-effect? I noticed this when trying to use patch to patch some sources in GOROOT.

Since go commit 02d24fc25285, the GOROOT directory has been made read-only, this affects the go1.13beta1 binary tarball as used by Travis CI. See golang/go#30316

bcmills · 2019-08-13T16:18:21Z

@Lekensteyn, see #33537.

Binary releases need to build Go and include binaries such as bin/go, bin/gofmt, and others. Previously, this was accomplished by running all.bash script for some GOOS/GOARCH pairs, and make.bash for others where it wasn't viable to run tests as part of the release process. This change makes the release process more consistent by always packaging the release archive file after running make.bash. We still run all.bash in situations where it was previously run, but we do so after the release file has already been created. This avoids the risk of any changes to GOROOT that may occur as part of all.bash (including changing file permissions to be read-only) being included in the final release file. Add a step to check that files in the buildlet's $WORKDIR/go and $WORKDIR/go/bin directories have expected permissions before creating the release file. Fixes golang/go#33537 Updates golang/go#30316 Change-Id: I7d40716dba656a8aca711377f2995df4880166c5 Reviewed-on: https://go-review.googlesource.com/c/build/+/189537 Reviewed-by: Andrew Bonventre <andybons@golang.org>

Since go commit 02d24fc25285, the GOROOT directory has been made read-only, this affects the go1.13beta1 binary tarball as used by Travis CI. See golang/go#30316

bcmills · 2019-11-11T16:13:18Z

Making GOROOT unwritable currently seems to be ineffective for some (?) tests on some (nearly all‽) of the builders: see #32407 (comment).

gopherbot · 2019-11-11T16:39:27Z

Change https://golang.org/cl/206458 mentions this issue: misc: ensure that test overlay directories are writable

Otherwise, the test cannot create new files in the directory. Updates #32407 Updates #30316 Change-Id: Ief0df94a202be92f57d458d4ab4e4daa9ec189b1 Reviewed-on: https://go-review.googlesource.com/c/go/+/206458 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

bcmills · 2019-11-12T14:39:42Z

After the tests have finished we destroy the VM, so not point restoring write access moments before death.

Is that also true for reverse builders (such as host-linux-arm5spacemonkey)?

bcmills · 2019-11-12T15:14:43Z

The other reason to restore access, of course, is so that we can test this behavior locally without infuriating ourselves.

(I should be able to run GO_BUILDER_NAME=linux-amd64-bcmills ./all.bash and get a reasonable simulation of what an actual builder would run.)

gopherbot · 2019-11-12T15:20:26Z

Change https://golang.org/cl/206757 mentions this issue: cmd/dist: save and restore original permissions in makeGOROOTUnwritable

gopherbot · 2019-11-12T15:23:45Z

Change https://golang.org/cl/206758 mentions this issue: cmd/dist: fail in makeGOROOTUnwritable if running as root

bcmills · 2019-11-12T15:29:04Z

Bingo: all of our linux- TryBots run tests as the root user of the container, so CL 163477 has no effect on them.

(https://storage.googleapis.com/go-build-log/726e41e1/linux-amd64_acf59a73.log)

gopherbot · 2019-11-20T22:15:10Z

Change https://golang.org/cl/208124 mentions this issue: misc/cgo/fortran: avoid writing to $PWD

bcmills · 2019-11-20T22:19:49Z

misc/cgo/testshared aggressively overwrites GOROOT/pkg. That one will need some more work. 😞

The bash script that drives this test needs to know whether the fortran compiler works, but it doesn't actually care about the generated binary. Write that binary to /dev/null. Updates #28387 Updates #30316 Change-Id: I4f86da1aeb939fc205f467511fc69235a6a9af26 Reviewed-on: https://go-review.googlesource.com/c/go/+/208124 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

One of the 'go build' commands executed by this test passed the '-i' flag, which caused the 'go' command to attempt to install transitive standard-library dependencies to GOROOT/pkg/$GOOS_$GOARCH_dynlink. That failed if GOROOT/pkg was not writable (for example, if GOROOT was owned by the root user, but the user running the test was not root). As far as I can tell the '-i' flag is not necessary in this test. Prior to the introduction of the build cache it may have been an optimization, but now that the build cache is required the '-i' flag only adds extra work. Updates #30316 Change-Id: Ib60080a008c1941aa92b5bdd5a194d89fd6202aa Reviewed-on: https://go-review.googlesource.com/c/go/+/208120 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

gopherbot · 2019-11-21T21:38:00Z

Change https://golang.org/cl/208232 mentions this issue: misc/cgo/testshared: make -v output less verbose

Also add a -testwork flag to facilitate debugging the test itself. Three of the tests of this package invoked 'go install -i -buildmode=c-archive' in order to generate an archive as well as multiple C header files. Unfortunately, the behavior of the '-i' flag is inappropriately broad for this use-case: it not only generates the library and header files (as desired), but also attempts to install a number of (unnecessary) archive files for transitive dependencies to GOROOT/pkg/$GOOS_$GOARCH_shared, which may not be writable — for example, if GOROOT is owned by the root user but the test is being run by a non-root user. Instead, for now we generate the header files for transitive dependencies separately by running 'go tool cgo -exportheader'. In the future, we should consider how to improve the ergonomics for generating transitive header files without coupling that to unnecessary library installation. Updates #28387 Updates #30316 Updates #35715 Change-Id: I3d483f84e22058561efe740aa4885fc3f26137b5 Reviewed-on: https://go-review.googlesource.com/c/go/+/208117 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>

The tests in this package invoked 'go install -i -buildmode=c-shared' in order to generate an archive as well as multiple C header files. Unfortunately, the behavior of the '-i' flag is inappropriately broad for this use-case: it not only generates the library and header files (as desired), but also attempts to install a number of (unnecessary) archive files for transitive dependencies to GOROOT/pkg/$GOOS_$GOARCH_testcshared_shared, which may not be writable — for example, if GOROOT is owned by the root user but the test is being run by a non-root user. Instead, for now we generate the header files for transitive dependencies separately by running 'go tool cgo -exportheader'. In the future, we should consider how to improve the ergonomics for generating transitive header files without coupling that to unnecessary library installation. Updates #28387 Updates #30316 Updates #35715 Change-Id: I622426a860828020d98f7040636f374e5c766d28 Reviewed-on: https://go-review.googlesource.com/c/go/+/208119 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>

Previously, 'go test -v' in this directory would result in a massive dump of go command output, because the test plumbed -v to 'build -x'. This change separates them into distinct flags, so that '-v' only implies the display of default 'go' command output. Updates #30316 Change-Id: Ifb125f35ec6a0bebe7e8286e7c546d132fb213df Reviewed-on: https://go-review.googlesource.com/c/go/+/208232 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot · 2019-11-22T21:58:35Z

Change https://golang.org/cl/208482 mentions this issue: misc/cgo/testshared: do not write to GOROOT

gopherbot · 2019-11-22T21:58:37Z

Change https://golang.org/cl/208481 mentions this issue: misc: remove use of relative directories in overlayDir functions

gopherbot · 2019-11-22T22:15:07Z

Change https://golang.org/cl/208483 mentions this issue: misc: log 'ok' from 'go run' tests on success

It turns out that the relative-path support never worked in the first place. It had been masked by the fact that we ~never invoke overlayDir with an absolute path, which caused filepath.Rel to always return an error, and overlayDir to always fall back to absolute paths. Since the absolute paths seem to be working fine (and are simpler), let's stick with those. As far as I can recall, the relative paths were only a space optimization anyway. Updates #28387 Updates #30316 Change-Id: Ie8cd28f3c41ca6497ace2799f4193d7f5dde7a37 Reviewed-on: https://go-review.googlesource.com/c/go/+/208481 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com>

Otherwise, these tests produce no output, which can make the overall output of all.bash a bit tricky to decipher. Updates #30316 Updates #29062 Change-Id: I33b9e070fd28b9f21ece128e9e603a982c08b7cc Reviewed-on: https://go-review.googlesource.com/c/go/+/208483 Run-TryBot: Bryan C. Mills <bcmills@google.com> Reviewed-by: Jay Conrod <jayconrod@google.com>

bcmills · 2019-11-25T17:05:21Z

As of CL 208482, all.bash passes when run under the conditions that trigger a read-only GOROOT.

Specifically, I tested with:

GO_BUILDER_NAME=linux-amd64-bcmills
GO_TEST_SHORT=0
GO_TEST_TIMEOUT_SCALE=5

I believe that the main (go) repo is now ready for a non-root linux-*-longtest builder to trigger read-only mode and prevent regressions.

Instead of installing shared libraries to GOROOT/pkg, clone the necessary files into a new GOROOT and run there. Given that we now have a build cache, ideally we should not need to install into GOROOT/pkg at all, but we can't fix that during the 1.14 code freeze. Updates #28387 Updates #28553 Updates #30316 Change-Id: I83084a8ca29a5dffcd586c7fccc3f172cac57cc6 Reviewed-on: https://go-review.googlesource.com/c/go/+/208482 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com>

gopherbot · 2020-03-17T15:09:34Z

Change https://golang.org/cl/223745 mentions this issue: cmd/go: replace TestCgoDependsOnSyscall with a simpler script test

The existing test attempted to remove '_race' binaries from GOROOT/pkg, which could not only fail if GOROOT is read-only, but also interfere with other tests run in parallel. Updates #30316 Updates #37573 Updates #17751 Change-Id: Id7e2286ab67f8333baf4d52244b7f4476aa93a46 Reviewed-on: https://go-review.googlesource.com/c/go/+/223745 Run-TryBot: Bryan C. Mills <bcmills@google.com> Reviewed-by: Jay Conrod <jayconrod@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>

gopherbot · 2020-03-17T17:11:33Z

Change https://golang.org/cl/223746 mentions this issue: cmd/go: add a "don't care about success" operator to script_test

gopherbot · 2020-03-17T20:16:43Z

Change https://golang.org/cl/223747 mentions this issue: cmd/go: add a missing curly-brace in the 'stale' command format string

The missing brace made the 'stale' command a no-op in the non-error case. Fix the 'short' skip in install_cross_gobin (it was backward) and update it to no longer check staleness of a not-necessarily-stale target and to no longer expect to be able to install into GOROOT/pkg. (This was missed in #30316 because that part of the test was erroneously skipped in non-short mode.) Change-Id: I6a276fec5fa5e5da3fe0daf0c2b5086116ed7c1a Reviewed-on: https://go-review.googlesource.com/c/go/+/223747 Run-TryBot: Bryan C. Mills <bcmills@google.com> Reviewed-by: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>

gopherbot · 2020-03-18T15:54:28Z

Change https://golang.org/cl/223919 mentions this issue: cmd/go: make 'go get' and 'go install' avoid updating mtimes of binaries that are not stale

Use that operator to make test_race_install_cgo agnostic to whether GOROOT/pkg is writable. Updates #37573 Updates #30316 Change-Id: I018c63b3c369209345069f917bbb3a52179e2b58 Reviewed-on: https://go-review.googlesource.com/c/go/+/223746 Reviewed-by: Jay Conrod <jayconrod@google.com>

gopherbot · 2020-03-27T14:50:44Z

Change https://golang.org/cl/225897 mentions this issue: cmd/go/internal/work: disallow testgo binary from installing to GOROOT

Installing to GOROOT makes tests non-parallelizable, since each test depends on the installed contents of GOROOT already being up-to-date and may reasonably assume that those contents do not change over the course of the test. Fixes #37573 Updates #30316 Change-Id: I2afe95ad11347bee3bb7c2d77a657db6d691cf05 Reviewed-on: https://go-review.googlesource.com/c/go/+/225897 Reviewed-by: Michael Matloob <matloob@golang.org>

gopherbot added this to the Unreleased milestone Feb 19, 2019

gopherbot added the Builders x/build issues (builders, bots, dashboards) label Feb 19, 2019

mvdan mentioned this issue Feb 21, 2019

all.bash: leaves $GOROOT/nonexistentpath behind #30338

Closed

bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 28, 2019

bcmills mentioned this issue Sep 17, 2019

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

Closed

bcmills mentioned this issue Oct 21, 2019

misc/cgo/stdio: test persistently broken on linux-arm-arm5spacemonkey builder #32407

Closed

bcmills mentioned this issue Nov 12, 2019

misc/cgo/testgodefs: failing on linux-arm-arm5spacemonkey #35536

Closed

bcmills mentioned this issue Nov 21, 2019

cmd/go: panic when computing Shlib path for a package with no install target #35759

Closed

bcmills mentioned this issue Feb 24, 2020

build: all.bash fails when GOBIN is set via 'go env -w' #37390

Closed

bcmills mentioned this issue Jan 19, 2021

os: spurious TestDirFS failures due to directory mtime skew on Windows #42637

Closed

bcmills mentioned this issue Jul 15, 2021

cmd/go: "read-only file system" with -linkshared even when files are present #47186

Closed

bcmills mentioned this issue Sep 30, 2021

cmd/go: tests fail when GOROOT is stale #48698

Closed

This comment was marked as resolved.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/build: make GOROOT read-only before running tests #30316

x/build: make GOROOT read-only before running tests #30316

bcmills commented Feb 19, 2019

dmitshur commented Feb 19, 2019

bradfitz commented Feb 19, 2019

bcmills commented Feb 19, 2019

bcmills commented Feb 19, 2019

bradfitz commented Feb 21, 2019

bcmills commented Feb 21, 2019

bradfitz commented Feb 21, 2019

gopherbot commented Feb 22, 2019

gopherbot commented Aug 8, 2019

Lekensteyn commented Aug 13, 2019

bcmills commented Aug 13, 2019

bcmills commented Nov 11, 2019

gopherbot commented Nov 11, 2019

bcmills commented Nov 12, 2019

bcmills commented Nov 12, 2019

gopherbot commented Nov 12, 2019

gopherbot commented Nov 12, 2019

bcmills commented Nov 12, 2019

gopherbot commented Nov 20, 2019

bcmills commented Nov 20, 2019

gopherbot commented Nov 21, 2019

gopherbot commented Nov 22, 2019

gopherbot commented Nov 22, 2019

gopherbot commented Nov 22, 2019

bcmills commented Nov 25, 2019 •

edited

Loading

gopherbot commented Mar 17, 2020

gopherbot commented Mar 17, 2020

gopherbot commented Mar 17, 2020

gopherbot commented Mar 18, 2020

gopherbot commented Mar 27, 2020

This comment was marked as resolved.

x/build: make GOROOT read-only before running tests #30316

x/build: make GOROOT read-only before running tests #30316

Comments

bcmills commented Feb 19, 2019

dmitshur commented Feb 19, 2019

bradfitz commented Feb 19, 2019

bcmills commented Feb 19, 2019

bcmills commented Feb 19, 2019

bradfitz commented Feb 21, 2019

bcmills commented Feb 21, 2019

bradfitz commented Feb 21, 2019

gopherbot commented Feb 22, 2019

gopherbot commented Aug 8, 2019

Lekensteyn commented Aug 13, 2019

bcmills commented Aug 13, 2019

bcmills commented Nov 11, 2019

gopherbot commented Nov 11, 2019

bcmills commented Nov 12, 2019

bcmills commented Nov 12, 2019

gopherbot commented Nov 12, 2019

gopherbot commented Nov 12, 2019

bcmills commented Nov 12, 2019

gopherbot commented Nov 20, 2019

bcmills commented Nov 20, 2019

gopherbot commented Nov 21, 2019

gopherbot commented Nov 22, 2019

gopherbot commented Nov 22, 2019

gopherbot commented Nov 22, 2019

bcmills commented Nov 25, 2019 • edited Loading

gopherbot commented Mar 17, 2020

gopherbot commented Mar 17, 2020

gopherbot commented Mar 17, 2020

gopherbot commented Mar 18, 2020

gopherbot commented Mar 27, 2020

This comment was marked as resolved.

bcmills commented Nov 25, 2019 •

edited

Loading