x/build: create builder that runs with GOEXPERIMENT set to 'staticlockranking' (once lock ranking change is in) #37937
Comments
cagedmantis
added
the
NeedsInvestigation
|
@danscales Do you think just a post-submit builder (not a part of the default trybot set) is sufficient, or do you think including this builder in trybots would be helpful too? |
|
Yes, I think a post-submit builder will be fine. This static lock ranking will only affect people who do fairly significant changes to the Go runtime, and those folks will hopefully also know when it will be useful to run the static lock ranking checks before checkin (because they changed the way certain runtime locks are used). |
|
I see. For those CLs, it'll be possible to run the new builder via SlowBots by asking for that builder specifically with something like I understand it should test only the main Go repository. Let me know if think it would be helpful to test some/many/all golang.org/x repos as well. |
|
Change https://golang.org/cl/224078 mentions this issue: |
No, there will be no need to run such a builder for any other repo other than the main Go repo. Thanks for figuring out the necessary builder work! I hope to get my first change (222925) submitted today or tomorrow, and then it may be a few more days before I get the full 'static lock ranking' change (207619) submitted. |
|
Sounds good. Thinking about it more, it should be useful to get the builder up and running starting with the latest existing commit on master, and ensure it's passing (or failing, if setting GOEXPERIMENT to a not-yet-supported value causes an explicit failure). Then, when you're ready to land your changes, you can use the builder via SlowBots to confirm everything is working as expected (your changes and the builder). I'll update the CL to remove the DNS. |
|
CL 224078 is deployed now, and the (Source: https://build.golang.org/log/afcc33b62c57fa7eee7a3807a81d081a8c9eff28) @danscales Please feel free to test your change with the new builder via SlowBots when it's ready, and let me know if anything more needs to be adjusted on the side of the builder. /cc @andybons |
|
OK, thank you for doing all this, @dmitshur . Will let you know how it goes with the SlowBots when I'm ready. |
For each experiment that has been enabled in the toolchain, define a build tag with the same name (but prefixed by "goexperiment.") which can be used for compiling alternative files for the experiment. This allows changes for the experiment, like extra struct fields in the runtime, without affecting the base non-experiment code at all. I use this capability in my CL for static lock ranking (https://go-review.googlesource.com/c/go/+/207619), so that static lock ranking can be fully enabled as a GOEXPERIMENT, but there is no overhead in the runtime when the experiment is not enabled. I added a test in cmd/go/testdata/scripts to make sure the build tags are being defined properly. In order to implement the test, I needed to provide environment variable GOEXPSTRING to the test scripts (with its value set from objabi.Expstring(), so that it can determine the experiments baked into the toolchain. I filed #37937 to make a builder with GOEXPERIMENT set to 'staticlockranking'. This builder will ensure another variant of GOEXPERIMENT is being tested regularly for this change, as well as checking static lock ranking in the runtime. Change-Id: Ieb4b86107238febd105558c1e639d30cfe57ab5c Reviewed-on: https://go-review.googlesource.com/c/go/+/222925 Run-TryBot: Dan Scales <danscales@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com> Reviewed-by: Bryan C. Mills <bcmills@google.com>
|
Change https://golang.org/cl/227379 mentions this issue: |
…lder The linux-amd64-staticlockranking builder was added pre-emptively so that it could be tested and used during the development of a CL that implements a new feature. It's expected to fail until that work is completed. Mark it as a builder with a known issue, golang/go#38029. The new BuildConfig.KnownIssue field can be used by builders being added in the future so that if they fail at first, it doesn't take away time for people looking at build.golang.org, which we always aim to have as green as possible¹. ¹ https://groups.google.com/d/msg/golang-dev/y0yM_Xi665Q/hUpBiUPiCAAJ Fixes golang/go#38283 For golang/go#38029 For golang/go#37937 For golang/go#11811 Change-Id: Iba1606c7021ffa7e67ddbaae2fc6b65f4e46ab34 Reviewed-on: https://go-review.googlesource.com/c/build/+/227379 Reviewed-by: Alexander Rakoczy <alex@golang.org>
|
Change https://golang.org/cl/227544 mentions this issue: |
CL 207619 has been submitted to master, fixing golang.org/issue/38029. The linux-amd64-staticlockranking builder has been passing since then. Unset its known issue so that if there are regressions in the future, they can be investigated. For golang/go#38029. For golang/go#37937. Change-Id: I0de8e4ac122effee56aac73c741fab41512ff0c2 Reviewed-on: https://go-review.googlesource.com/c/build/+/227544 Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Dan Scales <danscales@google.com>
Once my changes https://go-review.googlesource.com/c/go/+/222925 (enable build tag corresponding to
GOEXPERIMENTvalue) and https://go-review.googlesource.com/c/go/+/207619 (enforce static lock ranking in the runtime whenGOEXPERIMENTenabled) are checked in, we will want to enable a builder (probably just for linux/amd64) that runs withGOEXPERIMENTequal tostaticlockranking.This will not only test that the lock ranking is not being violated in the runtime (hence helping to avoid deadlocks related to lock acquisition ordering), but will also test that the buildtags are correctly set for a different
GOEXPERIMENTvalue.The text was updated successfully, but these errors were encountered: