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/tools: multiple test failures on plan9_arm #38772

Closed
millerresearch opened this issue Apr 30, 2020 · 13 comments
Closed

x/tools: multiple test failures on plan9_arm #38772

millerresearch opened this issue Apr 30, 2020 · 13 comments

Comments

@millerresearch
Copy link

@millerresearch millerresearch commented Apr 30, 2020

The golang.org/x/tools/... subrepo test takes a very long time on plan9_arm, and never passes. A recent result here for examplle shows these failures:

x/tools/go/internal/gcimporter - stack overflow (in go type checker)
x/tools/go/packages - timeout after 10m
x/tools/go/packages/packagestest - Plan 9 doesn't have symlinks
x/tools/internal/imports - timeout after 10m
x/tools/internal/lsp/diff/difftest - Plan 9 'diff' output has unexpected syntax
x/tools/internal/lsp/regtest - hard to interpret this failure: "context deadline exceeded" ?

I will investigate these, but in the meantime can I suggest disabling this subrepo test for plan9_arm in x/build/dashboard/builders.go ? This would free up the slow Raspberry Pi builders to get on with more useful tests.

I don't know the procedure for making changes to the build coordinator. Can I submit a CL and wait for the dashboard to be restarted, or should I make an official request somewhere?

@gopherbot gopherbot added this to the Unreleased milestone Apr 30, 2020
@andybons
Copy link
Member

@andybons andybons commented May 1, 2020

/cc @0intro

I don't know the procedure for making changes to the build coordinator. Can I submit a CL and wait for the dashboard to be restarted, or should I make an official request somewhere?

Yes, you can submit CLs. @cagedmantis @dmitshur @toothrot

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented May 1, 2020

It should be a matter of adjusting the buildsRepo policy function for the builder and providing the motivation in the CL description. Thanks.

@millerresearch
Copy link
Author

@millerresearch millerresearch commented May 6, 2020

This turns out to be a partial duplicate of #32834 - the stack overflow and timeouts are observed in tests which have a huge memory footprint, already skipped in the linux-arm builder. I'll submit a CL to skip them on plan9-arm too.

@gopherbot
Copy link

@gopherbot gopherbot commented May 6, 2020

Change https://golang.org/cl/232478 mentions this issue: internal/testenv: make ExitIfSmallMachine apply to plan9-arm builders

@gopherbot
Copy link

@gopherbot gopherbot commented May 6, 2020

Change https://golang.org/cl/232479 mentions this issue: internal/lsp/diff/difftest: skip TestVerifyUnified on Plan 9

gopherbot pushed a commit to golang/tools that referenced this issue May 7, 2020
Some x/tools tests use too much memory (virtual or real) or other
resources to run on "small" builders. Originally only linux-arm
was classified as small. This change addes plan9-arm to the list,
since it normally runs on Raspberry Pi boards with 1GB of RAM.

Partial workaround for golang/go#38772

Change-Id: I93307af10cccf7b1e26d57b9af914c85cf676d43
Reviewed-on: https://go-review.googlesource.com/c/tools/+/232478
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented May 8, 2020

Change https://golang.org/cl/232802 mentions this issue: go/packages/packagestest: ignore symlink test if OS doesn't have symlinks

gopherbot pushed a commit to golang/tools that referenced this issue May 8, 2020
TestVerifyUnified in internal/lsp/diff/difftest requires specific
behaviour of the 'diff' command which is known to be satisfied by
GNU diff. The plan9 'diff' command has no '-u' option, and the
illumos 'diff -u' produces output in a different format. Checking
specifically for the GNU version in the HasTool function ensures
the expected behaviour, and otherwise causes the test to be skipped.

Updates golang/go#38772

Change-Id: I5493fa8cfc48a112dc0b7356618c62d3ccb0366f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/232479
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented May 15, 2020

Change https://golang.org/cl/234112 mentions this issue: go/packages/packagestest: make Export skip tests involving unsupported filesystem links

@gopherbot
Copy link

@gopherbot gopherbot commented May 18, 2020

Change https://golang.org/cl/234397 mentions this issue: runtime: don't enable notes (=signals) too early in Plan 9

gopherbot pushed a commit that referenced this issue May 18, 2020
The Plan 9 runtime startup was enabling notes (like Unix signals)
before the gsignal stack was allocated. This left a small window
of time where an interrupt (eg by the parent killing a subprocess
quickly after exec) would cause a null pointer dereference in
sigtramp. This would leave the interrupted process suspended in
'broken' state instead of exiting. We've observed this on the
builders, where it can make a test time out waiting for the broken
process to terminate.

