Activity

Last Week
This sounds interesting, can you say more about how it would work? Would it only handle one level of pagination or are you thinking about multiple levels of pagination too? I don't know when I'll …
dmitshur pushed to main in github.com/shurcooL/home1d
c8586946c6ee9394cb069e42de62d884ac887a02internal/exp/service/auth/directfetch: handle photos with alt attribute
5d5a1576bdf9271891a998ecaf5e28f5a07c9c48internal/exp/service/notification/httpclient: retry in StreamNotifications
44102492bd8c0f5e63413585fd76018aa3175c33internal/code/http{client,handler}: fix typo in package comment
05616c9fc4260846215d3d52335787567ed7aafbinternal/exp/service/activity/github: support *PullRequestReviewEvent
ff602482ae70b71d9459e7d39de2d75177cae670internal/exp/app/{changes,issues}app: prefer flex for layout
The google-go-team team is being used now.
It sounds like this is okay to leave as is then. If in the future this change (possibly bundled with others like it) gets a higher benefit-to-cost ratio, a new issue can be filed (referencing this on…
We're getting close to the post-submit phase of the migration to LUCI, with many LUCI builders ported to https://ci.chromium.org/p/golang and ready to replace the previous coordinator-powered counter…
### Go version go version go1.22.0 darwin/arm64 ### Output of `go env` in your module/workspace: ```shell N/A ``` ### What did you do? I used fmt's `%#v` to print a value whose typ…
This was indeed fixed by [CL 566275](https://go.dev/cl/566275).
Thanks. This is covered in the mentioned issue: > (Most x/ repos get automatic monthly updates of x/ dependencies, but x/playground is currently skipped because of #24823. So fixing that might also…
The page title is h1, and the content needs to be nested lower for the table of content to recognize its sections.
Thanks. (nit) https://go.dev/wiki/CommitMessage suggests the first word after colon should be a verb, and a package before the colon. Probably just "cmd" is fine (like in CL 559795). For #64169.
The previous infrastructure did not display x/playground on the post-submit dashboard, but they did offer trybots. LUCI trybots are required now, so it's a good time to add basic coverage for x/playg…
Thanks. The linux/loong64 builder [isn't yet running](https://ci.chromium.org/ui/p/golang/g/port-linux-loong64/builders) on LUCI, so let's use the previous builders. TRY=linux-amd64-wsl, linux-loon…
dmitshur commented on os: implement CopyFS3d
> it needs some new directories and files inside the tree for its purposes Why can’t equivalent files inside a temp directory work? > maybe I should create them beforehand instead of doing it dur…
dmitshur commented on os: implement CopyFS3d
Tests in the standard Go library should not try to write within GOROOT itself. If writing is needed, the test should create a temp directory (e.g., in `t.TempDir()`), possibly copy files it needs the…
Done with: go get golang.org/x/mod go mod tidy Using go1.22.0. For golang/go#65891.
Consider a playground snippet that includes a go.mod file as if generated by `go mod init play.ground` using the latest stable version of Go: https://go.dev/play/p/ZtkN19wIOev. Pressing the "Forma…
dmitshur commented on os: implement CopyFS3d
(1 comment) Thanks for confirming that it wasn't an oversight.
Hey Will. https://go.microformats.io/ says the source for the site is this repository, so hopefully this is the right place to report this. Consider the following output of `Parse` on a given i…
Thanks very much for reporting this. I'm able to reproduce the error locally with: ```Go doc, err := html.Parse(strings.NewReader(`<div class="[ h-card ][ sidebar ]"><img class="u-photo" src="…
My review is from a while ago; I'll re-review after the CL is updated.
As a minor detail to append to https://github.com/golang/go/issues/64684#issuecomment-1959951186, the default value of the `bootstrapswarm -swarming` flag should already be the one you want to use, m…
The change looks reasonable to me. I understand the image needs to be built and tested to work. I think this change will apply to the LUCI version of this builder too, and by this point I suggest fo…
Thanks. Needs "golang/go" prefix in order to close the issue from an x/build CL.
dmitshur commented on cmd: remove support for GOROOT_FINAL4d
Thanks.
(1 comment) PS 4 looks good (and is easier to view in Gerrit 😅), thanks.
(1 comment) Sure, I just meant that there were still instances where the property is explicitly set to 1 instead of left out in generated/cr-buildbucket.cfg, but it can be fixed in any way that work…
(2 comments) (To remove some remaining no-op lines like this one.) I think the `if base_props["timeout_scale"] != 1: base_props.pop("timeout_scale")` step still needs to be moved down here, after a…
dmitshur commented on cmd: remove support for GOROOT_FINAL4d
(1 comment) Please move the `Cq-Include-Trybots` footer to be in the last paragraph, right next to the `Change-Id` footer: ```suggestion Change-Id: If7811c1eb9073fb09b7006076998f8b2e1810bfb Cq-Incl…
Thanks. Since the common case is no scaling (i.e., scaling is intended to be done when a builder is slower than normal, or a custom run mod like race or longtest needs more time than normal), and cr…
dmitshur commented on os: implement CopyFS4d
(1 comment) In the accepted [proposal](https://github.com/golang/go/issues/62484#issuecomment-1911326447), the doc comment included: > If a file name in fsys does not satisfy filepath.IsLocal, > an…
dmitshur commented on net/http: pkg import only once5d
I replied in a new inline comment. This will affect what shows up at https://pkg.go.dev/net/http#Redirect and doesn’t seem to be an improvement. (Note the parameter name used to be `urlStr` befor…
dmitshur commented on net/http: pkg import only once5d
(1 comment) Do you think it’s possible to remove `urlpkg` without harming readability of godoc? If so, I agree it’s better to remove it. Otherwise I would suggest leaving them as they are. urlp…
Thanks. I agree this is an improvement (see some further evidence for it below). :) Further evidence this is a good refactor: it helped me realize that this happens here, which I should've filtered …
Thanks. Interesting. https://pkg.go.dev/strings#Compare says "Compare is included only for symmetry with package bytes. It is usually clearer [...] to use the built-in string comparison operators [.…
Thanks. It seems to be made obsolete by the "owner:" search term which became available likely later on (e.g., https://dev.golang.org/reviews?q=owner%3Admitshur).
Moving to Unreleased, since Backlog seems to be for issues tied to the Go release cycle that aren't assigned a specific release milestone yet. (https://go.dev/wiki/HandlingIssues#milestones)
Some differences in configuration between the LUCI configuration vs the old coordinator linux-amd64-race trybot: - the coordinator one has 1 or 2 more test shards - coordinator skipped GOROOT/test …
The trend of the linux-amd64-race builder taking approximately 20 minutes is fairly visible here, under the Run Duration column: https://ci.chromium.org/ui/p/golang/builders/try/gotip-linux-amd64-…
Thanks. Consider using an issue number here if the context on this can be captured there easily and if you think someone else can pick it up. This path does align with the path chosen on line 943. …
Thanks. And this. Based on this. FWIW this seems to be the way to get it to skip on (all) darwin builders only. ```suggestion [GOOS:darwin] [go-builder] skip ```
I would like to believe the reason openbsd/amd64 trybots were disabled in CL 526255 no longer applies by now, so remove their special case as the next step. They seem to be passing consistently in po…
Thanks for preparing this. One possible way to move forward is for this change to be made available on a go-gl/glfw fork until the IME work lands in GLFW upstream. That is, take this change with l…
dmitshur commented on all: upgrade to Unicode 15.1.01w
Thanks for preparing this change. This is a fairly old parent commit. Please rebase to a more recent one that includes changes to add the doc/next directory. A release-note test should fail and poin…
Earlier
dmitshur commented on runtime: deprecate GOROOT1w
Sent CL 564275 to update the test case in x/tools.
The problem was that dynamic libraries weren’t available. That problem was resolved, so this CL isn’t needed now.
The problem was that dynamic libraries weren’t available. That problem was resolved, so this CL isn’t needed now.
(1 comment) If you want, feel free to include a change that unblocks local development without affecting prod. For example, if the -mastodon-api-secret flag isn't specified, mastodonClient can be le…
There should be a new patch set uploaded to go with the resolved comments, right? In case this was where you remember it from: older versions of `git-codereview mail` required switching to a branch …
(1 comment) FWIW they'll only be used on release branches (due to #64207) which are comparatively lower volume, in case that lets you select a different value here that might work better.
Thanks. Since this is new code it's temping to suggest the new-to-Go 1.12 `strings.ReplaceAll` API. But perhaps you chose this for consistency with line 852. Either way is fine.
dmitshur reviewed +2 on go.dev/wiki/SlowBots: fix typo1w
Thanks. Since this is a wiki and it's allowed to self+review here, feel free to do that for simple fixes in the future if you'd like.
Thanks. I think this is very clear, and very helpful to have documented. Here's an optional adjustment to refer to `Cq-Include-Trybots` as a git footer, and specifying it must be in the last paragra…
I suspect it should go to [Declined as Obsolete](https://github.com/golang/proposal?tab=readme-ov-file#declined-as-obsolete). Removed it from [Hold](https://github.com/golang/proposal?tab=readme-ov-f…
Thanks! v0.1.0-deprecated
(2 comments) Thanks, fixed. > is the reason why this isn't just the criteria for enabling platforms and is instead opt-in because we don't want to add ports without resources to presubmit? I think…
Thanks. If "go1.4/" directories are no longer applicable, maybe this is no longer needed. Fine to keep if it is safer to keep until a later (post-coordinator) clean up. Up to you.
Thanks. I believe this line intends to assign "arm" to the variable `goarch`.
A new distinct set of VMs will be used for security pre-submit and release testing. We choose not to have a shared pool in the security environment at this time, so take that into account. For gola…
Pull in the latest published version of github.com/google/pprof as part of the continuous process of keeping Go's dependencies up to date. Done with: go get github.com/google/pprof go mod tidy go mo…
I recall @adonovan used a GOPLSCACHE mechanism to help with that in the previous infrastructure ([CL 494297](https://go.dev/cl/494297)). I'm not seeing that in the LUCI infrastructure—perhaps it ne…
Thanks. I already started thinking about adding it. :) But FWIW I don't think it'd catch this type of skew, not until our Commit-Queue+2 is there and tests with rebase-onto-tip rather than as-is.
Thanks. Consider adding an `if testing.Short() { t.Skip("skipping in short mode") }` here, it seems like a good candidate given there's a shorter version of this as well.
Thanks. Some minor thoughts in inline comments. If someone sees this error, they might find it useful if it includes the got/want number of matches too. But maybe it's fine since the line itself is …
(1 comment) The TestDependencies test here seems quite similar in nature to the new test being added. Perhaps it'd be better if the new test were to be added to the same package, to keep these polic…
(1 comment) The diff handles this case unfortunately. The change from [before](https://go.googlesource.com/build/+/7fa0f16f99bb43dce484fe49c7fc8703a61bbb90/internal/releasetargets/releases.txt) to […
This change was created by taking the output of 'gotip tool dist list' and adding a corresponding builder type with a known issue. Don't add builder definitions for x/ repos until someone starts work…
There've been a few minor configuration changes in between CL 562536 being authored and submitted. Regenerate so that their effects apply in the recently added builders. Also add a note to README to…
The runtime.GOROOT function is about to be marked as deprecated, which will generate a diagnostic that isn't expected in the format.txt test case. Pick something else in the runtime package, since de…
dmitshur opened a change runtime: deprecate GOROOT1w
Fixes #51473.
Not spotting what caused the infra failure, but maybe I caused it with a duplicate push without changes (which Gerrit rejected, but maybe CV saw it anyway). Trying it again.
(1 comment) Spotted this now-useless continue while cleaning up releasetargets a bit in Ib66ca6958db695db0093556edc822dcdfbfdde0d. Removed and also updated comments above in PS 3.
Delete everything that runs on release-branch.go1.20 and older. This is dead code since only Go 1.21 and higher are supported now.
(1 comment) Right now this workflow starts the 'Mail announcement' step as soon tagging is done. Have you considered adding a "Wait to Announce" intermediate step here? Having that might be helpful…
Thanks. It's a bit odd to include this check since this is unsigned, so equivalent to constant `true`. A few years ago the yearly macOS updated bumped minor but kept major at 10. Since major 11 the…
Thanks.
Thanks for pointing that out. The comment there says "This must be fairly recent" and "In general this can be the most recent supported macOS version." If I understand correctly, that means it should…
Go 1.23 will require macOS 11 Big Sur or later, even on AMD64. The comment here suggests the main requirement for the OS and SDK version is to be recent enough not to break Apple signing, and recent …