Activity
Yesterday
dmitshur commented on golang.org/x/build/cmd/coordinator: enable post submit builder for linux-amd64-unified11h
(2 comments)
I think some parenthesis were missing, as this the modified boolean logic will still have 'continue' trigger for the linux-amd64-unified builder. But this may not matter because…dmitshur commented on golang.org/x/build/cmd/coordinator: unable to add a post submit builder for non-master branch12h
The second bullet point in #30779 is related to this.dmitshur commented on syscall: TestGroupCleanupUserNamespace test failure on Fedora [1.15 backport]17h
Approved as this is a small test fix. This backport applies to both 1.16 (#46769) and 1.15 (this issue).dmitshur commented on syscall: TestGroupCleanupUserNamespace test failure on Fedora [1.16 backport]17h
Approved as this is a small test fix. This backport applies to both 1.16 (this issue) and 1.15 (#46768).dmitshur commented on run.bash: document GOTESTONLY, GO_TEST_SHORT and GO_TEST_TIMEOUT_SCALE17h
By now I think it's okay to document `GO_TEST_SHORT` and `GO_TEST_TIMEOUT_SCALE` variables and their effect, even if they're meant to be used internally in the project and its build system. The docum…dmitshur reviewed +2 on cmd/internal/moddeps: use a temporary directory for GOMODCACHE if needed17h
(4 comments)
I see. You could in theory change CL 328770 to say "Fixes" and this one to say "Updates", but I think it's too minor to matter.
I suggest replacing this to clarify …Last Week
dmitshur commented on github.com/XProger/OpenLara: macOS Big Sur Support2d
> the standalone app isn't yet compatible
Which standalone app are you referring to, and in what way is it not compatible?
I was able to build and run OpenLara from source on an M1 MacBook Air …dmitshur opened an issue dmitri.shuralyov.com/go/generated: update implementation for 2020 spec changes3d
The rule for allowed placement of the auto-generated DO NOT EDIT comment changed in Go [proposal 41196](https://golang.org/issue/41196). Previously, the comment could be anywhere in the file, but now…dmitshur reviewed +2 on image: add RGBA64Image interface3d
Thank you.
Most likely. Having these sorted is a convention, it's not really enforced. Still, sorted is better, so thanks for fixing it.
We generally try to avoid Latin abbreviations in public …dmitshur commented on cmd/internal/moddeps: use a temporary directory for GOMODCACHE if needed3d
(5 comments)
I'm confused how this fixes that issue. In the original report, the test that failed was:
TestAllDependencies/goroot(quick)
The code changes here are all for the (thorough…dmitshur reviewed +2 on cmd/internal/moddeps: use -mod=readonly instead of -mod=mod3d
(1 comment)
Please add a sentence that says why make this change, per https://golang.org/wiki/CommitMessage#non-normative-references.dmitshur commented on image: add RGBA64Image interface3d
(1 comment)
This CL is adding new exported identifiers. Please document them in the Go 1.17 release notes. I think it would go under new image and image/draw parts of the https://tip.golang.org/doc/…dmitshur closed an issue image: add RGBA64Image interface [freeze exception]3d
dmitshur commented on image: add RGBA64Image interface [freeze exception]3d
We've discussed this exception, and it is approved. This will need to be mentioned in the release notes (#44513), which can be done in the same CL or a follow up one. Please proceed and get everythin…dmitshur reviewed +2 on golang.org/x/website/...: promote new app deployments only after they become ready3d
This is going to be very nice to have, thanks!
This CL modifies the 2 other copies of googleCN. Should it make the same change here?
By now all copies of this function are in one repository (rather…dmitshur commented on golang.org/x/build: adding linux-riscv64-unmatched builder3d
It's hard to have a concrete plan, since these builders are remote and depend on people running them. Having multiple builders for the same platform can help if one goes missing, another can still pr…dmitshur reviewed +2 on golang.org/x/website/cmd/golangorg: expose multiple domains on testing servers3d
The change looks good. A few questions.
I've also tested x/build/cmd/tip with this change, and it should work okay on tip.golang.org without needing changes. (That said, when testing cmd/tip loc…dmitshur commented on image: add RGBA64Image interface [freeze exception]4d
Thanks for making a freeze exception request. I'll discuss this with the @golang/release team tomorrow, and we'll try to make a decision by then.dmitshur reviewed +2 on golang.org/x/website/cmd/golangorg: make zip contents seekable4d
Do you think it'd be worth it to add test coverage for seekableFS, since it's not tested now?
internal/backport has testing/fstest—perhaps just TestFS is enough. (I thought about making Te…dmitshur commented on golang.org/x/website/blog: simplify app, stop being a submodule4d
(2 comments)
I guess that optimization regressed. Sounds good.
Thanks for the explanation.dmitshur commented on golang.org/x/website: "seeker can't seek" error when serving particular files4d
Thanks for the report. Russ and I have been landing changes in x/website, and this is quite likely related to that. We're going to look.dmitshur commented on golang.org/x/build: new builder for cmd/compile's "unified IR" mode4d
Thanks. Having that information written down here will be helpful in the future. I think it's okay to close the issue since the builder is added and my question is answered, unless you're aware of so…dmitshur reviewed +2 on golang.org/x/website/cmd/golangorg: find contentDir even on App Engine4d
dmitshur commented on golang.org/x/build: new builder for cmd/compile's "unified IR" mode4d
From looking at the CL, I understand this issue needs to be updated to mention TryBots (that is, pre-submit builders too, not just post-submit ones). If this will be helpful for development, adding t…dmitshur reviewed +2 on golang.org/x/website/go.dev: fix for new app deployment4d
dmitshur reviewed +1 on golang.org/x/playground/examples: add an HTTP server example4d
This seems like a nice example in general. However, it might increase importance of revisiting #44822 (I've run into a couple timeouts myself when trying my snippet). I'm okay with adding it …dmitshur reviewed +1 on image: add RGBA64Image interface5d
I don’t have objections to this landing for 1.17.
A comment about API check, and 2 really minor optional comments.
If this CL is going to be submitted for 1.17, before submission it’ll need to …dmitshur reviewed +2 on golang.org/x/website/cmd/golangorg: factor out main handler setup5d
See inline comment.
This was previously registered without going through hostEnforcerHandler, but now it is. I understand it's an intentional change, and is okay (at least I don't see a reas…dmitshur commented on golang.org/x/website/internal/web: redirect golang.org/pkg/... to pkg.go.dev/...5d
(1 comment)
I'm not sure I see a way for this case to work with TestLiveServer, which runs cases in web.txt as of CL 328010. Is it expected to work? Or should TestLiveServer be updated not to ru…dmitshur reviewed +2 on golang.org/x/website/internal/web: redirect golang.org/pkg/... to pkg.go.dev/...5d
(2 comments)
I'm okay with that.
What do you think about modifying webtest to add a new "host" verb to customize requests, and require URLs in webtest cases to be relative, like '/f…dmitshur reviewed +2 on golang.org/x/website/cmd/golangorg: merge into single source file5d
dmitshur reviewed +2 on golang.org/x/website/cmd/golangorg: deduplicate testdata/live.txt and testdata/web.txt5d
(1 comment)
Optionally, consider documenting the comment syntax in webtest documentation, since it doesn't appear to be currently documented (0 matches for "#" at https://pkg.go.dev/gola…dmitshur reviewed +2 on golang.org/x/website/cmd/golangorg: remove global fsys, site5d
dmitshur reviewed +2 on doc/go1.17: fix redundant space5d
Thanks. This seems to be the only one in this file.dmitshur reviewed +2 on golang.org/x/website/tour: fix for new app deployment5d
+2 after inline comments are resolved.
There's a good chance of caching getting in the way.dmitshur commented on golang.org/x/website/tour: fix for new app deployment5d
(2 comments)
Add:
Fixes golang/go#46793.
(Or 'For', if you don't want to close it right away.)
Does this need to change too?dmitshur opened an issue golang.org/x/website/tour: latest version cannot find template files in production5d
As of [CL 323897](https://golang.org/cl/323897), `golang.org/x/website/tour` runs into an error when deployed to [tour.golang.org](https://tour.golang.org):
```
running in App Engine Standard mod…dmitshur commented on golang.org/x/website/blog: simplify app, stop being a submodule5d
(2 comments)
I understand this is being removed intentionally. Can you add a note to commit message providing the rationale (which will be very useful to know if someone in the future tries to re-ad…dmitshur reviewed +2 on golang.org/x/website/cmd/googlegolangorg: make google.golang.org a separate service5d
(1 comment)
For consistency with other app.yaml files, let's place service as the top line, above runtime.dmitshur merged a change golang.org/x/build/cmd/tip: update for x/website changes on 2021-06-145d
dmitshur reviewed +2 on golang.org/x/website/cmd/golangorg: simplify, run on App Engine Standard, not Flex5d
In general everything looks good to proceed, see minor comments inline. I'm not sure if you've already tried to do a test deploy—I expect there may be minor issues that'll be found and …dmitshur commented on golang.org/x/build: Corellium builders are down5d
Thank you Elias. Unfortunately, we can't make use of the offer to get access to the dashboard at this time (also see https://github.com/golang/go/issues/41610#issuecomment-702940002).dmitshur commented on golang.org/x/build: Corellium builders are down5d
CC @eliasnaur. Are you able to take a look? Thanks.dmitshur commented on github.com/holzschu/a-shell: Please add golang and fzf and ripgrep OR the silver searcher (ag)6d
@personalizedrefrigerator FWIW, adding Go to a-Shell is not a requirement to have `gh` in a-Shell. Go has support for WebAssembly (although you may also want WASI, which is in development at https://…dmitshur opened a change golang.org/x/build/cmd/tip: update for x/website changes on 2021-06-146d
As of CL 323891, golangorg can't find x/website/_content
unless it's run inside the x/website repository. So, set
its working directory to something that works.
While here, also modernize ho…dmitshur commented on golang.org/x/build/cmd/tip: needs updating after recent x/website/cmd/golangorg changes6d
I think the fix is to set `golangorg.Dir` to `websiteDir` (the root of x/website repo checkout), so the `golangorg` process starts in a directory where it finds _content.
We can also preemptively …dmitshur opened an issue golang.org/x/build/cmd/tip: needs updating after recent x/website/cmd/golangorg changes6d
From https://farmer.golang.org/ (thanks @cagedmantis for reporting):
> - [tip.golang.org website](https://farmer.golang.org/status/tip) [[docs](https://github.com/golang/build/tree/master/cmd/tip)…dmitshur reviewed +2 on golang.org/x/term/...: upgrade x/sys to latest6d
dmitshur reviewed +2 on golang.org/x/website/doc/go1.14: add missing release note about text/template escaping6d
dmitshur reviewed +1 on golang.org/x/crypto/...: upgrade x/sys to latest6d
LGTM but +1 so an x/crypto owner also looks/approves.dmitshur commented on golang.org/x/crypto/...: upgrade x/sys to latest6d
(1 comment)
I believe go mod tidy needs to be run to remove this line.dmitshur commented on golang.org/x/term/...: upgrade x/sys to latest6d
(1 comment)
Please run go mod tidy to remove these 2 lines.dmitshur commented on lib/time: move to src/time/tzdata6d
(1 comment)
As I understand, "embedded" in that sentence refers to a mechanism through which the data is added to a program, which is currently done via a generated .go file containing a str…dmitshur commented on lib/time: move zoneinfo.zip to src/time/tzdata6d
Also see #43350.dmitshur commented on syscall: TestStdioAreInheritable fails on windows-arm64-aws because it tries to build a library using an unsupported build mode1w
Thanks. I understand issue #46701 tracks the remaining future work, so it's okay to keep this closed.dmitshur reviewed +2 on golang.org/x/website/internal/web: redirect golang.org/pkg/... to pkg.go.dev/...1w
(8 comments)
Ack.
Add:
For golang/go#44356.
(Or fixes if there's not more to do after this.)
It should probably be added as a flag, so that it's easier to test both modes in loca…dmitshur commented on golang.org/x/website/cmd/golangorg: simplify, run on App Engine Standard, not Flex1w
(1 comment)
This CL deletes the 'regtest' Makefile target, which was the main entry-point for TestLiveServer.
Without anything actively running TestLiveServer, I suspect it'll get out o…dmitshur closed an issue golang.org/x/build: linux-riscv64-unleashed builder is missing1w
dmitshur commented on golang.org/x/build: linux-riscv64-unleashed builder is missing1w
Thank you @bradfitz. I can see it's connected and running builds now, so closing as resolved.
@mengzhuo Would you like to proceed with your builder request, or do you think it's no longer needed g…dmitshur closed a change cmd/link: skip tests that build testprogcgo on windows/arm641w
dmitshur commented on cmd/link: skip tests that build testprogcgo on windows/arm641w
Issue #46502 is fixed via CL 327309, skipping these tests is no longer needed.dmitshur closed a change syscall: skip TestStdioAreInheritable when c-shared is unsupported1w
dmitshur commented on syscall: skip TestStdioAreInheritable when c-shared is unsupported1w
Issue #46582 has been resolved in a better way, this is no longer needed.dmitshur commented on syscall: TestStdioAreInheritable fails on windows-arm64-aws because it tries to build a library using an unsupported build mode1w
The windows/arm64 port supports c-shared build mode as of [CL 326310](https://golang.org/cl/326310), and `TestStdioAreInheritable` is no longer failing. (CC @zx2c4.) Closing as fixed.dmitshur closed a change cmd/go: skip TestWindowsDefaultBuildmodIsPIE/cgo on windows/arm641w
dmitshur commented on cmd/go: skip TestWindowsDefaultBuildmodIsPIE/cgo on windows/arm641w
Closing in favor of CL 327549.dmitshur closed an issue cmd/go: TestWindowsDefaultBuildmodIsPIE/cgo fails on windows-arm64-aws1w
dmitshur commented on cmd/go: TestWindowsDefaultBuildmodIsPIE/cgo fails on windows-arm64-aws1w
`TestWindowsDefaultBuildmodIsPIE/cgo` no longer fails on `windows-arm64-aws` as of [CL 327549](https://golang.org/cl/327549), so closing. (CC @zx2c4.)
There's also #46719 which is still open and c…dmitshur reviewed +1 on golang.org/x/build/cmd/gitmirror: use liveness probes1w
Leaving to Carlos to review, pointing out some things that jumped out when I looked.
Add:
w.Header().Set("Content-Type", "text/plain; charset=utf-8")
So that the HTTP server do…dmitshur commented on golang.org/x/build/cmd/gitmirror: get tini from Debian1w
Since this CL is cleaning up/simplifying the Dockerfile, I left an optional comment about something I spotted, in case you think it's worth expanding the scope. (It's fine not to expand scope…dmitshur commented on golang.org/x/website/cmd/golangorg: simplify, run on App Engine Standard, not Flex1w
I understand this CL needs to be updated given some changes since the time it was authored. I also asked some questions. See inline comments.
You're probably aware, but: golang.org used to be a …dmitshur reviewed +2 on golang.org/x/build/cmd/gitmirror: kill subprocesses with SIGINT before SIGKILL1w
Nits.
exec.Cmd.Run? There's no Run function in os/exec package.
s/Context/context/ since it seems to be used as a word rather than symbol.dmitshur reviewed +2 on golang.org/x/website/...: make compatible with Go 1.151w
(2 comments)
Nit: Can remove this blank line.
Minor: Since this CL isn't merged yet, consider backporting further changes to findContent from CL 323894 into here.dmitshur reviewed +2 on golang.org/x/build/cmd/gitmirror: kill subprocesses with SIGINT before SIGKILL1w
(1 comment)
... followed by cmd.Run,
except ...dmitshur reviewed +2 on golang.org/x/website/cmd/golangorg: change tests to use internal/webtest1w
(2 comments)
This became unused, delete.
Minor: The Corpus.InitVersionInfo method that this refers to was removed, by now it's called 'api.Load' or so. Also, its failure was upgraded fr…dmitshur reviewed +2 on golang.org/x/website/internal/webtest: add package for testing web servers1w
For information, is this the first version of this webtest package (similar in spirit to cmd/go's testscript) or are there other public places where something like webtest already exists? I'm…dmitshur closed an issue github.com/go-gl/gl: Big sur opengl1w
dmitshur commented on github.com/go-gl/gl: Big sur opengl1w
That looks like normal behavior. The `github.com/go-gl/gl/v4.1-core/gl` package is contained inside the `github.com/go-gl/gl` module, and that the go get command prints the module version that was ad…dmitshur reviewed +2 on golang.org/x/website/internal/backport: add backports of current Go dev template, fs packages1w
In general, this change makes sense. We've done similar things in x/pkgsite e.g. to pull in early fixes/new APIs to go/doc before they were released. My only comment on this CL is about ensuring …dmitshur commented on github.com/go-gl/gl: Big sur opengl1w
There isn't a package with import path `github.com/go-gl/gl/v4.1/gl`. There is [`github.com/go-gl/gl/v4.1-core/gl`](https://pkg.go.dev/github.com/go-gl/gl/v4.1-core/gl) and [`github.com/go-gl/gl/v4.1…dmitshur reviewed +2 on golang.org/x/website/go.dev/_content/about.md: mention blog post on retractions1w
(2 comments)
Repeated word.
Minor: The blank line here makes it look like they're separate paragraphs, but they're not. If you think it helps anyway, then it's okay to keep.dmitshur commented on golang.org/x/build: dragonfly-amd64-5_8 builder is missing1w
@tuxillo Yes—please mention that you need a key in the issue for the new builder, and the host name to use (I expect it'll be something like `host-dragonfly-amd64-6_0`). We can send it to you by em…dmitshur commented on github.com/go-gl/gl: Big sur opengl1w
macOS is supported. `v2.1/gl` or a higher version that corresponds to what's supported by the device should work.Earlier
dmitshur pushed to
master in github.com/shurcooL/tictactoe1w
dmitshur pushed to
master in github.com/shurcooL/tictactoe1w
dmitshur merged a change github.com/shurcooL/tictactoe: Ensure move is valid1w
dmitshur reviewed +2 on github.com/shurcooL/tictactoe: Ensure move is valid1w
Thanks.dmitshur commented on github.com/shurcooL/tictactoe: Ensure move is valid1w
Indeed, thanks.dmitshur reviewed +2 on golang.org/x/website/tour: fix dead link to golang.org search1w
Thank you Alberto. I made some optional suggestions for tweaking the URL, though I think the current one is good too.
Optional things to consider:
The '*' can be '+' (it's expec…dmitshur commented on cmd/go: clang does not generate .edata sections1w
Is this related to issue #46510?dmitshur merged a change golang.org/x/website/_content/.well-known: revert register libera.chat project1w
dmitshur reviewed +2 on golang.org/x/website/_content/.well-known: revert register libera.chat project1w
Thank you.
(For reference, if we need to add another file to _content/.well-known, the the //go:embed pattern can be modified to include '_content/.well-known'. To avoid the HTML being place…dmitshur commented on golang.org/x/website/go.dev/_content/about.md: advise retraction to remove modules1w
I'm not familiar with the go.dev codebase. Based on git blame, Russ added TestGolden fairly recently and Jamal was a reviewer.
I know there's some consolidation planned in a stack of CLs, an…dmitshur reviewed +2 on golang.org/x/website/go.dev/_content/about.md: advise retraction to remove modules1w
I think this is good to merge after the tests are fixed.
We should add go.dev owners to x/website/go.dev now that go.dev's code was merged into x/website. :)
Linking to the module reference is …dmitshur reviewed +2 on golang.org/x/website/cmd/golangorg: split out internal/codewalk [generated]1w
dmitshur reviewed +2 on golang.org/x/website/cmd/golangorg: introduce CodewalkServer1w
dmitshur reviewed +2 on golang.org/x/website/...: remove toFS usage1w
(5 comments)
This Go comment might be outdated by the code change above. (Or, I don't understand it.)
Since println("LOST", ...) is deleted below, should this println be deleted too?
D…dmitshur reviewed +2 on golang.org/x/website/internal/dl: move template to lib/godoc1w
I only looked at the code diff, did not test this more closely. If there are regressions they'll be easy to spot and follow up on.
Nit: Extra blank line doesn't seem to be intentional.dmitshur reviewed +2 on golang.org/x/website/internal/godoc: refactor into internal/web [generated]1w
(1 comment)
Consider also adding here:
'mv origPres origSite'