Updates #38772

Change-Id: I54584069fd3109595f06c78724c1f6419e028aab
Reviewed-on: https://go-review.googlesource.com/c/go/+/234397
Run-TryBot: David du Colombier <0intro@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David du Colombier <0intro@gmail.com>
@gopherbot
Copy link

@gopherbot gopherbot commented May 19, 2020

Change https://golang.org/cl/234537 mentions this issue: internal/imports: do not skip symlink tests on Windows

gopherbot pushed a commit to golang/tools that referenced this issue May 21, 2020
Windows does actually support symlinks, but older versions of
Windows only support symlinks when running as an administrator.
Newer versions of Windows support symlinks for all users.

Instead of skipping based on GOOS, first try the Symlink operation.
If it succeeds, we can proceed with the test; otherwise, we can try to
write a regular file to determine whether the problem was the symlink
operation itself or the destination path.

For golang/go#38772

Change-Id: Idaa9592011473de7f514b889859e420a84db6d01
Reviewed-on: https://go-review.googlesource.com/c/tools/+/234537
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
@9pi
Copy link

@9pi 9pi commented Jun 5, 2020

The problems with x/tools/internal/lsp/regtest are caused by the unusual semantics of os.Setenv on Plan 9, leading to different tests interfering with each other via unexpectedly shared changes to environment variables, thus a partial duplicate of #25234. I'm about to submit a CL which should deal with this.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 5, 2020

Change https://golang.org/cl/236520 mentions this issue: runtime, syscall: use local cache for Setenv/Getenv in Plan 9

@millerresearch
Copy link
Author

@millerresearch millerresearch commented Jun 19, 2020

Almost everything in this issue has now been dealt with. Outstanding CLs 234112 and 236520 should finish the job. Is it possible to get those merged soon, so we can see all subrepo tests passing on plan9 for release 1.15?

gopherbot pushed a commit that referenced this issue Jun 19, 2020
In os.Getenv and os.Setenv, instead of directly reading and writing the
Plan 9 environment device (which may be shared with other processes),
use a local copy of environment variables cached at the start of
execution. This gives the same semantics for Getenv and Setenv as on
other operating systems which don't share the environment, making it
more likely that Go programs (for example the build tests) will be
portable to Plan 9.

This doesn't preclude writing non-portable Plan 9 Go programs which make
use of the shared environment semantics (for example to have a command
which exports variable definitions to the parent shell). To do this, use
  ioutil.ReadFile("/env/"+key) and
  ioutil.WriteFile("/env/"+key, value, 0666)
in place of os.Getenv(key) and os.Setenv(key, value) respectively.

Note that CL 5599054 previously added env cacheing, citing efficiency
as the reason. However it made the cache write-through, with Setenv
changing the shared environment as well as the cache (so not consistent
with Posix semantics), and Clearenv breaking the sharing of the
environment between the calling thread and other threads (leading to
unpredictable behaviour). Because of these inconsistencies (#8849),
CL 158970045 removed the cacheing again.

This CL restores cacheing but without write-through. The local cache is
initialised at start of execution, manipulated by the standard functions
in syscall/env_unix.go to ensure the same semantics, and exported only
when exec'ing a new program.

Fixes #34971
Fixes #25234
Fixes #19388
Updates #38772

Change-Id: I2dd15516d27414afaf99ea382f0e00be37a570c3
Reviewed-on: https://go-review.googlesource.com/c/go/+/236520
Run-TryBot: David du Colombier <0intro@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Fazlul Shahriar <fshahriar@gmail.com>
Reviewed-by: David du Colombier <0intro@gmail.com>
@bcmills
Copy link
Member

@bcmills bcmills commented Jun 2, 2021

The remaining failure is #46503. Let's track individual test failures separately from here on out, so that we're not chasing so much of a moving target.

@bcmills bcmills closed this Jun 2, 2021
gopherbot pushed a commit to golang/tools that referenced this issue Jun 2, 2021
…d links

Also make Export create all parent directories before all files.
If the files are symlinks to directories, the target directory must
exist before the symlink is created on Windows. Otherwise, the symlink
will be created with the wrong type (and thus broken).

Fixes golang/go#46503
Updates golang/go#38772
Updates golang/go#39183

Change-Id: I17864ed66e0464e0ed1f56c55751b384b8c59484
Reviewed-on: https://go-review.googlesource.com/c/tools/+/234112
Trust: Bryan C. Mills <bcmills@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants