Activity
Today
dmitshur reviewed +1 on golang.org/x/website/doc/install: fixed how JS defines browser OS51m
(1 comment)
Would it be better to use const here?dmitshur closed an issue golang.org/x/website: Incorrect tar instructions for Linux download55m
dmitshur commented on golang.org/x/website: Incorrect tar instructions for Linux download55m
Thanks for looking into this @ipriver. The underlying problem is a problem in the OS detection code.
Let's close this as duplicate of #41232, and use #41537 to track this issue.Yesterday
dmitshur merged a change all: update vendored dependencies during Go 1.16 development18h
dmitshur commented on all: update vendored dependencies during Go 1.16 development18h
(1 comment)
I'm going to revert the change to src/README.vendor for now (there are further changes to updatestd I want to consider after a discussion with Bryan, so it's too early to be advertising …dmitshur opened an issue x/build/cmd/{relui, releasebot}: consider patch version updates to dependencies that become available before minor Go releases20h
When issuing a minor Go release, we can consider patch updates that may be available to dependencies.
For example, suppose that Go 1.18.3 standard library uses `golang.org/x/mod` at version v1.5.0…dmitshur commented on all: update vendored dependencies during Go 1.16 development22h
(1 comment)
This has changed a bit in PS 3 and PS 4, mind reviewing again please, including the new change to src/README.vendor? Thanks.dmitshur closed a change misc/update: add program to update golang.org/x dependencies23h
dmitshur commented on misc/update: add program to update golang.org/x dependencies23h
Moved the program to x/build, see CL 256357 instead.
Will merge the change to src/README.vendor into CL 255860 instead.dmitshur opened a change golang.org/x/build/cmd/updatestd: add program to maintain standard library modules23h
We need to update dependencies vendored into the standard library at
least two times during each major Go release cycle. We also need to
do this whenever there are fixes backported to release branche…dmitshur reviewed +2 on golang.org/x/build/cmd/gopherbot: address comments from CL 2561791d
(1 comment)
Thanks!dmitshur commented on golang.org/x/build/cmd/gopherbot: label x/pkgsite issues as pkgsite1d
(1 comment)
Two small comments here:
The common style in this file is to check if issue has a label, and then remove it. If you don't do the check, it will write messages like "Issue %d (in maintne…dmitshur commented on golang.org/x/build/cmd/gopherbot: label x/pkgsite issues as pkgsite1d
(1 comment)
Adding it is okay, but please move it into isDocumentationTitle.dmitshur reviewed +2 on golang.org/x/build/cmd/gopherbot: label x/pkgsite issues as pkgsite1d
(3 comments)
Does -dry-run output look okay?
(FWIW, I initially considered suggesting to re-inline this variable (like it was before CL 255939), but it's probably better to keep this as is so that …dmitshur commented on golang.org/x/build/cmd/gopherbot: label x/pkgsite issues as pkgsite1d
(1 comment)
Oh, I see this builds on top and modifies the label.dmitshur commented on golang.org/x/build/cmd/gopherbot: label x/pkgsite issues as pkgsite1d
(1 comment)
How does this relate to CL 255939?Last Week
dmitshur starred in github.com/thommil/tge2d
dmitshur commented on cmd/go: get does not understand go-import meta tag with mod method by default2d
If I understand this issue correctly (also based on reading https://github.com/darkfeline/animanager-go/issues/1), this issue doesn't exist in module mode, only in GOPATH mode.
As a consequence, t…dmitshur commented on misc/update: add program to update golang.org/x dependencies3d
(1 comment)
The test failure is because findGorootModules in moddeps_test.go calls 'go list -json -m' to determine the main module path and dir, but that fails because 'go list -m' tries to compute …dmitshur commented on misc/update: add program to update golang.org/x dependencies3d
(2 comments)
PTAL.
As a note, I'm primarily looking to submit CL 255860 sometime soon (on the order of days is fine), but not in a rush to get this in if you have more feedback or want to look for …dmitshur commented on misc/update: add program to update golang.org/x dependencies3d
(2 comments)
Done in PS 3.
(Luckily both 1.15 and 1.14 release branches are in good shape, so no need to special-case skip it.)
I'll go without vendoring in patch set 3 to show what it looks like.…dmitshur commented on golang.org/x/website: instructions are incorrect for tar.gz download3d
Thanks for reporting. This seems to also apply to Windows. It's possible to select a zip (e.g., go1.15.2.windows-amd64.zip) at https://golang.org/dl/, but the https://golang.org/doc/install#install s…dmitshur commented on misc/update: add program to update golang.org/x dependencies3d
(4 comments)
I considered that but didn't find a reason to include it here at this time. They're partially ignored in the context of packages, but not for module boundaries. E.g., github.com/TarsClo…dmitshur commented on misc/update: add program to update golang.org/x dependencies3d
(1 comment)
I should add that I intentionally used the Gerrit API here to avoid making caching done by the module mirror a factor, or being forced into turning off the module mirror when using the g…dmitshur commented on misc/update: add program to update golang.org/x dependencies3d
(4 comments)
Thanks for the detailed review Bryan.
I'll consider and respond to the rest of the comments later, but I wanted to reply to a subset sooner.
The version selection is done by making a …dmitshur closed a change : [release-branch.go1.13] empty change3d
dmitshur commented on : [release-branch.go1.13] empty change3d
The test is over, this empty CL is not needed now.dmitshur commented on golang.org/x/build/cmd/gopherbot: label x/pkgsite repos as go.dev3d
(1 comment)
s/repos/issues/dmitshur reviewed +2 on golang.org/x/build/cmd/gopherbot: label x/pkgsite repos as go.dev3d
(2 comments)
Should have golang/go# prefix since it's not the main repo.
This expression can be simplified to the constant "true", which I don't think is intended.
It can be fixed in place (e.g., …dmitshur opened an issue cmd/go: tests sometimes fail on linux-386-longtest builder due to no match for panic in stdout3d
The `TestScript/test_exit` test in `cmd/go` failed on the `linux-386-longtest` builder in https://build.golang.org/log/6f34555f06c618921fe4b9e85fbe1522a1a8078e because a panic wasn't seen:
```
--…dmitshur merged a change golang.org/x/perf/vendor: delete4d
dmitshur commented on golang.org/x/perf: vendor directory is out of sync with go.mod file, govendor tool used to generate it is deprecated4d
Submitting this now as I believe it's safe, the newly selected versions are functional. If this does cause some issue, please ping me and I'll look into it.dmitshur commented on golang.org/x/perf/vendor: delete4d
(1 comment)
TRY=longtest, linux-386-longtestdmitshur commented on golang.org/x/perf/vendor: delete4d
(1 comment)
TRY=longtestdmitshur reviewed +2 on golang.org/x/build/devapp/owners: remove hanwen as SSH owner4d
(1 comment)
Thank you for your work as an owner.dmitshur commented on github.com/go-gl/glfw: Bluetooth Xbox One controller throws error4d
Thanks, but I think this issue was already reported earlier in #292.dmitshur commented on misc/update: add program to update golang.org/x dependencies4d
(1 comment)
Done.dmitshur commented on misc/update: add program to update golang.org/x dependencies4d
(2 comments)
This needs a copyright header. The next patch set will add it.
This patch set vendors the single package from x/build here. It makes the CL more verbose.
I've considered the alternati…dmitshur commented on all: update vendored dependencies during Go 1.16 development4d
(1 comment)
TRY=js, s390x, netbsd-amd64, aix, windows-amd64-longtest, darwin-amd64-10_15, linux-amd64-longtest, linux-386-longtestdmitshur opened a change all: update vendored dependencies during Go 1.15 development4d
The Go 1.16 development cycle has started. This is the time to update
all golang.org/x/... module versions that contribute packages to the
std and cmd modules in the standard library to latest master…dmitshur opened a change misc/update: add program to update golang.org/x dependencies4d
We need to update dependencies vendored into the standard library at
least two times during each major Go release cycle. We also need to
do this whenever there are fixes backported to release branche…dmitshur commented on golang.org/x/tools/cmd/bundle: expand test coverage to all modes4d
(1 comment)
I followed the pattern shown in the example at https://pkg.go.dev/golang.org/x/tools/go/packages/packagestest#hdr-Example, and this is commonly done in x/tools (e.g., https://github.com/…dmitshur commented on all: update standard-library dependencies at the start and end of each code freeze4d
> Indirect golang.org/x dependencies are not updated, although they could be. I'm going with the more conservative choice for now. I'm open to feedback on this, and can see pros/cons to both strategi…dmitshur commented on all: update standard-library dependencies at the start and end of each code freeze4d
I'm creating a short program that performs this task. Using a program makes it easier to precisely define our update policy, and adjust it over time as needed. It also makes this task less error pron…dmitshur commented on github: consider downscoping "GitHub Editors" access to Triage role5d
Thanks for the thoughtful feedback @ALTree and others.
Even though the "go-approvers" group is documented as "People who can edit issues in the "go" repo", in reality some people in it take up muc…dmitshur commented on github: consider downscoping "GitHub Editors" access to Triage role5d
Given that we recently went through a change on Gerrit side for #40699 (/cc @rsc @andybons), I wanted to revisit this now. This issue has received favorable feedback so far.
> We should investigat…dmitshur commented on golang.org/x/build/internal: add a shared polling function5d
(1 comment)
(Minor.) A blank line needs to be added above the package clause to prevent the copyright comment from being considered a package comment.dmitshur reviewed +2 on golang.org/x/build/buildenv: remove EC2 zone configuration options5d
dmitshur commented on golang.org/x/website: include release date on dl page5d
> It's not a complete English sentence.
An in-between solution that addresses this could be something like:
"Go {{.V}} was released on {{.Date}}. See the [Release History](https://golang.org/doc/de…dmitshur starred in github.com/u-root/gobusybox5d
dmitshur commented on golang.org/x/website/internal/dl: add release dates to dl page5d
(1 comment)
Thanks for sending a CL. The issue this is fixing is currently in NeedsInvestigation state. I posted a comment there. Let's discuss this and agree we want this change first.dmitshur commented on golang.org/x/website: include release date on dl page5d
Copying a [comment](https://go-review.googlesource.com/c/website/+/254538/1#message-3d30dcbeb58ff148d105f0dc419a1cf3e9284e88) from the CL:
> Here's a screenshot of what the addition to the page lo…dmitshur commented on net/http: bundle invocation without -dst is fragile6d
I don't understand this issue. When the -dst flag is not specified, bundle uses the package in the current working directory. The `go generate` command always sets the working directory to that of th…dmitshur reviewed +2 on golang.org/x/tools/cmd/bundle: use caller's module version via go/packages6d
(2 comments)
Minor and optional suggestion to improve the CLI/clarity of documentation.
Giving an optional flag a non-zero default value feels incompatible with the phrasing on top:
> By default b…dmitshur opened a change golang.org/x/tools/cmd/bundle: expand test coverage to all modes6d
As of CL 189818, bundle has been updated to use the go/packages API to
load packages. That API supports module mode and legacy GOPATH mode.
Update the test to provide coverage for all modes.
Updates…dmitshur commented on golang.org/x/net/http2: http2 healthcheck not in h2_bundle.go6d
My understanding is that the bundled version and vendored version are currently specified independently. The bundled version wasn't updated for the Go 1.15 release.
I think this issue still exists…dmitshur opened an issue net/http: h2_bundle.go (and socks_bundle.go) can get out of sync with src/go.mod's x/net/http2 version6d
We've had issues in the past where version skew could be introduced if people forgot to update a one part of the distribution without also updating another (e.g., versions in src/go.mod and src/cmd/g…dmitshur reviewed +2 on golang.org/x/build/devapp/owners: add cagedmantis to x/build owners6d
(1 comment)
Thanks for adding yourself!dmitshur reviewed +2 on golang.org/x/tools/godoc: convert Markdown files to HTML during serving6d
(3 comments)
I think this is a great change. There's more we can do later, but this should work well.
Once this is pulled into x/website, this will also expose some .md files in the main Go tree, b…dmitshur commented on github.com/shurcooL/graphql: Ability to add custom headers to the request1w
@gurpreetz Thanks. That should not be happening, I'll need to investigate. Can you please file a new issue at https://github.com/shurcooL/githubv4/issues and specify what version of `golang.org/x/oau…dmitshur opened an issue github.com/fydy/mirror_issue: package githubv4 is imported at old import path1w
https://github.com/fydy/mirror_issue/blob/0b2042527c6dd994d2a512b2dd61623468975330/issues/source.go#L8
The canonical import of that package [has changed](https://github.com/shurcooL/githubv4/issue…dmitshur commented on github.com/shurcooL/graphql: Ability to add custom headers to the request1w
> I was already using those methods to authenticate when Github flagged it as soon to be deprecated - hence the request.
Can you please elaborate? Can I reproduce this somehow?
I've looked at h…dmitshur commented on github.com/shurcooL/vfsgen: Add option to generate a function rather than global variable1w
The best practice recommends avoiding global variables when it is possible and when it makes sense. It doesn't seem very applicable in this situation.
To answer your question, let me be more preci…dmitshur pushed to
master in github.com/shurcooL/githubv41w
dmitshur merged a change github.com/shurcooL/githubv4: Adding new github doc links1w
dmitshur reviewed +2 on github.com/shurcooL/githubv4: Adding new github doc links1w
LGTM. Thanks.dmitshur pushed to
master in github.com/shurcooL/githubv41w
dmitshur merged a change github.com/shurcooL/githubv4: regenerate for schema changes by 2020-09-141w
dmitshur reviewed +2 on github.com/shurcooL/githubv4: regenerate for schema changes by 2020-09-141w
Thanks. LGTM.
> Reference: https://developer.github.com/v4/changelog/.
It looks like https://docs.github.com/en/graphql/overview/changelog is a [better](https://github.blog/2020-07-01-launching…dmitshur commented on cloud.google.com/go/feat(godocfx): create Go DocFX YAML generator1w
FYI, there is a tracking issue golang/go#39883 for making that API available to for external use; mentioning it here so that this use case can be considered as part of that work. You may also want to…dmitshur commented on all: add macOS ARM64 port1w
Thanks for making this proposal. This corresponds to the following requirements in https://golang.org/wiki/PortingPolicy#requirements-for-a-new-port:
- A proposal must be filed and accepted in whi…dmitshur commented on golang.org/x/review/git-codereview: clean up usage message, docs1w
(1 comment)
Thanks for the notice.dmitshur reviewed +2 on golang.org/x/review/git-codereview: clean up usage message, docs1w
(3 comments)
Can add:
Fixes golang/go#34378.
Something looks off here.
[-nokeycheck] is repeated twice on this line.dmitshur commented on all: add GOOS=ios1w
(3 comments)
Is it in scope of this CL to update this documentation?
(In case it is helpful, take a look at a past CL where this was done, https://go-review.googlesource.com/c/go/+/239278/1/doc/ins…dmitshur reviewed +2 on misc/ios: quote paths1w
dmitshur commented on golang.org/x/review/git-codereview: add mail -trust to set Trust+1 vote1w
(2 comments)
Should [-trust] be added here?
Should [-trust] be added here?Earlier
dmitshur commented on github.com/shurcooL/vfsgen: Add option to generate a function rather than global variable1w
Can you please expand a bit on the motivation? What would you do differently if this were implemented?
Is this a feature request to be able to defer the cost of initialization of the virtual filesys…dmitshur commented on github.com/shurcooL/githubv4: Adding new github doc links1w
Thanks. The changes in the README.md file look good. Can you please also update one URL in doc.go?dmitshur reviewed +1 on golang.org/x/tour/content: link to working and updated korean translation1w
(1 comment)
I can reproduce an error when clicking the link above. It seems to happen only when visiting the URL with http:// scheme. There's no issue when using https://.
A screenshot of the probl…dmitshur closed a change testing: avoid registering flags except in test binaries1w
dmitshur commented on testing: avoid registering flags except in test binaries1w
I'll close this CL since issue #21051 has been fixed via CL 173722 (as Caleb noted above), so this CL is no longer needed.
If there's something left to do here, please feel free to re-open.dmitshur reviewed +2 on golang.org/dl/gotip: disable GC while building1w
(2 comments)
This is a change that has a chance to cause issues in some environments, either right away, or perhaps in the future. It's worth having an issue so people can find it and comment on it …dmitshur reviewed +2 on golang.org/x/website/cmd/golangorg: add -a flag to automatically find templates1w
(1 comment)
I don't know if you still want to proceed with this, but if so, this looks good.
We can always remove this flag if it becomes no longer useful thanks to other future improvements.dmitshur commented on golang.org/x/xerrors/all: defer to fmt.Errorf for Go 1.13 and above1w
(3 comments)
Is it viable to also do this for Is, As, and Unwrap (from errors package in Go 1.13's standard library)? Is it out scope for this CL?
A copyright comment is missing.
A small nit on th…dmitshur commented on golang.org/x/playground: faketime headers not stripped from os/exec subprocess output1w
I understand this issue is related to but not the same as #38751. The difference here is that it happens specifically for subprocess output even when the output is not large. Is that right?dmitshur commented on all: add openbsd/mips64 port1w
@4a6f656c I’m not sure. According to https://github.com/golang/proposal#proposal-review, proposal review meetings happen approximately weekly, but this week may be unusual.
@andybons Do you have…dmitshur commented on runtime: pcdata is -2 and 12 locals stack map entries error on nil pointer [1.15 backport]1w
@randall77 For a backport issue to get to CherryPickApproved state, someone on the release team needs to approve it. We've discussed this issue in past release meetings but a decision hasn't been mad…dmitshur commented on net/http/cgi: Cross-Site Scripting (XSS) when Content-Type is not specified1w
This issue was open because [CL 252179](https://golang.org/cl/252179) has not been submitted yet. I'll reopen to track that (otherwise we risk shipping Go 1.16 without the fix).
I left a ping commen…dmitshur commented on net/http/cgi: add Content-Type detection1w
(1 comment)
Friendly ping on this CL. It has a +2 and one unresolved minor comment. It may need to be rebased because of a merge conflict.dmitshur merged a change runtime: [release-branch.go1.14] fix ReadMemStatsSlow's and CheckScavengedBits' chunk iteration1w
dmitshur merged a change runtime: [release-branch.go1.15] fix ReadMemStatsSlow's and CheckScavengedBits' chunk iteration1w
dmitshur merged a change cmd/internal/obj: [release-branch.go1.14] fix inline marker issue on s390x1w
dmitshur merged a change cmd/internal/obj: [release-branch.go1.15] fix inline marker issue on s390x1w
dmitshur reviewed +1 on cmd/internal/obj: [release-branch.go1.14] fix inline marker issue on s390x1w
dmitshur reviewed +1 on cmd/internal/obj: [release-branch.go1.15] fix inline marker issue on s390x1w
(1 comment)
The backport and removal of the compiler check look good to me, but I'd like someone more familiar with the compiler to review it.dmitshur commented on plugin: program on linux/s390x sometimes hangs after calling "plugin.Open" [1.15 backport]1w
I've answered more generally on the thread. The two backport CLs need to be reviewed, as they can't be submitted without a +2. Could you please find a code reviewer for them? Thank you.dmitshur opened an issue golang.org/x/build: consider pushing open backport issue milestones automatically at start of each month1w
Right now, the milestones of open backport issues that did not get included in a given minor release are automatically bumped to the next minor release milestone by `releasebot` at the end of a succe…dmitshur commented on runtime: "fatal error: unexpected signal during runtime execution" on windows-amd64-longtest builder of Go 1.15.2 commit [1.14 backport]1w
@mknyszek Not a problem!dmitshur commented on runtime: "fatal error: unexpected signal during runtime execution" on windows-amd64-longtest builder of Go 1.15.2 commit [1.14 backport]1w
Approved as this is a test fix, needed to reduce false-positives during pre-release testing. This backport applies to both 1.15 (#41317) and 1.14 (this issue).dmitshur commented on runtime: "fatal error: unexpected signal during runtime execution" on windows-amd64-longtest builder of Go 1.15.2 commit [1.15 backport]1w
Approved as this is a test fix, needed to reduce false-positives during pre-release testing. This backport applies to both 1.15 (this issue) and 1.14 (#41322).