Activity
Yesterday
dmitshur reviewed +2 on golang.org/x/build/dashboard: run AMD64 Macs AWS, build 1.20 with 1314h
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…dmitshur reviewed +2 on golang.org/x/build/...: add additonal storage to linux-arm64-longtest builder15h
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…dmitshur reviewed +2 on golang.org/x/build/dashboard: update host-linux-loong64-3a5000 owner, ExpectNum, bootstrap17h
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
dmitshur opened a change golang.org/x/build/cmd/coordinator: gofmt1d
Apply standard formatting to all of x/build using gofmt from Go 1.19.3.dmitshur reviewed +2 on testing: remove stale builder names from windows tests1d
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:
``…dmitshur reviewed +2 on golang.org/x/build/buildenv: remove zone without ARM machine types1d
Thanks.
I understand the us-central1-{a,b,f} zones work well for our needs.dmitshur reviewed +2 on golang.org/x/build/buildenv: remove zone without ARM machine types1d
Thanks.
I understand the us-central1-{a,b,f} zones work well for our needs.dmitshur reviewed +2 on golang.org/x/build/cloudfns/wikiwebhook: update cloud.google.com/go/pubsub version1d
Thanks.dmitshur reviewed +2 on golang.org/x/build/...: update github.com/creack/pty version1d
Thanks.dmitshur reviewed +2 on golang.org/x/build/cmd/xb: add ARM images to valid layers lists1d
Thanks.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.dmitshur starred github.com/dethrace-labs/dethrace2d
Last Week
dmitshur starred github.com/xtrp/darkhn3d
dmitshur pushed to
master in github.com/shurcooL/githubv43d
dmitshur merged a change github.com/shurcooL/githubv4/feat: regenerate for schema changes by 2022-11-253d
dmitshur reviewed +2 on github.com/shurcooL/githubv4/feat: regenerate for schema changes by 2022-11-253d
Thanks.dmitshur reviewed +2 on golang.org/x/website/_content/doc/articles/race_detector: cgo requires a C compiler4d
Thanks.dmitshur starred github.com/corkami/pics5d
dmitshur reviewed +2 on lib/time: update to 2022f6d
Thanks.dmitshur reviewed +2 on golang.org/x/build/dashboard: add linux-arm64-longtest builder6d
Thanks, everything here looks right.dmitshur commented on crypto/x509: Verify on macOS does not return typed errors1w
> @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…dmitshur reviewed +2 on golang.org/x/build/dashboard: shard linux-amd64-longtest-race and windows-amd64-longtest-newcc builders1w
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 …dmitshur opened a change golang.org/x/build/dashboard: fix typo in host-darwin-arm64-12 description1w
The current releaselet descriptions are good, as recently discussed,
leaving just one typo in the macOS version name to fix up to reduce
confusion.dmitshur reviewed +2 on golang.org/x/build/cmd/relui: create internal release branches as part of pre-announce workflow1w
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…dmitshur reviewed +2 on cmd/dist: revise skip sequence added in CL 4527631w
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.dmitshur reviewed +2 on cmd/vendor: update vendored github.com/google/pprof for Go 1.20 release1w
Thanks.dmitshur reviewed +2 on golang.org/x/build/cmd/updatestd: use the go command from goroot flag1w
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…dmitshur reviewed +2 on all: update vendored golang.org/x dependencies for Go 1.20 release1w
Thanks.dmitshur reviewed +2 on golang.org/x/build/cmd/updatestd: check that the bundle command is what we want1w
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 != …dmitshur reviewed +2 on cmd/dist: skip -race w/ external linkage on windows 20081w
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'.dmitshur commented on golang.org/x/build/cmd/relui: create internal release branches as part of pre-announce workflow1w
(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 …dmitshur reviewed +2 on go/types: skip crypto/internal/bigmod/_asm in TestStdlib1w
(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…dmitshur reviewed +2 on cmd/dist: update to assume Go 1.17.13 as minimal Go bootstrap version1w
Thanks, it looks good to me.dmitshur commented on cmd/dist: update to assume Go 1.17.13 as minimal Go bootstrap version1w
(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
dmitshur commented on all: update golang.org/x/crypto to 2c476679df9a1w
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=longtestdmitshur opened a change net/http: regenerate h2_bundle.go1w
Done with:
go generate -run=bundle std
After CL 452096 updated the x/net version.dmitshur opened an issue cmd/api: add JSON output support to the run.go program1w
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…dmitshur opened an issue test: add JSON output support to the run.go program1w
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…dmitshur commented on golang.org/x/build/cmd/gopherbot: rely on Gerrit config for TryBot and resolved comments1w
This change is ready for review.dmitshur opened a change golang.org/x/build/cmd/gopherbot: rely on Gerrit config for TryBot and resolved comments1w
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,…dmitshur commented on golang.org/x/build/cmd/relui: create internal release branches as part of pre-announce workflow1w
(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.dmitshur commented on golang.org/x/build/cmd/relui: create internal release branches as part of pre-announce workflow1w
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…dmitshur reviewed +2 on golang.org/x/build/internal/task: target US business hours for security releases1w
Thanks.dmitshur commented on cmd/dist: update to make Go 1.17 the minimal Go bootstrap version1w
(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…dmitshur commented on golang.org/x/build: update x/ dependencies in untagged x/ repos when applying tags1w
@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…dmitshur commented on golang.org/x/oauth2: Add error fields to RetrieveError1w
I'm probably not the right person to review this change. @codyoss@google.com, can you suggest a better reviewer? Thanks.dmitshur reviewed +1 on golang.org/x/tools/internal/testenv: add a Command function that replaces exec.Command1w
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.dmitshur commented on github.com/golang/snappy/README: update instructions to install CLI1w
@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…dmitshur opened a change golang.org/x/build/cmd/gerritbot: omit optional since when no comments1w
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 golang.org/x/build/cmd/gerritbot: make sure to update when draft status changes1w
Thanks.dmitshur reviewed +2 on cmd/dist: simplify old code1w
Thanks.dmitshur reviewed +2 on cmd/dist: convert host tests to use goTest1w
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…dmitshur commented on cmd/compile: internal compiler error: missing typecheck [1.19 backport]1w
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…dmitshur commented on cmd/dist: use goTest for manual go test invocations1w
(1 comment)
Makes sense. Thanks for considering both approaches.dmitshur reviewed +2 on cmd/dist: convert most remaining tests to use goTest1w
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.dmitshur reviewed +2 on cmd/dist: make registerTest take a goTest1w
(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.dmitshur reviewed +2 on cmd/dist: use goTest for manual go test invocations2w
(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 …dmitshur reviewed +2 on cmd/dist: introduce "go test" abstraction2w
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…dmitshur commented on golang.org/x/build/cmd/gerritbot: make sure to update when draft status changes2w
@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
dmitshur reviewed +2 on golang.org/x/build/cmd/gerritbot: set Gerrit WIP when GitHub PR is draft2w
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
dmitshur reviewed +1 on cmd/dist: add doc comments to some functions2w
dmitshur reviewed +1 on cmd/dist: add curiously missing GOOS/GOARCH combinations2w
dmitshur commented on golang.org/x/build/cmd/gerritbot: set Gerrit WIP when GitHub PR is draft2w
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.)dmitshur reviewed +2 on golang.org/x/build/cmd/gerritbot: set Gerrit WIP when GitHub PR is draft2w
Thanks.dmitshur reviewed +2 on golang.org/x/build/...: replace linux-arm64-aws with linux-arm642w
(1 comment)
Thanks.dmitshur reviewed +1 on golang.org/x/build/dashboard: promote NetBSD 9.3 as the primary builder version2w
dmitshur commented on golang.org/x/build/cmd/gerritbot: set Gerrit WIP when GitHub PR is draft2w
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…dmitshur commented on golang.org/x/build: Set Gerrit WIP when GitHub PR is draft.2w
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…dmitshur reviewed +2 on golang.org/x/build/dashboard: add NetBSD 9.3 builders2w
Thanks.dmitshur reviewed +2 on golang.org/x/build/...: replace linux-arm64-aws with linux-arm642w
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…dmitshur closed a change crypto/x509: clarify package use-case and implementation reasoning2w
Thanks. Issue #26624 is resolved (via CL 266541), so closing.dmitshur reviewed +2 on cmd/api: point to API docs in all.bash failure2w
Thanks.dmitshur starred github.com/a-h/gpu3w
dmitshur commented on golang.org/x/pkgsite: auto-trigger standard library indexing after official go release3w
Also see #37568.dmitshur commented on golang.org/x/pkgsite: automate fetching of new versions of Go standard library3w
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…dmitshur closed an issue cmd/compile/internal/types2: TestSelf fails if a built pkg/.../syntax.a doesn't exist3w
This might be fixed at tip. The test passes without pkg/darwin_arm64/cmd/compile/internal/syntax.a.dmitshur commented on cmd/compile/internal/types2: TestSelf fails if a built pkg/.../syntax.a doesn't exist3w
Yeah, I think this is fixed via [CL 432535](https://go.dev/cl/432535). Closing and can reopen if we learn otherwise.dmitshur commented on cmd/compile/internal/types2: TestSelf fails if a built pkg/.../syntax.a doesn't exist3w
This might be fixed at tip. The test passes without pkg/darwin_arm64/cmd/compile/internal/syntax.a.dmitshur opened an issue cmd/compile/internal/types2: TestSelf fails if a built pkg/.../syntax.a doesn't exist3w
### 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 …dmitshur commented on github.com/golang/freetype: discrepancy in scale factor conversion between freetype and truetype packages1mo
To update this issue with the latest PR status: the comment above is resolved in https://github.com/golang/freetype/pull/86#issuecomment-970275372.dmitshur closed an issue golang.org/x/build/cmd/coordinator: sometimes TryBot builds fail to schedule a machine and get stuck1mo
dmitshur commented on golang.org/x/build/cmd/coordinator: sometimes TryBot builds fail to schedule a machine and get stuck1mo
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
dmitshur commented on golang.org/x/build/cmd/gerritbot: fails to import some PRs, Gerrit server refuses push due to an account being not found1mo
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…dmitshur commented on golang.org/x/pkgsite: display struct field & interface method doc comments in a less code-ish way?1mo
@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 merged a change github.com/shurcooL/githubv4: Feat/schema202210091mo
dmitshur pushed to
master in github.com/shurcooL/githubv41mo
dmitshur reviewed +2 on github.com/shurcooL/githubv4: Feat/schema202210091mo
Thanks.dmitshur commented on golang.org/x/review/git-codereview: mail requires -trust=false1mo
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…dmitshur opened an issue golang.org/x/build/internal/relui/sign: race in SigningServer.send1mo
`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…dmitshur reviewed +2 on github.com/go-gl/glfw/v3.3/glfw: Regenerate Wayland protocols on Ubuntu 20.041mo
Thanks.dmitshur starred github.com/OldUnreal/UnrealTournamentPatches1mo
dmitshur commented on golang.org/x/build/cmd/gerritbot: fails to import some PRs, Gerrit server refuses push due to an account being not found1mo
This error isn't happening as of yesterday 9:22 am (EDT), both PRs have been updated at around that time.