Activity

Yesterday
Whoops, I noticed I'm in CC not reviewer after finishing reviewing. Oh well. (Oh actually that changed while I was reviewing.) Thanks. Comma separating issues is fine since the verb here is "For". I…
Thanks. (nit) s/Gb/GB/ like in RootDriveSizeGB? I realize the field in compute.AttachedDiskInitializeParams type doesn't do this, but go.dev/s/style#initialisms includes: > Code generated by the pr…
Thanks. Two minor comments. Thanks. Two minor comments. Now that GOROOT_BOOTSTRAP points to a path without 1.18 in it, maybe remove this part of the comment or update it to "using own Go bootstrap;…
This Week
Apply standard formatting to all of x/build using gofmt from Go 1.19.3.
Thanks. I expect this to not need much maintenance and okay to leave as is. Just mentioning that it might be possible to further reduce dependence on specifics of builder names, something like: ``…
Thanks. I understand the us-central1-{a,b,f} zones work well for our needs.
Thanks. I understand the us-central1-{a,b,f} zones work well for our needs.
dmitshur reviewed +2 on doc/go1.20: fix missing <code> tag1d
Thanks.
dmitshur commented on doc/go1.20: fix HTML closing tag2d
The Review-Enforcement submit requirement needs 1 more vote. Ian or someone else will likely get to it tomorrow.
Last Week
dmitshur starred github.com/xtrp/darkhn3d
dmitshur pushed to master in github.com/shurcooL/githubv43d
0b5c4c7994eb7f1a569c78c9ab15eb4feef0c463regenerate for schema changes by 2022-11-25
dmitshur starred github.com/corkami/pics5d
dmitshur reviewed +2 on lib/time: update to 2022f6d
Thanks.
Thanks, everything here looks right.
> @golang/release requesting a freeze exception for this. @rolandshoemaker Thanks for letting us know. A freeze exception shouldn't be needed here since this looks like an unintentional bug on mac…
Thanks. I recall longtest+race being slower, but not sure how much. Adding significantly more helpers doesn't scale as well as the initial few (see https://go.dev/issue/37439#issuecomment-723413000 …
The current releaselet descriptions are good, as recently discussed, leaving just one typo in the macOS version name to fix up to reduce confusion.
Thanks. See some remaining minor comments posted inline. I understand you're planning to take care (or already have) of prod permissions as needed. Can be simplified to reqBodyJSON{&input}, which'l…
I think this gets everything right. Thanks. That does seem like a good way, I'll try to remember it and suggest it next time.
Thanks. There's one way in which forcibly using GOROOT/bin/go (instead of using the one in PATH) can work poorly: if executing commands that themselves need to find and execute a 'go' binary, they w…
Thanks. Consider checking the build info instead of executing the bundle command, since it might be be a bit more robust: ``` if bi, err := buildinfo.ReadFile(bundlePath); err != nil || bi.Path != …
Thanks. Question: Issue #56904 talks about -newcc builder but this matches both. Is that intentional so that it'll keep working when -newcc builders lose that suffix? Minor: Typo, delete 'v'.
(1 comment) I'd expect you can use the existing ReadBranchHead method here instead of creating this very similar new one. It may already support the gerrit.ErrResourceNotExist error Heschi mentions …
(1 comment) It makes sense to do that in general, and it's already done in some places like https://cs.opensource.google/go/go/+/master:src/cmd/internal/moddeps/moddeps_test.go;l=451-456;drc=6484e81…
Thanks, it looks good to me.
(1 comment) DirEntry.Type does seem to be missing permissions though, as checked via a small program (https://go.dev/play/p/jzbq_rdI4B7). So if you're switching to WalkDir, it is necessary to use Di…
Earlier
The cmd/internal/moddeps test spotted a problem, sent CL 452195 to fix it. It can be useful to run that test locally or via TRY=longtest SlowBots whenever changing dependency versions.
dmitshur commented on net/http: regenerate h2_bundle.go1w
TRY=longtest
dmitshur opened a change net/http: regenerate h2_bundle.go1w
Done with: go generate -run=bundle std After CL 452096 updated the x/net version.
This is a tracking issue for adding JSON output support to the cmd/api/run.go program, which is invoked by cmd/dist to implement the "API check" test. This is a subset of #37486, broken out into a sm…
This is a tracking issue for adding JSON output support to the test/run.go program, which is invoked by cmd/dist to run tests in the GOROOT/test directory. This is a subset of #37486, broken out into…
As of go.dev/issue/56031, the need to have passing TryBots and no unresolved comment threads is enforced by a submit requirement in Gerrit. The code in the auto-submit task is no longer load-bearing,…
(1 comment) reflect.DeepEqual is fine, but if you haven't already considered it, you can use cmp.Diff here which I expect might produce more readable test failures.
In general, everything looks good, nice work on your first relui workflow change! See some comments, mostly minor. "golang/go-private"? Nit: add period for consistency with line 1177 and others. C…
(2 comments) Given that the minimum Go bootstrap version has been changed to 1.17.13 minimum¹, it should help to replace mentions of "1.17" here and below with "1.17.13", to reduce possible confusi…
@FiloSottile This question is a better fit for issue #36905, if I understand it correctly. With the 1.20 freeze coming up next week, we will be updating the `std` and `cmd` modules to vendor the late…
I'm probably not the right person to review this change. @codyoss@google.com, can you suggest a better reviewer? Thanks.
Thanks for rebasing and updating, the changes makes sense to me from a quick look. I'll leave review to Rob since this is x/tools.
@nigeltao Yes, I expect a passing CLA is required (though I'm less familiar with snappy-specific details compared to the more common golang.org/x repos). This PR has it passing from looking at the [C…
dmitshur commented on image: add ListFormats1w
The TryBot failure should go away if you rebase this PR past CL 449640, but the api check should fail because this adds a new API without a corresponding accepted proposal. Please see https://github…
CL 450315 updated gerritbot to a newer GitHub API version, where the optional since parameter is specified as a pointer rather than value. This gives the caller more control over whether the optional…
dmitshur reviewed +2 on cmd/dist: simplify old code1w
Thanks.
Thanks. Since this is newly written Go code, would it be better to follow the https://go.dev/s/style#mixed-caps convention and name these fields goExe and goTmpDir? (Or are they intentionally treate…
https://go.dev/wiki/MinorReleases includes: > The issue should include a link to the original issue and a short rationale about why the backport might be needed. @randall77 Can you please add a…
(1 comment) Makes sense. Thanks for considering both approaches.
Thanks. Optional logical simplification, if you think it's an improvement: this can be a const, since nothing below needs to mutate it. Also fine as a variable. Looks like this is missing 2" after …
dmitshur reviewed +2 on cmd/dist: restructure cgo_test1w
Nice cleanup here. I didn't spot problems. Thanks.
(1 comment) Nit: The receiver is unused and can be left out if you think that's better and worth doing. (Same on line 1014.) Also fine as is.
(1 comment) It seems all current goTest methods only read their fields and don't try to modify them, so if you wanted, you could use value receivers instead of pointer receivers. That would let you …
Still reading through the details in the rest of the stack, but this abstraction looks very reasonable and I'm not spotting problems here. Minor comments only. Thanks. If both are set, are they join…
@hickford It should not be necessary to include a Change-Id footer in the PR description. [GerritBot](https://go.dev/wiki/GerritBot) is responsible for doing that.
dmitshur reviewed +1 on cmd/dist: eliminate "time" special case2w
Thanks. I'm realizing that, when the only change in a PR is its draft status, that won't get synchronized to Gerrit until another change also happens. It should be possible to improve the experience…
dmitshur reviewed +1 on cmd/dist: eliminate registerSeqTest2w
Great. If you're still looking for it, `go test ./...` would test all packages in this module. (Package patterns are explained in detail at https://pkg.go.dev/cmd/go#hdr-Package_lists_and_patterns.)
(1 comment) Thanks.
This generally looks good, but there are a few compilation errors, as reported by TryBots. You should be able to reproduce them locally with go test ./cmd/gerritbot. After they're resolved, I think w…
Thanks. Please see inline comments. Please see https://go.dev/wiki/CommitMessage, notably: • add "cmd/gopherbot: " path prefix • first word starts with lower case • no period at the end Shor…
Thanks. Nit: This line is a bit long and could use wrapping. This is of course not a big deal. I'm only mentioning it because it's one of the suggestions listed at https://go.dev/wiki/CommitMessage…
Thanks. Issue #26624 is resolved (via CL 266541), so closing.
Thanks.
dmitshur starred github.com/a-h/gpu3w
In #56560, @hyangah makes this suggestion too and points out that by now we have relui. So this can also be implemented by adding a task in the relui release workflow that notifies pkg.go.dev about t…
This might be fixed at tip. The test passes without pkg/darwin_arm64/cmd/compile/internal/syntax.a.
Yeah, I think this is fixed via [CL 432535](https://go.dev/cl/432535). Closing and can reopen if we learn otherwise.
This might be fixed at tip. The test passes without pkg/darwin_arm64/cmd/compile/internal/syntax.a.
### What version of Go are you using (`go version`)? <pre> $ go version go version go1.19.3 darwin/arm64 </pre> ### Does this issue reproduce with the latest release? Unsure, there isn't …
To update this issue with the latest PR status: the comment above is resolved in https://github.com/golang/freetype/pull/86#issuecomment-970275372.
We have learned that the overall problem of quota exhaustion still happens after coordinator restarts during a busy time; @prattmic and @heschi have made more progress on understanding that, but it's…
dmitshur starred github.com/gen2brain/mpeg1mo
I just checked and it doesn't seem this is happening on any newer PRs either. For reference, neither I nor @cagedmantis changed gerritbot, so the fix must've happened on the side of the Gerrit server…
@eli-darkly Note that it is possible to link to individual fields inside structs or methods in interfaces on pkg.go.dev. For example, here's the [`DataSource` field in `ldclient.Config` struct](https…
dmitshur pushed to master in github.com/shurcooL/githubv41mo
a134b1472cc7856d2dce3b6de514708c0f3a94ecregenerate for schema changes by 2022-10-20
Thanks.
The -trust flag is obsolete and was removed in [CL 399117](https://go.dev/cl/399117), which is included in v1.1.0. Maybe the `git-codereview` binary you installed with `go install golang.org/x/review…
`SigningServer.send` has a possibility of panicking when executing this line: https://cs.opensource.google/go/x/build/+/master:internal/relui/sign/server.go;l=110;drc=656fd833c86463e51c5e722b28134…
This error isn't happening as of yesterday 9:22 am (EDT), both PRs have been updated at around that time.