Activity
Today
dmitshur fixed an issue golang.org/x/build: auto-detect flakes2h
I think we can call this done. https://go.dev/wiki/Watchflakesdmitshur commented on runtime: remove ret field from gobuf2h
Not sure if it's the same underlying issue or not, but noting that perf_vs_release also broke on linux/amd64 (but not on linux/arm64), e.g., https://ci.chromium.org/b/8721827199841767905.dmitshur opened a change golang.org/x/build/main.star: resolve soft merge conflict between CL 653075 and CL 6498763h
dmitshur closed a change golang.org/x/website/tour: deprecate and retract the nested module3h
dmitshur reopened a change golang.org/x/website/tour: deprecate and retract the nested module3h
dmitshur closed a change golang.org/x/website/tour: deprecate and retract the nested module3h
The 'tour/v0.1.0' tag has been made. Nothing more to do here than to close the CL, since it's not intended to be submitted.dmitshur fixed an issue golang.org/x/website/tour: installing @latest gets older version3h
We've retracted all versions of the unintended x/website/tour module in [v0.1.0](https://pkg.go.dev/golang.org/x/website/[email protected]?tab=versions) (including v0.1.0 itself), so now installing or runn…dmitshur commented on golang.org/x/website/tour: deprecate and retract the nested module4h
See my comment above. The CL isn't intended to be rebased on HEAD and submitted. It's meant to be tagged only. The parent is intentionally chosen as the very last commit before tour/go.mod was delete…dmitshur commented on golang.org/x/website/tour: deprecate and retract the nested module4h
(1 comment)
This change is ready for review.
This CL is to review a commit intended to be tagged as "tour/v0.1.0", in order to publish a v0.1.0 version of the golang.org/x/website/tour module. Othe…dmitshur reviewed +2 on golang.org/x/build/internal/gophers: update taking's email6h
Thanks.dmitshur commented on golang.org/x/build/devapp/owners: adjust taking ownership7h
(1 comment)
Doesn't need to be this CL, but you'll likely also want to update the primary Gerrit email so that gopherbot doesn't use the previous one:
```suggestion
addPerson("Tim King", "taking@g…dmitshur reviewed +2 on golang.org/x/build/internal/gophers: add shaojunyang9h
Thanks.dmitshur fixed an issue golang.org/x/build: bot plan9-386 reported as broken10h
It appears this issue is resolved by now. The plan9-386 bot is currently alive and [working](https://ci.chromium.org/ui/p/golang/builders/luci.golang.ci/gotip-plan9-386). Thanks.dmitshur commented on net: return proper error from Context10h
(1 comment)
Since the named error return value isn't being used in a defer or a bare return, and the name "err" doesn't add more context that the "error" type already communicates (in contrast to us…Yesterday
dmitshur fixed an issue golang.org/x/tools/cmd/present: remove/hide/disable Native Client for Go 1.14 or newer1d
Fixed by [CL 468355](https://go.dev/cl/468355) (commit golang/tools@c3550e9181961ca59dfc54232f9690ee4794dbe1).dmitshur commented on golang.org/x/tools/cmd/present: doesn't work with go in git master (1.16)1d
Fixed by [CL 468355](https://go.dev/cl/468355) (commit golang/tools@c3550e9181961ca59dfc54232f9690ee4794dbe1).dmitshur fixed an issue golang.org/x/tools/cmd/present: doesn't work with go in git master (1.16)1d
Fixed by [CL 468355](https://go.dev/cl/468355) (commit golang/tools@c3550e9181961ca59dfc54232f9690ee4794dbe1).dmitshur opened a change golang.org/x/website/tour: deprecate and retract the nested module1d
It stopped being a nested module and its content was merged into
x/website in CL 323897. It was never intended to be a published
module, rather it happened to be one during earlier development
of x/w…dmitshur commented on golang.org/x/website/tour: installing @latest gets older version1d
@ericfrederich Yes, there is a workaround, as mentioned in the original report:
> A workaround is to specify `@HEAD` or the exact version of tour one wants.
For example, `go run golang.org/x/websit…dmitshur reviewed +2 on golang.org/x/build/main.star: enable presubmit for internal vendor branches1d
Thanks.
Ah, never mind, the \\d+ vs -vendor suffix is different.dmitshur commented on golang.org/x/build/main.star: enable presubmit for internal vendor branches1d
Thanks. I think you need to fix formatting to fix trybots; I'll add review vote after that. Optional suggestion.
Might be slightly simpler to use a single regexp that matches both:
```
"private-(re…dmitshur commented on golang.org/x/build/cmd/makemac: check secret.InitFlagSupport error, don't use it in tests1d
Thanks.dmitshur commented on cmd/go: packages with non root vendor elements excluded from module1d
@jsteenb2 The intersection of directories "testdata" and "vendor" is an interesting edge case. (It's possible it already came up and was discussed in a prior issue.) It may be reasonable to expect th…dmitshur opened a change syscall: [release-branch.go1.24] disable O_DIRECTORY on Windows for js/wasm1d
O_DIRECTORY is not available on all platforms, as described at
https://nodejs.org/docs/latest/api/fs.html#file-open-constants .
On Windows, only O_APPEND, O_CREAT, O_EXCL, O_RDONLY, O_RDWR,
O_TRUNC…dmitshur commented on cmd/compile: "fatal error: found pointer to free object" on arm64 [1.24 backport]1d
Removing Critical from backport issue, it only applies to upstream issues (which already has it).dmitshur commented on cmd/compile: nil dereference when storing field of non-nil struct value [1.24 backport]1d
Removed Critical label here, since it only applies to upstream issues.dmitshur commented on syscall: js/wasm file operations fail on windows / node.js1d
@gopherbot Please add this issue for us to consider for backport to 1.24.
I can be convinced otherwise, but not sure if this meets the bar: the experimental js/wasm port is intended to run in browse…dmitshur commented on golang.org/x/build/internal/task: pull in git-generate fix for Windows1d
An unrelated flake. Not enough information to make any changes now (maybe the 'search' endpoint sometimes produces fewer results?), so will let watchflakes report how often it happens in CI.dmitshur reviewed +2 on golang.org/x/build/cmd/watchflakes: log the issue number not the ID1d
Thanks.dmitshur opened a change golang.org/x/build/internal/task: pull in git-generate fix for Windows1d
A fix for https://github.com/rsc/rf/issues/30 has landed.dmitshur reviewed +2 on github.com/go-gl/glfw: Replace Travis with GitHub Workflows and fix linter issues1d
LGTM. Thanks.
I'm not sure how helpful it is to use goimports instead of using gofmt. I think gofmt would be helpful and sufficient.
But if you think there's enough upsides to goimports, let's ke…dmitshur deleted branch in rsc.io/rf1d
fix-windowsdmitshur deleted branch in rsc.io/rf1d
preserve-tabdmitshur commented on github.com/go-gl/glfw: Replace Travis with GitHub Workflows and fix linter issues1d
Thanks for doing this.
I suggest reducing the scope to test only the module in v3.3 (and v3.4 when that’s in), not the top-level one with v3.2 and older. We’re not doing development for v3.2 a…dmitshur starred github.com/ttkb-oss/dedup1d
This Week
dmitshur commented on golang.org/x/build: add LUCI linux-s390x builder2d
Thanks for getting to that trace. Yes, it looks like swarming_bot's logic for auto-detecting information about the CPU in order to populate its dimensions doesn't work as expected on the machine you'…dmitshur commented on golang.org/x/build/internal/task: migrate x509 UpdateBundle to GenerateAutoSubmitChange API2d
Thanks.dmitshur commented on golang.org/x/build/internal/task: detect "no CL needed" in GenerateAutoSubmitChange2d
Thanks.dmitshur closed a change golang.org/x/scratch/gopherbot: reorder words2d
Test of CloudBuild.GenerateAutoSubmitChange is complete. Closing.dmitshur reviewed +2 on golang.org/x/build/internal/task: publish the release after asset upload2d
Thanks. One minor optional comment.
This is a minor and unlikely edge case, but consider a sequence where:
1. a draft release is created with CreateRelease
2. one of its fields (other than "draft")…dmitshur commented on golang.org/x/build/internal/task: migrate x509 UpdateBundle to GenerateAutoSubmitChange API2d
(1 comment)
Yeah, we have that in some places, and this approach as well (e.g., [here](https://cs.opensource.google/search?q=%22flag.Lookup(%5C%22test.run%5C%22)%22&sq=&ss=go%2Fx%2Fbuild)).
My inte…dmitshur commented on golang.org/x/build/cmd/makemac: check secret.InitFlagSupport error, don't use it in tests2d
Copied Votes:
* Code-Review+1 (copy condition\*: "**is:ANY**")
Outdated Votes:
* LUCI-TryBot-Result+1 (copy condition: "changekind:NO_CODE_CHANGE")
\* The label has `labelCopyEnforcement` or `label…dmitshur commented on golang.org/x/build/cmd/makemac: check secret.InitFlagSupport error, don't use it in tests2d
(1 comment)
Can be more precise here: there are 4 leases to add the 4 instances, though only 3 unique makeLeaseRequest calls - one per image config and its cert/key pair. Reworded in PS 3 to clarify.dmitshur commented on golang.org/x/build/cmd/makemac: vacate active instances if they exceed configured count2d
Thanks.dmitshur commented on golang.org/x/build/cmd/makemac: vacate active instances if they exceed configured count2d
Copied Votes:
* Code-Review+1 (copy condition\*: "**is:ANY**")
Outdated Votes:
* LUCI-TryBot-Result+1 (copy condition: "changekind:NO_CODE_CHANGE")
\* The label has `labelCopyEnforcement` or `label…dmitshur fixed an issue golang.org/x/tools/gopls/internal: OpenFile failures on js/wasm with new 1.24.0 toolchain upgrade of Go 1.232d
This should be fixed now per above. See https://ci.chromium.org/ui/p/golang/builders/ci/x_tools-go1.23-js-wasm, and in particular, https://ci.chromium.org/b/8721951848493527569 is the first build wit…dmitshur commented on golang.org/x/build/cmd/golangbuild: support toolchain upgrades when testing GOARCH=wasm ports2d
This is done via the CL mentioned above. There's some cleanup for after Go 1.23 stops being supported, but we don't need to keep this open for that.dmitshur fixed an issue golang.org/x/build/cmd/golangbuild: support toolchain upgrades when testing GOARCH=wasm ports2d
This is done via the CL mentioned above. There's some cleanup for after Go 1.23 stops being supported, but we don't need to keep this open for that.dmitshur commented on golang.org/x/website: ref/mod's vcs-find section should mention the "mod" version control system3d
In that section, where the 3 parts of go-import are being explained, the following written for "vcs":
> `vcs` is the version control system. It must be one of the tools listed in the table below or …dmitshur opened a change golang.org/x/build/cmd/makemac: vacate active instances if they exceed configured count3d
Makemac already takes on increasing instance count when the current
amount falls under the configured MinCount, and decreasing instance
count down to zero when an image outright stops being used.
Ex…dmitshur commented on golang.org/x/tools/gopls/internal: OpenFile failures on js/wasm with new 1.24.0 toolchain upgrade of Go 1.233d
> Can we (hopefully temporarily) disable the go1.N-1 js/wasm builder for the x/tools/gopls module?
Yes, that's certainly fine.
> I filed issue #71902 with the long term fix for the builders. Unfort…dmitshur commented on golang.org/x/build/cmd/golangbuild: support toolchain upgrades when testing GOARCH=wasm ports3d
Sent [crrev.com/c/6299734](https://crrev.com/c/6299734).dmitshur commented on golang.org/x/build/internal/task: detect "no CL needed" in GenerateAutoSubmitChange3d
(1 comment)
Yeah, this is certainly still not ideal. It started out with hardcoded step constants in the completedGeneratingCL method, which was really not great. As you say, adding an extra step is…dmitshur commented on golang.org/x/build/devapp/owners: replace vscode-go owner3d
Thanks.dmitshur commented on golang.org/x/build/cmd/watchflakes: detect and report broken bots3d
(3 comments)
Oh, I understand my confusion now.
I was thinking of the "target" property with "goos" and "goarch" values. We do in fact set those on every builder definition ([here](https://cs.opens…dmitshur commented on golang.org/x/build/internal/task: migrate x509 UpdateBundle to GenerateAutoSubmitChange API3d
Copied Votes:
* Code-Review+1 (copy condition\*: "**is:ANY**")
Outdated Votes:
* LUCI-TryBot-Result-1 (copy condition: "changekind:NO_CODE_CHANGE")
\* The label has `labelCopyEnforcement` or `label…dmitshur commented on golang.org/x/build/internal/task: migrate x509 UpdateBundle to GenerateAutoSubmitChange API3d
(1 comment)
This doesn't work on Windows due to a problem in git-generate (https://github.com/rsc/rf/issues/30). I sent a fix which is enough to make this work (tested [here](https://ci.chromium.org…dmitshur opened a change golang.org/x/build/internal/task: try out git-generate patches4d
DO NOT REVIEW, DO NOT SUBMIT.
This is meant to test out fixes
for https://github.com/rsc/rf/issues/30
and https://github.com/rsc/rf/issues/31.dmitshur pushed to
main in rsc.io/rf4d
dmitshur pushed to
main in rsc.io/rf4ddmitshur opened a change rsc.io/rf/git-generate: use git cat-file to get the raw commit message body4d
The git log command is considered porcelain, and by default it expands tabs into spaces, which can break some scripts that are sensitive to whitespace changes. It's possible to add the --no-expand-ta…dmitshur opened a change rsc.io/rf/git-generate: clean gitdir filepath on Windows4d
For some reason, on Windows, git prints the path with '/' separators instead of '\'. This by itself isn't a problem, but it intersects poorly with prefix-based relative path computation in walkGit:
…dmitshur pushed to
preserve-tab in rsc.io/rf4ddmitshur pushed to
main in rsc.io/rf4ddmitshur pushed to
fix-windows in rsc.io/rf4ddmitshur opened an issue rsc.io/rf/git-generate: tab characters in scripts are expanded to spaces4d
Consider any git-generate script that happens to contain tab characters; for example:
```
[git-generate]
cat <<EOF | wc -c > out.txt
.
EOF
```
Running that under bash -e, or by placing it in a fil…dmitshur opened an issue rsc.io/rf/git-generate: doesn't work on Windows due to path separator mismatch4d
Trying to use git-generate on Windows seems to ouright not work, failing with an error like:
cannot compute relative path: C:/reporoot vs C:\reporoot\file.txt
This reproduces with git on 2 Windows…dmitshur pushed to
main in rsc.io/rf4ddmitshur created branch in rsc.io/rf4d
fix-windowsdmitshur created branch in rsc.io/rf4d
preserve-tabdmitshur forked in rsc.io/rf4d
dmitshur commented on cmd/compile/internal/ssa: correct MOVDnop handling for arm644d
(1 comment)
Let's drop the '#' before the number, it causes it to get interpreted as an issue number rather than the CL as you intended.
```suggestion
CL 568616. This means that the late lower pass…Last Week
dmitshur commented on golang.org/x/tools/gopls/internal: OpenFile failures on js/wasm with new 1.24.0 toolchain upgrade of Go 1.235d
Yeah, this is indeed related to the new toolchain behavior introduced in Go 1.21, and the wasm builders not yet being updated to handle it. We just haven't really noticed it until now.
I filed issue…dmitshur opened an issue golang.org/x/build/cmd/golangbuild: support toolchain upgrades when testing GOARCH=wasm ports5d
The first GOARCH=wasm port was added [in Go 1.11](https://go.dev/doc/go1.11#wasm), way before the [Go 1.21 toolchain selection/switching](https://go.dev/doc/toolchain) behavior was added.
Builders t…dmitshur commented on golang.org/x/build: bot linux-riscv64-mengzhuo--vf2-4 reported as broken5d
Thanks for taking a look at this @mengzhuo.
Deleting LUCI bots is currently requires administrative access. If it's viable to extend access to you as the bot owner, we'll do that, but for now you ha…dmitshur fixed an issue golang.org/x/build: bot linux-riscv64-mengzhuo--vf2-4 reported as broken5d
Thanks for taking a look at this @mengzhuo.
Deleting LUCI bots is currently requires administrative access. If it's viable to extend access to you as the bot owner, we'll do that, but for now you ha…dmitshur commented on golang.org/x/build/internal/task: migrate x509 UpdateBundle to GenerateAutoSubmitChange API6d
Copied Votes:
* Code-Review+1 (copy condition\*: "**is:ANY**")
Outdated Votes:
* LUCI-TryBot-Result-1 (copy condition: "changekind:NO_CODE_CHANGE")
\* The label has `labelCopyEnforcement` or `label…dmitshur opened a change golang.org/x/build/internal/task: migrate x509 UpdateBundle to GenerateAutoSubmitChange API6d
The GenerateAutoSubmitChange API is hopefully slightly simpler, but
mostly I'm using this small task as a convenient way to test out both
the real Cloud Build implementation and the fake at a more fi…dmitshur reviewed +2 on golang.org/x/crypto/acme/autocert: check host policy before probing the cache6d
dmitshur reviewed +2 on golang.org/x/build/cmd/watchflakes: detect and report broken bots6d
(1 comment)
`r.Body.Close` [should happen](https://pkg.go.dev/net/http#pkg-overview) even if status code is unexpected; re-arranging their order gets you that behavior:
```suggestion
defer r.Body.…dmitshur reviewed +2 on golang.org/x/build/internal/task: don't reuse ChangeInfo pointers6d
Thanks.
That's one of the advantages of doing `if new, err := ...; err != nil` over `new, err := ...; if err != nil`, i.e., can't accidentally reuse it after the if block.dmitshur reviewed +2 on golang.org/x/build/cmd/watchflakes: detect and report broken bots6d
(2 comments)
Thanks, at first glance that's unexpected to me, I thought we try to always set it. I'll take a look at that, but no need to make changes here now. Thanks.
Sure; I'm referring to this …dmitshur reviewed +2 on golang.org/x/build/internal/task: when ignoring errors, maintain ChangeInfo6d
(1 comment)
Is it intentional PS 2 makes the change in the other file but not here?dmitshur reviewed +2 on golang.org/x/build/internal/task: when ignoring errors, maintain ChangeInfo6d
Whoops, yeah.
Hmm, if MoveChange succeeds and RebaseChange gets here, newCI from successful MoveChange will be overwritten with the original one. I guess you don't want that?
Since you're already c…dmitshur reviewed +2 on golang.org/x/build/internal/task: add newline to error message check6d
Thanks.dmitshur reviewed +2 on golang.org/x/build/internal/task: be tolerant of change/rebase being a noop6d
Thanks.
(nit) The comment and `var httpErr` line order seem swapped?
I think it'd be nice for this to not to ignore the status code, since the docs for this endpoint promise 409 Conflict in this ca…dmitshur reviewed +2 on golang.org/x/build/internal/task: remove keppAllVotes from MoveChange6d
Thanks.dmitshur reviewed +2 on crypto/rand: add example for Int6d
Thanks. Re-adding votes from PS 3.dmitshur reviewed +2 on golang.org/x/build/internal/task: set proper repo name for privx6d
Thanks.dmitshur commented on golang.org/x/build/internal/task: narrow mailing list email search by subject and keywords6d
Thanks.dmitshur reviewed +2 on golang.org/x/build/internal/task: add SUBMITTABLE opt to privx GetChange6d
Thanks.dmitshur reviewed +2 on golang.org/x/build/cmd/watchflakes: detect and report broken bots6d
Thanks! Some minor comments.
It's great to have automation notice when a builder needs attention and CC its owners for awareness. I understand it'll be mostly up to humans to follow up on the issue,…dmitshur commented on golang.org/x/crypto/x509roots/fallback: drop obsolete build constraint6d
Thanks. One question for Roland, since the build constraint affects the language version in addition to specifiing the minimum toolchain.
This is the only loop, and it's not the kind whose behavior …dmitshur commented on golang.org/x/build/repos: add AutoTagNextMinorVersion and AutoMaintainGoDirective6d
Thanks.dmitshur commented on golang.org/x/build/internal/task: add GoDirectiveXReposTasks to maintain go directive6d
Copied Votes:
* Code-Review+1, Code-Review+2 (copy condition\*: "**is:ANY**")
Outdated Votes:
* LUCI-TryBot-Result+1 (copy condition: "changekind:NO_CODE_CHANGE")
\* The label has `labelCopyEnforce…dmitshur commented on golang.org/x/build/internal/task: add GoDirectiveXReposTasks to maintain go directive6d
Thanks.dmitshur closed an issue runtime: result of `(*runtime.Func).Name` is malformed in go1.246d
dmitshur commented on golang.org/x/build/internal/task: add GoDirectiveXReposTasks to maintain go directive6d
Copied Votes:
* Code-Review+1 (copy condition\*: "**is:ANY**")
Outdated Votes:
* Commit-Queue+1 (copy condition: "changekind:NO_CODE_CHANGE")
\* The label has `labelCopyEnforcement` or `labelCopyRe…dmitshur commented on golang.org/x/build/internal/task: add GoDirectiveXReposTasks to maintain go directive6d
(1 comment)
This change is ready for review.
Resolved via CL 650156, so this can be removed now. Done in PS 4.dmitshur commented on golang.org/x/build/repos: add AutoTagNextMinorVersion and AutoMaintainGoDirective6d
Copied Votes:
* Code-Review+1, Code-Review+2 (copy condition\*: "**is:ANY**")
Outdated Votes:
* LUCI-TryBot-Result+1 (copy condition: "changekind:NO_CODE_CHANGE")
\* The label has `labelCopyEnforce…