build: plan9-amd64-9front builder is unhappy #19388
Comments
bradfitz
added
Builders
|
David, any update here? |
|
David, please stop this builder. It is still red. Unless there is a plan to work on it and make it not red. But I don't believe there is such a plan, so it's not doing any good keeping it. |
|
The plan9-amd64-9front builder is currently failing because one of the runtime/trace test is running out of memory (see build log). I've haven't fixed it yet because I spent my time on other issues, but I see no reason why this issue couldn't be fixed as well. So, my plan would be investigate and fix this issue. I'd prefer to keep the plan9-amd64-9front builder for now. I'm ready to convert it to the buildlet if you provide me the key. Thanks. |
|
Okay, I sent you a host key. |
|
9front builder is now failing with: If you update your buildlet to get golang/build@a7e875c then you can set your own GOROOT_BOOTSTRAP without me having to configure it. |
|
(and then let me know and I'll clear the failed builds from the dashboard) |
|
This is what I did, but it didn't work. I've looked a bit and this is what happened:
On Plan 9, the environment is shared with the parent process. Since the GOROOT_BOOTSTRAP is properly set only in handleExec, the buildlet will override GOROOT_BOOTSTRAP with the default value if handleExec hasn't been called. I think we should not set GOROOT_BOOTSTRAP to its default value if GOROOT_BOOTSTRAP has already been set and contains an existing directory. I've submitted CL 53475. |
I don't like that approach. The whole point of the new-style builders is to have a lot more centralized control over our many builders, rather than have to sysadmin each one separately. I made the conciliatory golang/build@a7e875c change to give builder operators (like you) at least some control of the environment when it's omitted from the central control's config, but if there's a value in dashboard/builders.go, it must override. And if there's nothing specified at all, we need to keep using $WORKDIR/go1.4 as the default, as that's what we've always done before and I don't want to risk breaking any other builders right now. (especially as I'm about to go on leave). If there's a problem with golang/build@a7e875c then let's understand and fix it. The intent of that change was that we use your system's $GOROOT_BOOTSTRAP if the server's explicit or implicit value doesn't exist. Why didn't my CL work? |
Oh, that's why!? Plan 9 is different from every other operating system in this regard? Should Go's os/exec.Cmd.Start() use a rfork flag or something that makes it get its own environment? Go programs are supposed to behave consistently and portably. Sigh. I'll consider a workaround. |
|
This is what happens: The buildlet overrides the GOROOT_BOOTSTRAP environment variable with the default value (in the initGorootBootstrap function). We lose the initial value of GOROOT_BOOTSTRAP because the environment is shared with the parent on Plan 9. If I recall correctly, we chose this behavior so the different Go processes could share the same environment, as it it expected between threads on Unix (for example, using For example, the rc shell has the same behavior: |
|
As a simple workaround, on my side, without modifying the buildlet code, is to set |
|
Perfect. Do that. Or if you prefer, send a change to add |
|
All three plan9 builders are halfway okay lately. The arm one is pretty red, but not 100%. Closing. |
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>
@0intro, let's fix the plan9-amd64-9front builder or kill it.
Its column of red is distracting.
The text was updated successfully, but these errors were encountered: