Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/playground: support third-party imports #31944

Open
bradfitz opened this issue May 9, 2019 · 30 comments

Comments

Projects
None yet
@bradfitz
Copy link
Member

commented May 9, 2019

It's time for the playground to support importing third-party packages.

We have https://proxy.golang.org/ now which is the hard piece.

It might have to assume @latest for now, unless we let people write a go.mod file somehow (magic comments either one per module version, or magic comments separating the textarea into N logical files, ala mime/multipart by easier to type?)

/cc @bcmills @dmitshur @ysmolsky

@gopherbot gopherbot added this to the Unreleased milestone May 9, 2019

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

e.g. @neild posted this snippet recently: https://play.golang.org/p/1rR97pPojOd ... it'd be nice if that actually worked.

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Well, that was easy...

bradfitz@go:~/src/golang.org/x/playground$ git di
diff --git a/sandbox.go b/sandbox.go
index 7493213..bf7a421 100644
--- a/sandbox.go
+++ b/sandbox.go
@@ -325,7 +325,7 @@ func compileAndRun(req *request) (*response, error) {
        exe := filepath.Join(tmpDir, "a.out")
        goCache := filepath.Join(tmpDir, "gocache")
        cmd := exec.Command("go", "build", "-o", exe, in)
-       cmd.Env = []string{"GOOS=nacl", "GOARCH=amd64p32", "GOPATH=" + os.Getenv("GOPATH"), "GOCACHE=" + goCache}
+       cmd.Env = []string{"GOOS=nacl", "GOARCH=amd64p32", "GO111MODULE=on", "GOPROXY=https://proxy.golang.org", "GOPATH=" + os.Getenv("GOPATH"), "GOCACHE=" + goCache}
        if out, err := cmd.CombinedOutput(); err != nil {
                if _, ok := err.(*exec.ExitError); ok {
                        // Return compile errors to the user.

There's a minor complication with supporting the legacy code.google.com/p/go-tour/pic/* packages in module mode. We currently pre-install them in the Docker container's $GOPATH, but module mode doesn't use that.

@dmitshur

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Agreed this would be nice, and is easier now than before.

There are no additional security concerns with this feature request. It's always been possible to accomplish this in a round-about way: by re-writing the snippet such that all third-party imports are inlined into the main program (similar to what bundle does). If the snippet does too much and takes too long to run, it'll still time out, etc.

ala mime/multipart by easier to type?

txtar format comes to mind.

One can also imagine a more featureful UI that allows adding extra files. For example, https://gist.github.com/ has an "Add file" button that adds another text box.

Maybe we can just have the playground run gists. *flashbacks to 2016* 😱

There's also some overlap with https://github.com/dave/jsgo#how-it-works and perhaps some other playground-like projects (https://goplay.space/, etc.).

It might have to assume @latest for now

As long as we fix #31889, using @latest would at least return consistent results.

@bcmills

This comment has been minimized.

Copy link
Member

commented May 9, 2019

One easy way to support non-latest versions would be to add comments to import statements:

import (
	"fmt"

	"gopkg.in/errgo.v2/errors"  // v2.1.0+incompatible
)

But that doesn't describe all of the transitive dependencies, and thus isn't particularly reproducible.
Might be better to write and store an explicit go.mod file; we could show it in a second column (perhaps initially hidden in a tray).

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@bcmills, or how about this: if the user hits "play" or "share" on a package that uses a third-party import, we append the auto-generated go.mod to the end of the textarea buffer (using whatever magic delimiter syntax we come up with), so it's included in shares and reproducible.

e.g. user writes:

package main

import "github.com/davecgh/go-spew/spew"

func main() {
        spew.Dump(map[string]string{"foo": "bar"})
}

Then hits "Run" or "Share" and we first change it to:

package main

import "github.com/davecgh/go-spew/spew"

func main() {
        spew.Dump(map[string]string{"foo": "bar"})
}

# File: go.mod
module play
go 1.13
require github.com/davecgh/go-spew v1.1.1
@ysmolsky

This comment has been minimized.

Copy link
Member

commented May 9, 2019

I love the idea. And it really comes to this: support multiple files in playground. I like the idea of using delimeters per file (txtar). I really wanted to implement support for multiple files, but I hate the idea of adding a box per each file - it does not feel right for the current UI - I tried multiple options there.

I support the idea of having multiple files specified by txtar, also main.go does not have to be prefixed by any file marker.

@gopherbot

This comment has been minimized.

Copy link

commented May 9, 2019

Change https://golang.org/cl/176317 mentions this issue: playground: support third-party imports (off by default for now)

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

The basic functionality (without any of the txtar etc multi-file splitting) is now done in CL 176317. It's off by default for now with an env flag (ALLOW_PLAY_MODULE_DOWNLOADS) set to false in the Dockerfile. That gives us a quick knob to turn it off later.

We also want to do some cleaning probably, so the disk doesn't fill. We'll probably want to hold a RWMutex while cleaning to avoid #31948

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Actually, CL 176317 now also cleans up after itself (any downloaded modules) after each build.

@bcmills

This comment has been minimized.

Copy link
Member

commented May 10, 2019

I would still rather we use a separate textarea instead of tacking the files together in the same buffer.
I don't really care whether that textarea goes below the Go source file or beside it.

I find it really useful to be able to press ⌃a ⌃c and paste the result directly into cat, and having to trim the file becomes particularly annoying as the files get longer.

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

@bcmills, we can start with a single buffer because it's a ton easier. We can add UI later, once somebody has time for that.

I find it really useful to be able to press ⌃a ⌃c and paste the result directly into cat, and having to trim the file becomes particularly annoying as the files get longer.

a) maybe playground snippets should only be snippets :)
b) I'll write you a splitter CLI tool you can pipe into. :)

