(1 comment)
We already use the 'os:Linux' dimension for coordinator builders (where, like here, any Linux is fine). It seems I can resolve this TODO by changing the expected_dimensions["os"] in crre…
Thanks.
If you haven't already, you could consider %q over %v, which might help if there are filenames with spaces in the future.
(minor)
The ServiceAccount field [docs](https://pkg.go.dev/cloud.g…
If LUCI_MACHINE_TOKEN is set in the environment (intended for the
swarming bot; added in crrev.com/i/6478812), also recognize it here
and use it as the default token file path flag value.
> `2023/09/22 11:24:17 unable to read file "/var/lib/luci_machine_tokend/token.json": open /var/lib/luci_machine_tokend/token.json: no such file or directory`
@rorth Sorry, there was a typo, the e…
Start by adding it as a post-submit builder, only in the main Go repo,
and only at tip. Mostly so it's easier to see the generated code and
iterate. Once it works, it'll be turned up in the remaining…
Thanks.
Can you add another footer, after the Change-Id:
```
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest
```
And then run Commit-Queue+1 again. (See https://go.dev/wiki/SlowBots…
Thanks.
I think it's fine. It's even better in some ways:
1. it's officially supported¹ ("Because of backward compatibility [...] any release ≥ Go 1.20.6 but < Go 1.x will work as the bootstr…
Thanks.
It's very nice.
(minor) The CLs in this stack seem to have a common theme. If so, it'd be convenient to include "For golang/go#nnn." pointing to an issue here, so they're easier to track.
…
Thanks.
```suggestion
ServingURL string // ServingURL is a gs:// or file:// URL, no trailing slash.
```
The example was for ScratchURL, so it's more out of place for ServingURL. Ther…
Thanks. This captures great advice, and I look forward to more Go developers benefitting from it being written down here.
Minor optional comments inline.
Consider inlining this too, like done above…
Thanks.
(minor) "committers" in this comment seems to refer to something like acl.CQ_COMMITTER from [LUCI terminology](https://chromium.googlesource.com/infra/luci/luci-go/+/refs/heads/main/lucicfg/…
> Hold; trying it out in x/arch uncovered something I need to take care of first.
For sharded builders working with golang.org/x repo, it's necessary to explicitly pass the commit chosen by reading …
Sharded golang.org/x repo builders need to pass down the go_branch
commit they've selected to the children builds. Add an overridable
property for that.
(1 comment)
Ack, thanks. I'll see what I can do about this after landing this CL.
> perhaps drop run mods from the builder name in a follow-up CL
I was thinking about that opportunity too. There a…
The misc-compile builder is the bottleneck by a significant margin in
some golang.org/x repos such as x/tools. This is the first time we're
extending sharding beyond the main Go repo, so there are so…
Thanks, see two minor comments.
Thanks. Please also remove "about" at the end. Just "..., let the user know."
s/exists/exist/ on this line. Multiple files exist. A single file exists.
I thought I ran long tests for CL 528155 locally, but clearly I missed
some nearby relevant packages. Fix the broken test case. It's a bit odd
because it encodes a snapshot in time when Go 1.16+1.15 …
Thanks.
Please delete this blank line, then run `gofmt -w coordinator.go` (or equivalent) so it's sorted within the block below. There's no need for it to stand out in a separate block.
```suggesti…
One more suggestion to merge the comment that will be posted with the existing one, otherwise I think this looks good.
(minor) This isn't gofmt'ed:
```
$ gofmt -l .
cmd/coordinator/coordinator.go
`…
Maybe this can be improved by documenting the meaning of the `auto` parameter, perhaps something like:
```
// installHook installs Git hooks to enforce code review conventions.
//
// auto is whether…
We've been able to extend LUCI test coverage for the needs of gopls,
which makes Kokoro CI testing that previously provided said testing
obsolete. What I failed to realize until trying to clean up af…
Thanks.
The https://go.dev/wiki shortlink to wiki pages is shorter to read (when someone hovers over the link to see where "here" links to), and will work better after #61940 is implemented.
```sug…
Thanks for addressing this.
This note is possibly better to leave to a future change, and consider out of scope of this one (since this one is about addressing the trailing dot specifically), but le…
(5 comments)
> Regarding invalid builds being reported as an error or warning, what is your take on this?
I think either approach can work okay and it's not a significant difference given these slo…
Just a note that as mentioned at https://groups.google.com/g/golang-dev/c/H65LelTvAXQ, we're moving to LUCI for pre-submit and post-submit testing. The old SlowBot syntax (i.e., https://go.dev/wiki/S…
(1 comment)
Thanks. I initially started with a tuple, but wanted to try a struct, and thought it was a small improvement since the caller can use `cq_group.name` and not worry about assigning the re…
Extend the gotip CQ groups to watch other development branches like
"dev.inline", "gopls-release-branch.0.13", and so on. That is, they
now watch all branches other than branches already watched by g…
(1 comment)
ir.ConstExpr was removed in CL 526395, so it seems this condition can be dropped as it'll never happen (in addition to fixing the build error). Sent CL 526196 if it's helpful.
> I originally did tricium on purpose (since the analyzers currently only pertain to the current CL contents, IIUC)
I that's my understanding too, and I was on the fence about tricium. I'll leave it…
There's no reason for this module to still be assuming the semantics
of Go 1.12. Pick 1.18 as a semi-arbitrary newer version because it's
used by a few other golang.org/x modules, and it's enough to …
(1 comment)
I can certainly do that if that’s preferable.
I initially considered using a special exit code to indicate “skip”, but some searching around didn’t find precedent, so I thought …
(1 comment)
> So if we just ignore a `Mount` error in the subprocess, the test should still pass.
If I understand correctly, in that case it would be slightly better for the test to be marked as sk…
CL 513779 added crude skips for tests that couldn't work when run under
'unshare --net --map-root-user' as used by the current iteration of the
no-network check in LUCI. Bryan suggested a more target…
It seems we should change the default GOARM value for cross-compilation
from 7 to 6 to match the decision we've made for the posted downloads.
But if we don't do that, then we need to teach gorebuild…
As visible at https://go.dev/rebuild, `gorebuild` has uncovered the possibility of non-reproducibility on go1.21.1.linux-armv6l.tar.gz (and its module zip version, v0.0.1-go1.21.1.linux-arm.zip) in t…
(1 comment)
I understand the goal here is to express that the text must contain `"Version: " + p.FormattedVersion`, though additional whitespace in between the text is acceptable.
I think this shou…
The decision to not ship race detector syso files for other platforms was made in 2018 in cmd/release ([CL 144281](https://go.dev/cl/144281)), to reduce archive binary size. It was ported to cmd/relu…
So far the data points suggest the flake rate is at an acceptable (to me) rate. At least it's not unreasonably high.
Closing since doesn't need active work and watchflakes will still post results.
(2 comments)
(minor) Consider factoring out the log.Fatalf error to the caller. That is, NewSwarming in this internal gomote package can return (*SwarmingServer, error) and the caller can handle the…
Thanks for comments @bcmills@google.com. I replied to the `testenv.SyscallIsNotSupported` point, happy to improve it further if you have ideas given the constraint that the use of `unshare --net --ma…
Thanks.
(nit) Consider moving log lines similar to this to happen after flag.Parse() in main(). That way, they don't print if -help is used, or the program otherwise exits due to a flag parsing erro…
Thanks for the CL. I left some comments if you're still interested in working on it.
This documentation probably needs to be updated for the new behavior.
```suggestion
// If hook file exists but…
@BerndCzech Thanks for looking to contribute to Go. There is indeed an existing CL here, and a next step before it can be submitted is for it to be [reviewed](https://go.dev/doc/contribute#review). I…