Activity
Yesterday
dmitshur commented on runtime: timeout in TestNeedmDeadlock on darwin-amd64-10_12 builder1d
macOS 10.12 is no longer supported by any supported Go release, and the builder was dropped in [CL 419194](https://go.dev/cl/419194). Let's close this @bcmills?dmitshur commented on golang.org/x/website/_content/blog: defaulting to system theme1d
Thanks for reporting this Todd. The need for images to handle both light and dark modes applies to content on the entire Go website, including documentation, tutorials, etc. There've been some previo…dmitshur closed an issue golang.org/x/website: Bitly case study page returning 403 for images1d
dmitshur commented on golang.org/x/website: Bitly case study page returning 403 for images1d
This seems to be resolved now:
<img width="641" alt="image" src="https://user-images.githubusercontent.com/1924134/192028154-d974575b-b133-4ff6-bd5e-ad229c1e433e.png">dmitshur commented on golang.org/x/website/_content/doc/install: add link to the minimum requirements wiki page1d
Sounds good, thanks.dmitshur closed an issue golang.org/x/build: document that Ubuntu 14.04 doesn't meet minimum requirement for Go 1.16 binary release1d
dmitshur commented on golang.org/x/build: document that Ubuntu 14.04 doesn't meet minimum requirement for Go 1.16 binary release1d
By now the two [supported](https://go.dev/doc/devel/release#policy) Go releases are 1.19 and 1.18, so we'll not be making 1.16-specific changes. Issue #41212 tracks general improvements to how we tra…dmitshur commented on golang.org/x/website: broken link to system requirements1d
The old requirements page was out of date and intentionally removed, as described in https://github.com/golang/go/issues/41212#issuecomment-686773546.
[CL 424575](https://go.dev/cl/424575) removes t…dmitshur starred github.com/Rezmason/matrix1d
This Week
dmitshur pushed to
master in github.com/shurcooL/githubv42d
dmitshur merged a change github.com/shurcooL/githubv4: regenerate for schema changes by 2022-09-162d
dmitshur reviewed +2 on github.com/shurcooL/githubv4: regenerate for schema changes by 2022-09-162d
Thanks.dmitshur opened an issue golang.org/x/build: have darwin/amd64 builders on macOS 13 (Ventura)2d
This is the tracking issue like #55354, but for amd64.dmitshur opened an issue golang.org/x/build: have darwin/arm64 builders on macOS 13 (Ventura)2d
Apple announced macOS 13 Ventura during WWDC 2022 earlier in the year. Its [preview page](https://www.apple.com/macos/macos-ventura-preview/) currently says "Coming this October", which is coming up.…dmitshur commented on golang.org/x/time/x/time/rate: add TokenAt and Tokens methods to Limiter.2d
(2 comments)
The "x/time/" part is only for issues and not used in CL subjects, just "rate" is better. (See https://go.dev/wiki/CommitMessage#other-repos.)
This needs to be fully qualified like gol…dmitshur commented on golang.org/x/build/internal/relui: TestRelease failures2d
This seems to be the tracking the same problem as issue #53972 (CC @heschi). We think this might be fixed as of Sept 9 (see https://github.com/golang/go/issues/53972#issuecomment-1241117375).
The …dmitshur reviewed +2 on golang.org/x/build/dashboard: remove TRY=mac and macos2d
Thanks.dmitshur reviewed +2 on golang.org/x/build/cmd/buildlet/stage0: add freebsd-riscv64-unmatched2d
Thanks.dmitshur pushed to
master in github.com/gopherjs/gopherjs3d
dmitshur merged a change github.com/gopherjs/gopherjs/compiler/natives: use embed package3d
dmitshur commented on golang.org/x/vuln/cmd/govulncheck: document that we only report for current GOOS/GOARCH5d
(1 comment)
There are two Change-ID footers (on line 9 and 13).dmitshur commented on golang.org/x/vuln/client: use hash for index5d
(1 comment)
There are two Change-ID footers (on line 29 and 33).dmitshur commented on golang.org/x/tools/gopls/internal: new LSP stub generator replaces typescript code5d
(1 comment)
You might be aware by now, "aux" is one of reserved names on Windows and can't be used by Go packages that aim to support that OS. That's causing the windows TryBot failures.dmitshur commented on golang.org/x/build: no error for missing file in windows workspace5d
Notes from a quick look.
It is failing on the windows-amd64-2016 TryBot:
```
builder: windows-amd64-2016
rev: 706d84fca2b36fdf670a0d921e6a8a3b481eaa05
buildlet: http://10.128.0.48 G…dmitshur reviewed +2 on golang.org/x/build/internal/task: integration test for x repo tagging5d
(1 comment)
Doing the test error checking here rather than at the caller sounds very reasonable to me. I trust you to pick whichever style works better in this context.dmitshur reviewed +2 on golang.org/x/build/internal/task: finish initial implementation of x/ repo tagging5d
Thanks.dmitshur commented on cmd/api: make check pickier about api/*.txt5d
Thanks for making this better. See inline comments for minor problems that need a resolution.
fail was used to exit with exit code 1 when compareAPI failed. Maybe it should be replaced entirely by t…dmitshur reviewed +2 on golang.org/x/build/internal/task: integration test for x repo tagging5d
Thanks.
Is this word now extra?
s/serveTarballs/ServeTarball/
'&& !opts.SystemLevel' maybe can be added here if you'd find that helpful.
If you think it can be helpful for debugging, you could al…dmitshur commented on golang.org/x/build/internal/task: finish initial implementation of x/ repo tagging5d
(One more thing spotted while reviewing the next CL in the stack, in case it's helpful to you.)
When there are no dependencies, both "empty go.sum file" and "no go.sum file" are considered tidy (see…dmitshur closed an issue cmd/go: "mod tidy" doesn't delete go.sum when go.mod has no dependencies5d
dmitshur commented on cmd/go: "mod tidy" doesn't delete go.sum when go.mod has no dependencies5d
I suspect this is the same as #36236. (Please comment otherwise.)dmitshur reviewed +2 on golang.org/x/build/internal/task: finish initial implementation of x/ repo tagging5d
In general this looks great, thanks for implementing a large part of this complex task here. This LGTM after you have a chance to see and deal with inline comments. It's fine for some of them to be c…dmitshur commented on golang.org/x/tools/...: use constant to avoid repeated definitions5d
Thanks.dmitshur commented on golang.org/x/tools/...: use constant to avoid repeated definitions5d
(2 comments)
Thanks for clarifying.
The change here should probably be undone. This is a testdata file that happens to have a .go extension, and so code simplification shouldn't be applied to it as…dmitshur commented on golang.org/x/tools: all：use constant to avoid repeated definitions6d
(1 comment)
The use of the fullwidth colon (U+FF1A) differs from the usual ASCII colon followed by a space that most Go commits use. It seems okay, though tooling may need to take it into account if…dmitshur commented on github.com/gopherjs/gopherjs/compiler/natives: simplify file embedding by using embed package, if it's a good fit6d
Thanks. I've made PR #1151 that takes care of `natives`. The rest can be future work.dmitshur opened a change github.com/gopherjs/gopherjs/compiler/natives: use embed package6d
For #1149.dmitshur opened an issue github.com/gopherjs/gopherjs/compiler/natives: simplify file embedding by using embed package, if it's a good fit6d
GopherJS relies on vfsgen to embed some files. vfsgen predates Go's embed package, so I wanted to see if it's no longer needed. I tried it out on the [`embed-natives`](https://github.com/gopherjs/gop…Last Week
dmitshur commented on cmd/dist: remove go1.4 code path1w
> cmd/dist still needs to be built with go1.4
As I understand it can and is built with Go 1.17.13, perhaps this change is doing something wrong?dmitshur commented on golang.org/x/build/internal/task: finish initial implementation of x/ repo tagging1w
Posting what I have so far, will finish the review soon thereafter.
Minor: strings.Count(...) == 0 can be slightly simplified to !strings.Contains(...).
(No need to change anything if you're intent…dmitshur reviewed +2 on golang.org/x/build/maintner/maintnerd/maintapi: validate page size in mixed mode1w
Maybe I haven't looked too much into what "mixed" does exactly—I was just basing that on it not supporting pagination. I figured it not supporting pagination was because it it can return a bounded …dmitshur opened a change golang.org/x/build/cmd/updatestd: print stderr in runOut when command fails1w
Stderr is much more likely to contain useful information than
stdout in the unexpected case of the command failing. Show it.
For golang/go#55067.dmitshur commented on golang.org/x/build/maintner/maintnerd/maintapi: validate page size in mixed mode1w
I'm not sure how hard it is to figure out the minimum correct value, but it's not as easy as counting branches, and I don't know that it's worth the effort to figure out... Your call.
I think it's m…dmitshur reviewed +2 on golang.org/x/build/cmd/coordinator/internal/legacydash: uncap maintner request1w
Setting MaxCommits to a low number on a non-paginated request is indeed unhelpful; thanks for fixing.dmitshur closed an issue golang.org/x/build/cmd/relnote: look for CLs that had revert CLs sent (but abandoned)1w
dmitshur commented on golang.org/x/build/cmd/relnote: look for CLs that had revert CLs sent (but abandoned)1w
I don't recall seeing many other instances where this would've helped, and I suspect the noise to signal ratio would be too low, so this is unlikely worth doing. Closing for now.dmitshur commented on golang.org/x/build/internal/task: finish initial implementation of x/ repo tagging1w
Still looking, posting partial comments pointing out various minor things I spotted so far.
Move this above the status code check.
s/Go/semver/
Also on line 391.
Without a "." after %s here or an…dmitshur commented on net/http/httputil: forward 1xx responses in ReverseProxy1w
Thanks.dmitshur commented on cmd/go: enable -msan on freebsd/amd641w
We're in 1.20 dev window now, so the Hold+1 can be removed if that was the only reason for it.dmitshur commented on regexp: allow patterns with no alternates to be one-pass1w
We are indeed in 1.20 dev window now, thanks for the ping. I'll let Russ remove it since he added it and is also the reviewer on this CL.
Proposal #21463 mentioned in the commit message isn't in acc…dmitshur reviewed +2 on golang.org/x/build/internal/task: start x repo tagging tasks1w
(1 comment)
I thought about that too but didn't mention it (wasn't sure that it'd be worth the overhead compared to just adding the comment—it's an internal implementation detail and both paths ar…dmitshur reviewed +2 on cmd/go/testdata/script: remove reference to fixed issue1w
Thanks.dmitshur reviewed +2 on path/filepath: document that WalkDir uses OS paths1w
Thanks.dmitshur closed a change golang.org/dl/...: add go1.17.121w
Duplicate of CL 417135.dmitshur closed a change golang.org/dl/...: add go1.18.41w
Duplicate of CL 417134.dmitshur closed a change golang.org/dl/...: add go1.19rc11w
Duplicate of CL 416274.dmitshur closed a change golang.org/dl/...: add go1.19beta11w
Duplicate of CL 411416.dmitshur closed a change golang.org/dl/...: add go1.19beta11w
Duplicate of CL 411416.dmitshur closed a change golang.org/dl/...: add go1.19beta21w
This was a test, no longer needed.dmitshur closed a change golang.org/dl/...: add go1.191w
Duplicate of CL 420557.dmitshur closed a change golang.org/dl/...: add go1.191w
Duplicate of CL 420557.dmitshur commented on golang.org/x/build/...: remove legacy gomote API and SSH implementation1w
(1 comment)
Readded in CL 430695.dmitshur commented on golang.org/x/build/internal/relui: buildrelease_test fails with flake on uuid length1w
Commit golang/build@656fd833c86463e51c5e722b28134318ab25bf6a is just before [CL 429536](https://go.dev/cl/429536), so there's no new action take here yet.dmitshur commented on golang.org/x/build/...: remove legacy gomote API and SSH implementation1w
Nice removal!
Is this removed intentionally? httpRouter was related to the legacy gomote API, but hostPathHandler doesn't seem to be.dmitshur reviewed +2 on golang.org/x/build/cmd/gopherbot: change label for vulncheck and vulndb issues1w
Thanks.
This label doesn't exist in the main repo right now, but "vulncheck or vulndb" exists (the two words are just in the other order). One or the other should be renamed so they match.dmitshur reviewed +2 on golang.org/x/build/cmd/gopherbot: default x/vuln{db} issues to vuln/unplanned milestone1w
(1 comment)
288 perhaps? 287 looks like "vuln/2022" instead of "vuln/unplanned".dmitshur commented on github.com/gopherjs/gopherjs/README: suggest go install for installing1w
Thanks for reviewing!dmitshur pushed to
embed-natives in github.com/gopherjs/gopherjs1wdmitshur pushed to
embed-natives in github.com/gopherjs/gopherjs1wdmitshur pushed to
embed-natives in github.com/gopherjs/gopherjs1wdmitshur created branch in github.com/gopherjs/gopherjs1w
embed-nativesdmitshur opened a change github.com/gopherjs/gopherjs/README: suggest go install for installing1w
GopherJS gained support for building code in module mode in 2021 (see PR #1042), and it itself didn't have problems being built in module mode since much earlier.
Recently, it started getting semver…dmitshur created branch in github.com/gopherjs/gopherjs1w
README-go-installEarlier
dmitshur commented on dmitri.shuralyov.com/idiomatic-go: Use defer if its overhead is relatively negligible, or it buys meaningful readability1w
I agree with the points made above. This entry tried to say "use this language feature when it's appropriate; don't use it when it's not", which can be said for any language feature. I'll close this …dmitshur pushed to
master in github.com/shurcooL/frontend2wdmitshur merged a change all: [release-branch.go1.18] upgrade golang.org/x/net to v0.0.0-20220907013725-0a43f88f7ef02w
dmitshur merged a change all: [release-branch.go1.19] upgrade golang.org/x/net to v0.0.0-20220907013720-d52c520e37662w
dmitshur commented on golang.org/x/build/internal/relui: use atomic writes in test code too2w
(1 comment)
Or maybe it's not a problem because ReadDir implementations are required to return io.EOF at the end, so the caller can be looking at that.
Maybe the only case you're not handling corre…dmitshur reviewed +2 on golang.org/x/build/internal/relui: use atomic writes in test code too2w
Thanks.
Would it be simpler to write temporary files to another directory (rather than as dot files), so they can't get in the way trivially without complicating ReadDir?
s/ioutil/os/ to avoid rein…dmitshur reviewed +2 on golang.org/x/build/internal/relui: enable retries for source match2w
Nice catch. Good counter-example to the mailing tasks where we intentionally wanted to separate out the emailing step so it'd be "locked in" as soon as it completed successfully. Thanks.
I take it C…dmitshur reviewed +2 on golang.org/x/website/internal/history: remove redundant type conversion2w
dmitshur commented on golang.org/x/website/internal/backport/go/build: remove unnecessary copies of package variables2w
> this simplification can be applied to the real go/build package too
(Was already done in CL 429636, thanks.)dmitshur reviewed +2 on go/build: remove unnecessary copies of package variables2w
Thanks.
If you think it's worth doing, maybe also drop the 'b' prefix since 'plusBuild' happens not to collide with anything, and the prefix will make less sense next to slashSlash, etc.dmitshur reviewed +2 on golang.org/x/website/internal/history: remove redundant type conversion2w
Thanks.dmitshur reviewed +2 on golang.org/x/website/internal/backport/go/build: remove unnecessary copies of package variables2w
Thanks. It seems this simplification can be applied to the real go/build package too, and it'll last longer if sent there. (This is an internal backport copy that will eventually be deleted, once the…dmitshur commented on lib/time: update tzdata before release2w
Go tip is currently at 2022b, which includes the mentioned Chile DST start change, and 2022c is [described](http://mm.icann.org/pipermail/tz-announce/2022-August/000072.html) to work around an awk bu…dmitshur commented on golang.org/x/build: some Gerrit CLs are getting into a state where they can't be updated3w
We ran into this in https://go.dev/cl/231464 (created in May 2020), reported by @ianthehat recently.dmitshur commented on all: add freebsd/riscv64 port3w
Is it right for this issue to be closed now? `go tool dist list` at tip doesn't include freebsd/riscv64.dmitshur starred github.com/ange-yaghi/engine-sim3w
dmitshur reopened an issue golang.org/x/net: missing 'net' import breaks z/OS build4w
dmitshur commented on golang.org/x/build/cmd/gopherbot: -2 any CLs with large binary blobs unless declared in commit message4w
Another instance in https://go-review.googlesource.com/c/net/+/425534/2#message-141a8f089a209ce957040ffea9e117baa773736a.dmitshur commented on net: WriteMsgUDPAddrPort should accept IPv4 destination addresses on IPv6 UDP sockets1mo
The status of #54074 is that it's approved, but a cherry-pick CL hasn't been made yet. I made [CL 425488](https://go.dev/cl/425488) following https://go.dev/wiki/MinorReleases#making-cherry-pick-cls.…dmitshur closed an issue golang.org/x/net/http2: Add `DialTLSContext` to `http2.Transport`1mo
dmitshur commented on golang.org/x/net/http2: Add `DialTLSContext` to `http2.Transport`1mo
Closing as a duplicate of #52114, which has now been resolved.dmitshur commented on github.com/shurcooL/githubv4: query pull request struct1mo
The query struct fields likely need to be exported (that is, begin with an upper-case letter) to be writeable by the githubv4 package. Does it help if you try that?
```diff
type LoopStruct struct…dmitshur commented on access: gomote1mo
CC @joedian, @cagedmantis.dmitshur edited the wiki1mo
dmitshur opened an issue internal/poll: can update newPipe to take advantage of newer Linux kernel requirement1mo
`newPipe` currently contains this comment:
```
// pipe2 was added in 2.6.27 and our minimum requirement is 2.6.23, so it
// might not be implemented. Falling back to pipe is possible, but prior …dmitshur commented on golang.org/x/build/env/linux-ppc64le/osuosl: update builder to Debian Bullseye1mo
> Would it be ok to move to the latest LTS?
I think that should be okay.