Activity

Yesterday
(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…
The second bullet point in #30779 is related to this.
Approved as this is a small test fix. This backport applies to both 1.16 (#46769) and 1.15 (this issue).
Approved as this is a small test fix. This backport applies to both 1.16 (this issue) and 1.15 (#46768).
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…
(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
> 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 …
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 …
(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…
(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/…
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…
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…
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…
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…
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.
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…
(2 comments) I guess that optimization regressed. Sounds good. Thanks for the explanation.
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.
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…
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…
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 …
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…
(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…
(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…
(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 doc/go1.17: fix redundant space5d
Thanks. This seems to be the only one in this file.
+2 after inline comments are resolved. There's a good chance of caching getting in the way.
(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?
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…
(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…
(1 comment) For consistency with other app.yaml files, let's place service as the top line, above runtime.
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 …
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).
CC @eliasnaur. Are you able to take a look? Thanks.
@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://…
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…
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 …
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)…
LGTM but +1 so an x/crypto owner also looks/approves.
(1 comment) I believe go mod tidy needs to be run to remove this line.
(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…
Also see #43350.
Thanks. I understand issue #46701 tracks the remaining future work, so it's okay to keep this closed.
(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…
(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…
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…
Issue #46502 is fixed via CL 327309, skipping these tests is no longer needed.
Issue #46582 has been resolved in a better way, this is no longer needed.
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.
Closing in favor of CL 327549.
`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…
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…
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…
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 …
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.
(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.
(1 comment) ... followed by cmd.Run, except ...
(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…
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…
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…
(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.
@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
e573ff1376a319e124f48a5cfba62563afdf29f3document what it means for a move to be valid and legal
dmitshur pushed to master in github.com/shurcooL/tictactoe1w
d19fb9efac10cebf6f30a3b05f568ea636ebaec4factor check for move validity into Board.Apply
Indeed, thanks.
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…
Is this related to issue #46510?
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…
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…
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 …
(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…
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.
(1 comment) Consider also adding here: 'mv origPres origSite'