Activity
Yesterday
dmitshur commented on golang.org/x/build/main.star: use cipd_platform instead of os/arch where possible12h
(2 comments)
```suggestion
# TODO(mknyszek): Consider adding "_suffix" to the end of this.
```
`lucicfg fmt`?dmitshur reviewed +1 on golang.org/x/image/font/sfnt: support early version 0 OS/2 tables12h
dmitshur commented on golang.org/x/build/cmd/bootstrapswarm: default token file path to LUCI_MACHINE_TOKEN12h
Thanks.dmitshur commented on golang.org/x/build/cmd/bootstrapswarm: default token file path to LUCI_MACHINE_TOKEN14h
@heschi@google.com Is this going in the direction you intended given the discussion in #61666? Happy to adjust this or close if not.dmitshur commented on golang.org/x/build/main.star: start adding a js-wasm builder15h
(1 comment)
Updated both CLs. Done.dmitshur commented on golang.org/x/build/main.star: start adding a js-wasm builder15h
(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…dmitshur reviewed +2 on golang.org/x/build/internal/task: fix cloud build support16h
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…dmitshur opened a change golang.org/x/build/cmd/bootstrapswarm: default token file path to LUCI_MACHINE_TOKEN18h
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.dmitshur commented on golang.org/x/build: add LUCI solaris-amd64 builder19h
> `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
dmitshur opened a change golang.org/x/build/main.star: start adding a js-wasm builder1d
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…dmitshur reviewed +2 on cmd: go get golang.org/x/tools@f9b8da7 and revendor1d
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…dmitshur reviewed +2 on golang.org/x/mobile/cmd/gomobile: resolve symbolic links as a workaround for an Xcode bug1d
Thanks.
(nit) Insert space here.
```suggestion
symbolic links. As a workaround, resolve the path with symbolic links by
```dmitshur reviewed +2 on golang.org/x/build/internal/relui: extra debugging for mismatched tarball hashes1d
Thanks.dmitshur reviewed +2 on golang.org/x/build/internal/relui: use Cloud Build to build primary distpacks1d
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…dmitshur reviewed +2 on golang.org/x/build/internal/task: add script support to Cloud Build1d
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.
…dmitshur reviewed +2 on golang.org/x/build/internal/relui: create ScratchFS1d
(4 comments)
```suggestion
Scratch
</div>
</dt>
<dd class="TaskList-itemResultDefinition">
{{.Artifact.Scratch}}
</dd>
</dl>
{{else i…dmitshur closed an issue log/slog: JSONHandler generates invalid JSON with empty Attr [1.21 backport]2d
dmitshur reviewed +1 on golang.org/x/build/main.star: open up luci-analysis to everyone and document why that's OK2d
Makes sense to me, thanks. +1 so Heschi can look too.dmitshur closed a change golang.org/x/website/blog: fix malformed link to support policy3d
Thanks. This change was applied in CL 529257.dmitshur reviewed +1 on golang.org/x/build/main.star: move luci-analysis bindings to the project level3d
dmitshur commented on golang.org/x/build/internal/buildgo: always prepend -force to make script arguments3d
(1 comment)
@joel@sing.id.au Friendly ping if you're still interested in finishing this, it seems very close. Thanks.dmitshur commented on runtime: set stackguard1 on extra M g03d
@mpratt@google.com Just checking, is this CL being not yet submitted intentionally, or was it accidentally missed? Thanks.dmitshur reviewed +2 on golang.org/x/build/internal/gomote: remove LUCI from coordinator3d
Thanks.dmitshur reviewed +2 on cmd/go: [release-branch.go1.21] in TestScript/gotoolchain_path, remove the user's PATH3d
Thanks.dmitshur reviewed +2 on golang.org/x/build/internal/relui: create ScratchFS3d
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…dmitshur reviewed +2 on golang.org/x/website/_content/doc: add guide for organizing a Go module (project layout)3d
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…dmitshur reviewed +2 on golang.org/x/build/main.star: enable access to luci-analysis3d
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/…dmitshur closed an issue cmd/go: error text for missing package import becomes confusing after toolchain selection4d
dmitshur commented on cmd/go: error text for missing package import becomes confusing after toolchain selection4d
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…dmitshur commented on golang.org/x/build/main.star: use consistent misc-compile sharding for all x repos4d
> 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 …dmitshur reviewed +2 on cmd/go: in TestScript/gotoolchain_path, remove the user's PATH4d
Thanks.dmitshur opened a change golang.org/x/build/main.star: add overridable go_commit property4d
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.dmitshur commented on doc/go1.22: correct tipo languague => language4d
Thanks.Last Week
dmitshur commented on golang.org/x/build/main.star: use consistent misc-compile sharding for all x repos1w
Hold trying it out n x/arch uncovered something I needdmitshur opened a change golang.org/x/arch: empty change1w
DO NOT REVIEW.
DO NOT SUBMIT.
I'm using this CL to try out trybots.dmitshur commented on golang.org/x/build/main.star: increase misc-compile sharding to 3 for x/arch1w
(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…dmitshur commented on golang.org/x/build/main.star: increase misc-compile sharding to 3 for x/tools1w
This change is ready for review.dmitshur opened a change golang.org/x/build/main.star: permit sharding golang.org/x repos1w
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…dmitshur commented on testdata/vcstest/bzr: fix unit test with Ubuntu 2020 LTS bzr 3.0.21w
Marked resolved, since there's nothing more to do here. Thanks for improving the reporting rules thepudds.dmitshur opened a change golang.org/x/build/main.star: remove golang.parallel_compile_only_ports experiment1w
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.dmitshur commented on golang.org/x/review/git-codereview: make hooks command report conflicting hooks1w
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.dmitshur opened a change golang.org/x/build/maintner/maintnerd/maintapi: update test cases for x/tools 1.19+1.181w
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 …dmitshur reviewed +2 on golang.org/x/build/cmd/coordinator: report invalid SlowBots in TRY comments1w
dmitshur reviewed +2 on golang.org/x/build/cmd/coordinator: report invalid SlowBots in TRY comments1w
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…dmitshur commented on golang.org/x/build/cmd/coordinator: report invalid SlowBots in TRY comments1w
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
`…dmitshur commented on golang.org/x/review/...: update installHook1w
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
dmitshur opened a change golang.org/x/build/maintner/maintnerd/maintapi: test x/tools with Go 1.19+1.18 longer1w
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…dmitshur reviewed +2 on golang.org/x/build/cmd/gerritbot: remind again to log in to Gerrit1w
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…dmitshur reviewed +2 on golang.org/x/build/cmd/gerritbot/internal/rules: stop complaining about trailing period after bug number1w
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…dmitshur commented on golang.org/x/build/cmd/coordinator: report invalid SlowBots in TRY comments1w
(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…dmitshur commented on golang.org/x/build/cmd/coordinator: report invalid SlowBots in TRY comments1w
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…dmitshur commented on golang.org/x/build/main.star: extend gotip CQ groups to watch development branches too1w
(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…dmitshur opened a change golang.org/x/build/main.star: extend gotip CQ groups to watch development branches too1w
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…dmitshur opened a change golang.org/x/tools: [gopls-release-branch.0.13] empty change1w
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.dmitshur commented on golang.org/x/build/cmd/gorebuild: add GOARM=6 special case to reproduce linux/arm downloads1w
Thanks.dmitshur commented on golang.org/x/build/cmd/gorebuild: add GOARM=6 special case to reproduce linux/arm downloads1w
This change is ready for review.Earlier
dmitshur starred github.com/Antonio-R1/engine-sound-generator1w
dmitshur pushed to
master in github.com/shurcooL/gofontwoff1w
dmitshur starred artyom.dev/gofontweb1w
dmitshur reviewed +2 on golang.org/x/image/draw: simplify some calls to fmt.Fprintf1w
Thanks.dmitshur commented on cmd/compile/internal/inline: analyze function param properties2w
(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.dmitshur opened a change cmd/compile/internal/inline/inlheur: delete ConstExpr case2w
ir.ConstExpr was deleted in CL 526395, so no need to check for it.
Fixes the build error.dmitshur reviewed +2 on Revert "cmd/cgo: silence unaligned-access"2w
dmitshur commented on golang.org/x/build/...: clean up obsolete +build lines and io/ioutil uses [generated]2w
(1 comment)
Ack, agreed.dmitshur opened a change golang.org/x/build/internal/releasetargets: delete Target.Race field2w
It was last used by Go 1.19 releases, which are no more.dmitshur commented on syscall: skip tests that shouldn't run under 'unshare -n -r'2w
(1 comment)
This too is resolved by CL 526835.dmitshur commented on golang.org/x/build/main.star: disable reuse of results for more builders2w
> 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…dmitshur commented on syscall: skip unshare tests if mount syscall is not available2w
TRY=longtestdmitshur commented on syscall: skip unshare tests if mount syscall is not available2w
(1 comment)
Ok, done. Thanks for the suggestion.dmitshur opened a change golang.org/x/image/go.mod: update go directive to 1.182w
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 …dmitshur commented on syscall: skip unshare tests if mount syscall is not available2w
(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 …dmitshur commented on syscall: skip tests that shouldn't run under 'unshare -n -r'2w
(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…dmitshur opened a change syscall: skip unshare tests if mount syscall is not available2w
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…dmitshur opened a change golang.org/x/build/main.star: disable reuse of results for more builders2w
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.dmitshur opened a change golang.org/x/build/cmd/gorebuild: add GOARM=6 special case to reproduce linux/arm downloads2w
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…dmitshur opened an issue golang.org/x/build/cmd/gorebuild: expects go1.21.1.linux-armv6l.tar.gz and v0.0.1-go1.21.1.linux-arm.zip to be rebuildable without explicitly setting GOARM=62w
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…dmitshur reviewed +2 on golang.org/x/pkgsite/static/frontend: added fix for screen reader to announce complete info.2w
dmitshur commented on golang.org/x/pkgsite/static/frontend: added fix for screen reader to announce complete info.2w
(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…dmitshur reviewed +2 on golang.org/x/pkgsite/static/frontend: added fix for screen reader to announce complete info.2w
dmitshur reviewed +2 on golang.org/x/pkgsite/static/frontend: added fix for screen reader to announce complete info.2w
dmitshur reviewed +2 on golang.org/x/build/main.star: on Solaris, the bot calls AMD64 i86pc-642w
dmitshur commented on golang.org/x/build/internal/task: provide a release archive with all the syso files, or release the files separately2w
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…dmitshur closed an issue golang.org/x/build/internal/task: TestTagXRepos failures2w
Hasn't happened since the increased timeout.dmitshur closed an issue golang.org/x/build/internal/task: track TestGetDevelVersion failures2w
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.dmitshur reviewed +2 on golang.org/x/build/cmd/gomoteserver: use the gomote swarming GRPC server2w
Thanks.dmitshur reviewed +2 on golang.org/x/build/internal/gomote: create gomote swarming server implementation2w
(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…dmitshur reviewed +2 on golang.org/x/build/internal/gomote/protos: rearrange function order2w
> rearrange
I understand the motivation is to move CreateInstanceRequest to be closer to CreateInstanceResponse.dmitshur commented on syscall: skip tests that shouldn't run under 'unshare -n -r'2w
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…dmitshur reviewed +2 on golang.org/x/build/cmd/gomoteserver: fix gomoteserver deployments2w
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…dmitshur reviewed +2 on github.com/mdempsky/unconvert: update deps, fixing support for Go 1.222w
LGTM. Thanks.dmitshur commented on golang.org/x/review/...: update installHook2w
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…dmitshur commented on golang.org/x/review/git-codereview: hooks does not re-install hooks2w
@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…dmitshur reviewed +2 on golang.org/x/pkgsite/go.mod: update golang.org/x dependencies2w
dmitshur commented on golang.org/x/pkgsite/go.mod: run 'go mod tidy'2w
Done in I65f582da11e5c6925c00392953a7965ed8a02686. (Made it separately to hopefully simplify code review.)dmitshur reviewed +2 on golang.org/x/build/main.star: disable openbsd presubmits2w