Activity

Today
It was previously `48e542e58588bc75faacd63439f40e57db80b2f1` at the time of the force push. From looking, the only CL I see dropped was [CL 258437](https://golang.org/cl/258437). It was re-sent as […
(1 comment) Thanks for catching this and re-sending.
dmitshur reviewed +1 on doc: fix typo in contribute.html58m
(1 comment) I think a smaller fix would've been to s/uses/use/. This version seems good to me too. Moving "This will leave a comment in the issue ..." on a new line was done intentionally in CL 11…
The error happens during building of the `github.com/go-gl/glfw/v3.3/glfw` package. I guess `gollvm` is not supported. The error message "PLEASE submit a bug report to https://bugs.llvm.org/ and in…
Yesterday
(1 comment) This is indeed incomplete. I wrote about what needs to be done at https://go-review.googlesource.com/c/go/+/258540/1#message-7a7fbd2e9357329b1bd7517c398e32519c9c7629.
(2 comments) After CL 258478 is submitted (assuming the backport issue gets approved), this CL can be created using the usual sequence: go get -d golang.org/x/net@release-branch.go1.15 go m…
(1 comment) Thanks for reviewing. This CL can only be submitted after issue #41387 goes into CherryPickApproved state. If that backport issue is not accepted, we will close this CL and not proceed.…
When working on branches other than the main one, or making cherry-pick CLs, we have a convention of putting the branch name in square brackets before the subject. From https://golang.org/wiki/Min…
This is now done. If you've submitted a CL to master branch recently and it's not there now, please re-send it.
The merge [CL 258525](https://golang.org/cl/258525) was accidentally merged into the master branch, rather than the intended branch. I'll rewrite history to revert master to the parent commit `846…
(1 comment) Should this also be checked, or is okay not to? !c.tls.IsZero() && ... Another question, can it be used in place of checking that the port is exactly 443, or are there scenarios wh…
When the body.Write fails during processData, the connection flow control must be updated to account for the data received. The connection's WINDOW_UPDATE should reflect the amount of data that was n…
When the body.Write fails during processData, the connection flow control must be updated to account for the data received. The connection's WINDOW_UPDATE should reflect the amount of data that was n…
(1 comment) Please add "For #40423." or equivalent here. (It's already mentioned above on line 12, but having it here helps see the main issue associated with this CL.)
Also see #149. (There's no overlap between the two.)
dmitshur deleted branch in golang.org/x/pkgsite1d
rm-todo
This Week
(1 comment) (Trivial.) Since State in _windows.go got it, should this get a "// State contains the state of a terminal." comment too?
(1 comment) Please include some additional information here to make navigating code history easier in the future. See the "when moving code between repos, ..." bullet point at https://golang.org/wik…
(1 comment) I see. Thanks. I'll stop it now.
(1 comment) I'll keep as is since this is consistent with the rest of the codebase and a minor point.
(1 comment) I made the original suggestion because the current state seemed to me like it would be confusing to users, but after spending more time thinking about it, I no longer think that. So kee…
My 👍 reaction was to indicate that I thought it was a good explanation of why it is a good change that we should make. I'm convinced that adding the release date to the https://golang.org/dl/ …
This task was resolved via declVisitor in commit c29176c6819e2879ed1a47f13ce56ec79ca5923b.
This task was resolved via declVisitor in commit c29176c6819e2879ed1a47f13ce56ec79ca5923b.
dmitshur created branch in golang.org/x/pkgsite2d
rm-todo
@trongbq Thank you for investigating this. This godoc behavior is called a "section heading" and it is currently best documented in the second paragraph of [`"go/doc".ToHTML`](https://golang.org/pkg…
(1 comment) I expect it'll fail on the ios (darwin-arm64-corellium) builder because issue #41610 is not resolved yet, but I wanted to start a slowbot run to see how exactly it'll fail. TRY=darwin-a…
> We may want to repurpose or clone the existing darwin-arm64-corellium builder for this. This seems to be our best bet so far. @eliasnaur Are you able to assist with this transition? @steeve F…
Support for GO386=387 is being dropped in Go 1.16. There is no need to select this target and test it anymore. For #40255.
dmitshur opened a change src/buildall.bash: remove mobile filter2d
DO NOT REVIEW: This is expected to fail trybots before CL 258057 is submitted and deployed. Mobile targets are not supported by misc-compile trybots, as tracked in issues #25963, #35596, and need to…
Since sending CL 257617, I've learned that iOS was never a supported target for misc-compile. It was always filtered out unconditionally in GOROOT/src/buildall.bash, and the comment suggesting that w…
dmitshur pushed to master in github.com/shurcooL/githubv43d
d292edc3691bf39212dabd7fc79b0493cdd638fbremove outdated status note
dmitshur pushed to master in github.com/shurcooL/graphql3d
18c5c3165e3af7c70ca625593f8066462ad684ccremove outdated status note
410806f8a482d6c2b70e8cacafe3f1aee179cdef.travis.yml: use go vet instead of go tool vet
Last Week
dmitshur reviewed +2 on all: enable more tests on macOS/ARM645d
(2 comments) I think it's helpful to include a mention to a relevant issue for tracking purposes. Maybe this GOARCH check can be removed for consistency with elsewhere (can replace ios/arm64 with "…
dmitshur reviewed +2 on vendor: update vendored x/sys and x/net5d
(1 comment) Maybe add mention for the tracking issue here.
Create the builder as a post-submit builder with a non-zero KnownIssue value first. Once we know it works well, we'll have the confidence to be able to safely promote it to a normal TryBot, without t…
@mvdan From https://golang.org/wiki/MinorReleases: > Gerrit is configured to only allow release managers to submit to release branches, Submitting the CL is a step that release managers are respons…
(3 comments) A question. There's only one type implementing this interface. Is it expected that there'll be more in some future, justifying the need for creating an interface? If not, I'd nudge tow…
(1 comment) Is using NewUUID (UUID version 1) instead of New (UUID version 4) an intentional decision, despite the "In most cases, New should be used." suggestion in the uuid package doc? If so, th…
(4 comments) Optional comments, I don't think any of them are worth acting on. Was this addition meant to be in CL 257239 instead? (This got resolved in patch set 4.) Consider documenting that it…
> The solution here should be to remove <code> tags from the code examples which are used inside <pre> tag at the https://golang.org/ref/mod page itself. As far as I know, the <code> tags in <pre>…
I think to start it would be sufficient to turn this issue into a proposal by adding the Proposal label, as described in step 1 of https://github.com/golang/proposal#the-proposal-process. That can be…
It seems this has already been done for Windows 3 years ago. See issue #23025 and [CL 104115](https://golang.org/cl/104115). Going to look into the current state of the macOS installer next.
Yes, I can take this.
We are aware that a builder is missing, and have the tracking issue golang.org/issue/41610 for resolving that. Add a skip to the tests so that the build dashboard is green until the work of adding th…
CL has added GOOS=ios to the output of `go tool dist list`. `TestTryBotsCompileAllPorts` in x/build/dashobard is failing because we don't yet have a builder (real or misc-compile one) covering the ne…
@jayconrod Friendly ping on this. Does it sound good to you to move this issue to NeedsFix for 1.16 milestone? I'll also /cc @stevetraut who's been working on improving the installation experience…
I've confirmed this happened on the 1.15 release branch because we have had flexibility between what version of x/net module is vendored, and what version of packages from x/net are bundled. Differen…
> Right now, it's possible to update the bundled copy independently from the src/go.mod version, but it's not clear that flexibility is something we want to keep. Discussion so far suggests we do …
Thanks for answering @bcmills. Based on those answers, I think we want a normal test that is skipped in -short test mode (so it only runs on longtest builders or when executed locally manually). A…
dmitshur commented on all: add GOOS=ios1w
(1 comment) RELNOTE=yes
[CL 256357](https://golang.org/cl/256357) has updated golang.org/x/... packages, and [CL 248499](https://golang.org/cl/248499) (/cc @hyangah) will update the remaining packages (pprof and demangle). …
(5 comments) I created this CL because I wanted to get code review on this script, which I made primarily to generate CL 255860 (and have something semi-reproducible to discuss afterwards). I got go…
Patch set 1 of [CL 256357](https://golang.org/cl/256357) implemented a per-module update strategy for golang.org/x dependencies in the std and cmd standard library modules. At a high level: 1. Det…
dmitshur reviewed +2 on doc: update overview for authentication1w
This issue affects Chrome on macOS too: ![image](https://user-images.githubusercontent.com/1924134/94039921-5028e300-fd96-11ea-8311-8cf2fd349d5f.png) There's some relevant CSS: ``` code { …
(1 comment) I filed #41578 about fixing the builder.
In order to unblock [CL 244057](https://golang.org/cl/244057) (which fixes #39945), we need to rebuild the android-amd64-emu builder image given that [CL 247357](https://golang.org/cl/247357) has bee…
> But I just tried it and couldn't get it to work, so please let us know if you find a working solution. @gmlewis In what way did it not work? A require directive should work to ensure the mention…
dmitshur commented on cmd/vendor: sync pprof@1a94d8640e991w
(1 comment) s/Updates/Fixes/ since it seems this CL resolves that issue fully. (If there's more to do, "Updates" is fine; please update that issue with what still needs to be done.) Also consider a…
dmitshur reviewed +2 on cmd/vendor: sync pprof@1a94d8640e991w
(3 comments) See one comment about go.sum, and an optional comment about intelSyntax. Please remove this line. It's no longer needed, and go mod tidy removes it. This CL was rebased on top of CL 25…
I've added the label to a few open issues where it seemed fitting. We can keep doing that in the future. Closing since this is done.
It's true that the implementation for this is fairly small and easy. However, once we add support for this, it will become hard (nearly impossible) to remove and we'll need to maintain it indefinitel…
(1 comment) Please see https://groups.google.com/d/msg/golang-dev/jgkuktb_m7A/oLOSzYZvAwAJ. It's a change to improve the security of the contribution process, there are some minor changes to take in…
Thanks for reviewing. I'll close this for now. May re-open (or re-send) if this becomes needed again.
(1 comment) Hmm, I see more clearly now that demangle is a dependency of github.com/google/pprof, which is used directly by cmd/pprof and cmd/trace. So it probably makes more sense to update to a n…
Update demangle to the latest published version during Go 1.16 development. $ go get -d github.com/ianlancetaylor/demangle@latest github.com/ianlancetaylor/demangle latest => v0.0.0-20200824232613…
(1 comment) Would it be better to use const here?
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.
(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 …
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…
(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.
Moved the program to x/build, see CL 256357 instead. Will merge the change to src/README.vendor into CL 255860 instead.
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…
(1 comment) Thanks!
(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…
(1 comment) Adding it is okay, but please move it into isDocumentationTitle.
(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 …
(1 comment) Oh, I see this builds on top and modifies the label.
(1 comment) How does this relate to CL 255939?
Earlier
dmitshur starred in github.com/thommil/tge1w
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…
(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 …
(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 …
(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.…