Activity

Last Week
Thanks. As a side note, I'm starting to wonder if we should instead (or in addition) link to the upstream documentation on this at https://www.glfw.org/docs/latest/compile_guide.html#compile_deps_…
The comment of the DialConfig function was dropped during CL 463097. There doesn't seem to be a good reason to do that, so bring it back. For golang/go#57953.
This CL targets release-branch.go1.22, is that intentional? If so, please see https://go.dev/wiki/MinorReleases for the process of backporting, which begins with creating backport issues.
This manual parameter might become obsolete in the future, in favor of being pulled from another source automatically. Since that hasn't been implemented yet, add a simple automated check to it in th…
dmitshur reviewed +2 on cmd/internal/testdir: support -godebug4d
Thanks.
This is done, and announced at https://groups.google.com/g/golang-dev/c/CqVVuxngzDU/m/MD4qrteuAQAJ. There may be more to do in this general, but bulk of the work is done, so I'll close this. We ca…
Thanks. This seems reasonable to try. If builder availability gets too much in the way of #17104 we can refine it further.
(2 comments) Go 1.22 and 1.21 by now, right? (gotip will be Go 1.23.) These are the current darwin/amd64 builder types. They all tend to specify the macOS version, so "darwin-amd64" doesn't match a…
We're aware of some issues on the LUCI post-submit dashboard (https://ci.chromium.org/p/golang/g/go-gotip/console). This is an umbrella tracking issue to collect most critical ones that affect the Go…
This change is ready for review.
Thanks. CL 567816 resolves that issue.
(1 comment) Just checking, is it not an option (or a less preferable option) to use a `//go:debug` directive in the test file?
(1 comment) `goDebug` is added near `goExperiment` under a comment that says “Package-scoped variables that are initialized at the start of Test.” However it’s not initialized to any value at…
(1 comment) I agree both ways are fine and this is very minor. Thanks for considering it.
(1 comment) Since `k` seems to not be needed anywhere else, maybe inline it: ```suggestion switch t.Kind() { ```
Thanks for the fix. I think a good approach is to use the -dry-run flag and run it locally. For example, I ran it with `-dry-run -single-pr=golang/go#65218` flags as is, and it printed: ``` Proces…
Ok, that involves more steps, please see https://go.dev/wiki/MinorReleases. Since this is the main tracking issue, let's move it to the Go1.23 milestone.
@timothy-king This issue is described to be for x/tools. Was it added to Go1.22.1 milestone by mistake, and can be Unreleased instead? Thanks.
Thanks. Adding Cherry who reviewed the new packages and is in the proposed primary owner group (runtimeTeam) to review. Is this the right import path? The packages added in CL 548695 and CL 548676 d…
Thanks for checking in. I've looked into this and as things stand now, there is expected additional time after the public Go 1.22.0 release, before it's available for the CIPD package building pip…
CL 567495 removed the special case that CL 528155 added temporarily. This CL removes the updated test cases that CL 528540 needed to add back then. They're no longer passing and that is working as in…
Post-submit builders for x/playground have been added in LUCI, for example: https://ci.chromium.org/p/golang/g/x-playground-gotip/console (There are more dashboards for release branches and wit…
Right now the x/playground repo is tested via the usual `go test ./...` as other x/ repos. As described in #24823, there are tests that need additional dependencies to run (Docker). This is the…
relui's monthly tagging + updating workflow currently skips x/playground: https://cs.opensource.google/go/x/build/+/master:cmd/relui/main.go;l=280;drc=9005598d96168e9f85759004415182eff2246809 I…
Thanks for reporting. Fixing this will involve navigating the constraint that the current message is used for de-duplication, so changes to its exact text without an additional mechanism to detect…
(1 comment) Post-submit testing reports this test case will also need to be updated.
`/bin/bash: /bin/ls: Device error` Looks like a general instance of #65040 rather than something specific to this test.
For posterity (I didn't hit send earlier): this was deployed and confirmed to work in https://go-review.googlesource.com/c/tools/+/527455/comments/ce012492_44228720.
Thanks. Thanks for the CL, and congrats on 1.18 dropping off! This is generally right, but will have effect in the old infrastructure. For LUCI trybots the change will be in `main.star` on the `luc…
I understand from CL 567495 that x/tools no longer needs to be tested with Go 1.18.x. This updates the LUCI pre-submit coverage accordingly.
The builder has been running some time and producing good signal so far. Remove its known issue as its next step. The issue tracks the remaining work to document its setup, and improve life-cycle edg…
(1 comment) The replacement for this was the 10px padding of div.page being added where needed to compensate. I'll check with another browser too to be safe.
This change is ready for review.
Restore the local "dev" mode after the refactor in CL 422956. The development environment doesn't have KubeServices set to anything, so KubeServices.Location() was otherwise panicking. While here, a…
(1 comment) Pointing out that when mentioning this upcoming feature in https://go-review.googlesource.com/c/go/+/567296/comment/a8a880b9_da27e615/, I noticed that the previous approach to linking to…
(1 comment) This URL isn't quite right. It will link to https://go.dev/reflect.Value and be 404. See https://cs.opensource.google/go/go/+/master:doc/README.md;l=30. (Note that in a near future ther…
@thanm Are there specific tasks left to do here, or is it a good time to mark this as resolved? Thanks.
Thanks. (nit) Duplicate '/'. Some of the comments around this test talk about vendoring in the go repo generally. Perhaps it's slightly better to generalize this slightly so vendored packages have …
TestReverseDial failures are indeed unrelated and should be fixed by https://crrev.com/i/7007547 soon.
(1 comment) Swapped order.
Retrying trybots since the 503 seems transient, to be confirmed.
Thanks. Is it worth also including `stdout.String()` here, since it seems to be unused otherwise (but may be interested to see what's there, even if nothing)?
No, purple failure and "resource exhaustion" means those builders haven't been fully added, see https://go.dev/wiki/LUCI#infra-failed--purple-failure and #60440.
Earlier
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/home1w
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 CopyFS1w
> 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 CopyFS1w
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 CopyFS1w
(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_FINAL1w
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_FINAL1w
(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 CopyFS1w
(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 once1w
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 once1w
(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)