Activity

Yesterday
(2 comments) ```suggestion # TODO(mknyszek): Consider adding "_suffix" to the end of this. ``` `lucicfg fmt`?
@heschi@google.com Is this going in the direction you intended given the discussion in #61666? Happy to adjust this or close if not.
(1 comment) Updated both CLs. Done.
(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…
This Week
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. (nit) Insert space here. ```suggestion symbolic links. As a workaround, resolve the path with symbolic links by ```
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. …
(4 comments) ```suggestion Scratch </div> </dt> <dd class="TaskList-itemResultDefinition"> {{.Artifact.Scratch}} </dd> </dl> {{else i…
Makes sense to me, thanks. +1 so Heschi can look too.
Thanks. This change was applied in CL 529257.
(1 comment) @joel@sing.id.au Friendly ping if you're still interested in finishing this, it seems very close. Thanks.
@mpratt@google.com Just checking, is this CL being not yet submitted intentionally, or was it accidentally missed? Thanks.
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/…
Thanks. I can confirm: ``` $ GOTOOLCHAIN=go1.20.7 go build p.go:3:8: import "": import of unknown directory $ GOTOOLCHAIN=go1.20.8 go build go: downloading go1.20.8 (darwin/arm64) p.go:3:8: p…
> 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.
Thanks.
Last Week
Hold trying it out n x/arch uncovered something I need
dmitshur opened a change golang.org/x/arch: empty change1w
DO NOT REVIEW. DO NOT SUBMIT. I'm using this CL to try out trybots.
(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…
This change is ready for review.
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…
Marked resolved, since there's nothing more to do here. Thanks for improving the reporting rules thepudds.
By now there was enough data collected. crrev.com/c/4864701 made the decision to keep the parallel misc-compile path. This experiment has concluded.
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…
dmitshur closed a change golang.org/x/tools/txtar: use fmt.Append1w
This was useful to confirm CL 528155 works as expected, and it is. Closing again.
dmitshur reopened a change golang.org/x/tools/txtar: use fmt.Append1w
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…
DO NOT REVIEW. DO NOT SUBMIT. I'm using this CL to try out trybots.
dmitshur closed a change golang.org/x/tools/txtar: use fmt.Append1w
Test complete, the 1.18-only failure was correctly caught via the x_tools-go1.18-linux-amd64 build.
dmitshur opened a change golang.org/x/tools/txtar: use fmt.Append1w
DO NOT REVIEW, DO NOT SUBMIT: Test for 1.18+1.19 trybots.
Earlier
dmitshur pushed to master in github.com/shurcooL/gofontwoff1w
a36d5fd8747a8123cb4cdd657b51c367c4368930README: mention the artyom.dev/gofontweb alternative
dmitshur starred artyom.dev/gofontweb1w
(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.
ir.ConstExpr was deleted in CL 526395, so no need to check for it. Fixes the build error.
It was last used by Go 1.19 releases, which are no more.
(1 comment) This too is resolved by CL 526835.
> 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…
(1 comment) Ok, done. Thanks for the suggestion.
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…
Extend the change CL 526735 applied to tricium checks and additional trybots. At least I can't think of a reason to treat them differently.
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…
Hasn't happened since the increased timeout.
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…
> rearrange I understand the motivation is to move CreateInstanceRequest to be closer to CreateInstanceResponse.
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…
Done in I65f582da11e5c6925c00392953a7965ed8a02686. (Made it separately to hopefully simplify code review.)