@rsc

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Yes, please lets start with a single txtar buffer. We can worry about fancier UI later.

@Sajmani

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

A discussion around whether to support multiple files in the playground is in issue #3806.

@AlexRouSg

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Would this be only for pure Go imports or would it also work for self contained cgo using packages?

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Pure Go. It's still nacl for now. (See #30439 for removing nacl and moving the playground to a different type of sandbox, which might then support cgo)

gopherbot pushed a commit to golang/playground that referenced this issue May 10, 2019

playground: support third-party imports (off by default for now)
Also, modernize the Dockerfile while I'm at it, using multi-stage
builds more aggressively, and stop using gitlock (golang/go#26872) and
switch to using Go modules to build the playground. (This also turns
the playground into a module.)

Updates golang/go#31944

Change-Id: Ic6f6152469f1930fd04180a3d1e63ed92ea2cfbd
Reviewed-on: https://go-review.googlesource.com/c/playground/+/176317
Reviewed-by: Yury Smolsky <yury@smolsky.by>
@gopherbot

This comment has been minimized.

Copy link

commented May 13, 2019

Change https://golang.org/cl/176940 mentions this issue: playground: enable third-party imports

gopherbot pushed a commit to golang/playground that referenced this issue May 13, 2019

playground: enable third-party imports
Updates golang/go#31944

Change-Id: Iffc0755cca419b72d8facd8ece950e78cdae892e
Reviewed-on: https://go-review.googlesource.com/c/playground/+/176940
Reviewed-by: Andrew Bonventre <andybons@golang.org>
@akyoto

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

This is amazing because it lets library authors add playground links to document their API.
People can test it without installing the library on their own machine.
A big thank you for this feature.

@breerly

This comment has been minimized.

Copy link

commented May 14, 2019

@akyoto even better, examples in godoc could in principle automatically link to the playground.

@rogpeppe

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Thanks so much for doing this! Here's one issue: I just tried a snippet I was sharing yesterday and I get an error from vet, although the actual code works as expected:

The code is at: https://play.golang.org/p/cy1YkpSRtZJ
The vet errors I see seem like the third party deps haven't been pulled in before vet runs:

prog.go:9:2: cannot find package "github.com/heetch/confita" in any of:
	/usr/local/go/src/github.com/heetch/confita (from $GOROOT)
	/go/src/github.com/heetch/confita (from $GOPATH)
prog.go:10:2: cannot find package "github.com/heetch/confita/backend/file" in any of:
	/usr/local/go/src/github.com/heetch/confita/backend/file (from $GOROOT)
	/go/src/github.com/heetch/confita/backend/file (from $GOPATH)
prog.go:11:2: cannot find package "github.com/kr/pretty" in any of:
	/usr/local/go/src/github.com/kr/pretty (from $GOROOT)
	/go/src/github.com/kr/pretty (from $GOPATH)
Go vet exited.
@ysmolsky

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@rogpeppe, I see that @bradfitz already have deployed the new code. There are no vet errors when I run the snippet above.

EDIT: It might be an issue with playground.js not updated in the browser cache. When it happens it tries to call the old endpoint /vet, so that endpoint can fail.

@rogpeppe

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@ysmolsky Yes. When I hard-reloaded, the issue went away. Thanks!

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

Yeah, vet+modules required an update to the protocol and new JS client code.

@xdragon1015

This comment has been minimized.

Copy link

commented May 14, 2019

Awesome 😊

@ysmolsky

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@bradfitz I would like to tackle multi files support in playground to enable go.mod generation and processing. Is it okay to copy txtar from go/internal to playground?

@dmitshur

This comment has been minimized.

Copy link
Member

commented May 14, 2019

txtar is not in scope of #31761, so copying it (if that's the design we're implementing) sounds reasonable. If you're going to copy it 1:1 and don't need to make any modifications to it, I suggest copying via bundle so that there's less work of manually keeping it up to date. You can see an example of bundle usage to copy a package in golang/build@2a37d0d. It should also be placed it in an internal directory so outsiders don't start depending on it.

FWIW, there's also a copy of it at github.com/rogpeppe/go-internal/txtar, but not sure if it's viable to import a third-party package in x/playground.

Edit: That said, the playground already requires a couple of third-party modules, so maybe requiring github.com/rogpeppe/go-internal is acceptable? What do you think @bradfitz?

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

I have a local CL already in progress for multi-file support using github.com/rogpeppe/go-internal/txtar.

@gopherbot

This comment has been minimized.

Copy link

commented May 14, 2019

Change https://golang.org/cl/177043 mentions this issue: playground: support multiple input files in txtar format

gopherbot pushed a commit to golang/playground that referenced this issue May 15, 2019

playground: support multiple input files in txtar format
Updates golang/go#32040
Updates golang/go#31944 (Notably, you can now include a go.mod file)

Change-Id: I56846e86d3d98fdf4cac388b5b284dbc187e3b36
Reviewed-on: https://go-review.googlesource.com/c/playground/+/177043
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented May 15, 2019

Change https://golang.org/cl/177421 mentions this issue: playground: format go.mod files; fix paths in formatting errors

gopherbot pushed a commit to golang/playground that referenced this issue May 15, 2019

playground: format go.mod files; fix paths in formatting errors
Previously, handleFmt applied gofmt/goimports formatting to .go files
only. This change makes it apply formatting to go.mod files as well.
The cmd/go/internal/modfile package (well, a non-internal copy thereof)
is used to perform the go.mod file formatting.

Add test cases for error messages, and fix some cases where the paths
weren't accurate.

Detect when the error was returned by format.Source and needs to be
prefixed by using the fixImports variable instead of checking for the
presence of the prefix. This makes the code simpler and more readable.

Replace old fs.m[f] usage with fs.Data(f) in fmt.go and txtar_test.go.

Updates golang/go#32040
Updates golang/go#31944

Change-Id: Iefef7337f962914817558bcf0c622a952160ac44
Reviewed-on: https://go-review.googlesource.com/c/playground/+/177421
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@peterhellberg

This comment has been minimized.

Copy link

commented May 16, 2019

Running https://play.golang.org/p/-LQxD_2TQWw results in the output:

prog.go:3:8: cannot find package "github.com/peterhellberg/gfx" in any of:
	/usr/local/go/src/github.com/peterhellberg/gfx (from $GOROOT)
	/go/src/github.com/peterhellberg/gfx (from $GOPATH)
Go vet exited.

@@@@@@****------
@@@@**@@--**----
@@**@@@@----**--
**@@@@@@------**
**------@@@@@@**
--**----@@@@**@@
----**--@@**@@@@
------****@@@@@@

Program exited.

Changing the code somewhat (by for example adding a new empty line) and running it does not output the error from go vet.

(This is probably the same issue that @rogpeppe was seeing)

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.