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

Deprecate gqlgen Binary #415

Closed
mathewbyrne opened this issue Nov 6, 2018 · 16 comments
Closed

Deprecate gqlgen Binary #415

mathewbyrne opened this issue Nov 6, 2018 · 16 comments

Comments

@mathewbyrne
Copy link
Contributor

mathewbyrne commented Nov 6, 2018

Currently we ship a binary with gqlgen, but increasingly we're running into several issues with it:

  1. Bootstrapping is hard — we want to update the getting started docs to remove the go get usage, but then how do you run gqlgen in your project?

  2. Versioning differences — the getting started docs switch to using dep by their end, but this will cause issues if master is ahead of the latest release.

  3. Shipping Plugins — this becomes a lot harder since we need some way to load code dynamically from a fixed binary. We'd rather avoid trying to solve this by letting users roll their own binary anyway.

We're attempting to address the getting started issues at the moment with documentation, but we'd also like to make this code change at the same time. So starting from the next release out recommendation will be that you integrate gqlgen into your project by calling a new method in the cmd package. The documentation will be updated to reflect this.

This approach isn't set in stone yet, and I'm open to any other suggestions people might have. We've already seen a few projects do something similar, calling into the codegen package to have a custom binary.

Migration away

Follow these steps to migrate away from the gqlgen binary:

  1. In your project, create a scripts/gqlgen.go file with the following contents:
// +build ignore

package main

import "github.com/99designs/gqlgen/cmd"

func main() {
	cmd.Execute()
}
  1. If you are using dep to manage dependencies for your project, ensure that you have them all now:
dep ensure
  1. Update any generate commands in your project to use this new script. Ensure that the path to the script is correct:

    //go:generate go run scripts/gqlgen.go -v
  2. (optional) Remove the previous binary from your bin path so that you aren't accidentally running a previous version of gqlgen.

@MichaelMure
Copy link
Contributor

Can't agree enough. I hacked my way into this approach by copy/pasting internal code from gqlgen (see https://github.com/MichaelMure/git-bug/blob/master/graphql/gen_graphql.go), it works really well, no risk of version mismatch and go dep will vendor anything that might be required.

That said, why the cmdpackage ? Intuitively, this package should be for a command line tool, not for package that you import and use. Why not simply the codegen package or even the root gqlgen package ?

@vektah
Copy link
Collaborator

vektah commented Nov 6, 2018

We want to make sure there are no breaking changes here, so once someone copies the stub in they should only need to change it again if they want to add a plugin.

Secondly, we need to expose at least two commands, init and generate. Maybe we can package up init separately or get users to write a second entry point but it's additional work, and more surface area makes forward compatibility harder.

We also thought about changing the root package, but it would be a breaking change for everyone currently using gqlgen.

We should still make programmatic access to codegen a little cleaner, but we are focusing on improving the bootstrapping process which currently generates a few recurring issues (and probably causes some level of abandonment too)

@icco
Copy link
Contributor

icco commented Nov 6, 2018

Worth mentioning that whatever the solution is, it should work for people who use 1.11's go mod outside of the GOPATH as well, which the current solution doesn't.

I personally always found the binary confusing, and much preferred just using go generate

@mathewbyrne
Copy link
Contributor Author

@icco that wont be addressed here, but you will note that the updated getting started guide in #416 adds a box-out note that we don't currently support Go Modules. Proper support is tracked in #226

@emil-nasso
Copy link

I documented my (somewhat brute) approach to getting a working gqlbinary in issue #384 . No go get usage at all. Would very much like to know if there are any issues with my approach.

@mathewbyrne
Copy link
Contributor Author

mathewbyrne commented Nov 11, 2018

@emil-nasso nice job, that will get you across the line currently. The main issue you might eventually run into with your approach is that we may remove the func main() in the gqlgen package in the future and your go build command would stop working.

#416 has a new getting started document that also works without go get or gorunpkg. We will get this merged soon.

@yakovzaytsev
Copy link

@mathewbyrne migration seems to not work

~/go/src/github.com/yakovzaytsev/api-go (master) $ go generate -v ./...
cmd/api/api.go
cmd/api/api_test.go
pkg/api/generated.go
pkg/api/models_gen.go
pkg/api/resolver.go
stat scripts/gqlgen.go: no such file or directory
pkg/api/resolver.go:1: running "go": exit status 1
pkg/auth/auth.go
~/go/src/github.com/yakovzaytsev/api-go (master)$ go version
go version go1.11.2 darwin/amd64
~/go/src/github.com/yakovzaytsev/api-go (master)$ tree scripts/
scripts/
└── gqlgen.go

0 directories, 1 file
~/go/src/github.com/yakovzaytsev/api-go (master)$ head -2 pkg/api/resolver.go 
//go:generate go run scripts/gqlgen.go -v

~/go/src/github.com/yakovzaytsev/api-go (master)$ 

@mathewbyrne
Copy link
Contributor Author

mathewbyrne commented Dec 9, 2018

@yakovzaytsev what does your go generate command look like? My guess it that you do not have the correct path to your gqlgen script. You need to ensure that the path is relative to the file you have the generate command in. It looks like you're resolver is in pkg/api so you probably want something like:

//go:generate go run ../../scripts/gqlgen.go -v

The docs could be a bit clearer on this.

@yakovzaytsev
Copy link

@mathewbyrne you are right. Thank you. BTW, if you link project root it does not work

~/go/src/github.com/yakovzaytsev$ ls -la
lrwxr-xr-x   1 ysz  staff         23 Dec  9 23:40 api-go -> /Users/ysz/my/api

then

~/go/src/github.com/yakovzaytsev/api-go (master *)$ go generate ./...
gqlgen must be run from inside your $GOPATH

@yakovzaytsev
Copy link

yakovzaytsev commented Dec 10, 2018

Still does not work but I am not sure if that is relevant here

@mathewbyrne mind quick look? I'm on macOS

cmd/api/api.go
cmd/api/api_test.go
pkg/api/generated.go
pkg/api/models_gen.go
pkg/api/resolver.go
/usr/local/go/src/crypto/x509/root_cgo_darwin.go:13:10: fatal error: 'errno.h' file not found
#include <errno.h>
         ^~~~~~~~~
1 error generated.
/usr/local/go/src/net/cgo_bsd.go:11:10: fatal error: 'netdb.h' file not found
#include <netdb.h>
         ^~~~~~~~~
1 error generated.
/usr/local/go/src/os/user/cgo_lookup_unix.go:20:10: fatal error: 'unistd.h' file not found
#include <unistd.h>
         ^~~~~~~~~~
1 error generated.
/usr/local/go/src/crypto/x509/root_cgo_darwin.go:13:10: fatal error: 'errno.h' file not found
#include <errno.h>
         ^~~~~~~~~
1 error generated.
/usr/local/go/src/net/cgo_bsd.go:11:10: fatal error: 'netdb.h' file not found
#include <netdb.h>
         ^~~~~~~~~
1 error generated.
/usr/local/go/src/os/user/cgo_lookup_unix.go:20:10: fatal error: 'unistd.h' file not found
#include <unistd.h>
         ^~~~~~~~~~
1 error generated.
Skipped resolver: github.com/yakovzaytsev/api-go/pkg/api.Resolver already exists
/usr/local/go/src/net/cgo_bsd.go:11:10: fatal error: 'netdb.h' file not found
#include <netdb.h>
         ^~~~~~~~~
1 error generated.
cgo failed: [go tool cgo -objdir /var/folders/y4/fv7sd0lx6_5dncrg02t7th440000gn/T/net_C749225356 -- -I /var/folders/y4/fv7sd0lx6_5dncrg02t7th440000gn/T/net_C749225356 cgo_bsd.go cgo_resnew.go cgo_sockold.go cgo_unix.go]: exit status 1
/usr/local/go/src/os/user/cgo_lookup_unix.go:20:10: fatal error: 'unistd.h' file not found
#include <unistd.h>
         ^~~~~~~~~~
1 error generated.
/usr/local/go/src/crypto/x509/root_cgo_darwin.go:13:10: fatal error: 'errno.h' file not found
#include <errno.h>
         ^~~~~~~~~
1 error generated.
cgo failed: [go tool cgo -objdir /var/folders/y4/fv7sd0lx6_5dncrg02t7th440000gn/T/os_user_C674198907 -- -I /var/folders/y4/fv7sd0lx6_5dncrg02t7th440000gn/T/os_user_C674198907 cgo_lookup_unix.go getgrouplist_darwin.go listgroups_unix.go]: exit status 1
cgo failed: [go tool cgo -objdir /var/folders/y4/fv7sd0lx6_5dncrg02t7th440000gn/T/crypto_x509_C203742366 -- -I /var/folders/y4/fv7sd0lx6_5dncrg02t7th440000gn/T/crypto_x509_C203742366 -mmacosx-version-min=10.10 -D__MAC_OS_X_VERSION_MAX_ALLOWED=101300 root_cgo_darwin.go]: exit status 1
/usr/local/go/src/os/user/lookup.go:53:9: undeclared name: lookupGroupId
/usr/local/go/src/os/user/lookup.go:47:9: undeclared name: lookupGroup
/usr/local/go/src/os/user/lookup.go:41:9: undeclared name: lookupUserId
/usr/local/go/src/os/user/lookup.go:32:9: undeclared name: lookupUser
/usr/local/go/src/os/user/lookup.go:11:41: undeclared name: current
/usr/local/go/src/os/user/lookup.go:58:9: undeclared name: listGroups
/usr/local/go/src/net/lookup_unix.go:323:23: undeclared name: cgoLookupPTR
/usr/local/go/src/net/lookup_unix.go:123:24: undeclared name: cgoLookupCNAME
/usr/local/go/src/net/lookup_unix.go:107:23: undeclared name: cgoLookupPort
/usr/local/go/src/net/lookup_unix.go:95:24: undeclared name: cgoLookupIP
/usr/local/go/src/net/lookup_unix.go:80:24: undeclared name: cgoLookupHost
/usr/local/go/src/crypto/x509/root.go:21:32: undeclared name: loadSystemRoots
/usr/local/go/src/crypto/x509/cert_pool.go:65:9: undeclared name: loadSystemRoots
/Users/ysz/go/src/github.com/yakovzaytsev/api-go/pkg/api/resolver.go:106:9: cannot use &(mutationResolver literal) (value of type *mutationResolver) as MutationResolver value in return statement: missing method NewSpace
validation failed: couldn't load packages due to errors: os/user, crypto/x509, net and 1 more
exit status 2
pkg/api/resolver.go:1: running "go": exit status 1
pkg/auth/auth.go

@mathewbyrne
Copy link
Contributor Author

@yakovzaytsev that doesn't look related to gqlgen specifically. Maybe something to do with your environment. The link thing is interesting. I would suggest link in reverse, and have the concrete folder in your $GOPATH and link from your user directory.

@MichaelMure
Copy link
Contributor

Having finally upgraded to the latest gqlgen, I migrated to this solution. I'd like to suggest a slight improvement:

Instead of using the urfave/cli command in the script plugged into go:generate, gqlgen could provide a high-level function with the same flags/arguments, something along the line of:

func Generate(path string, verbose bool) error { /* ... */ } 
func GenerateWithFilename(path string, verbose bool, filename string) error { /* ... */ } 

Usage would be something like:

func main() {
	cwd, _ := os.Getwd()
	dir := path.Join(cwd, "graphql")

	fmt.Println("Generating GraphQL code ...")

	err := gqlgen.Generate(dir, false)
	if err != nil {
		log.Fatal(err)
	}
}

With the go:generate line being simply //go:generate go run scripts/gqlgen.go without any flags.

Advantages would be:

  • more programmability: you are not limited by what you can't do in a go:generate command line, you can easily change the current directory, have some logs, react to an error, maybe even pre-process your schema for some reason ..
  • no need to import/vendor urfave/cli

@vektah
Copy link
Collaborator

vektah commented Dec 24, 2018

We though about doing that, but for bootstrapping (gqlgen init).

We might also want additional subcommands.

I don't see any harm in adding another function that can be used like this though, but it's probably not the default.

@janflyborg
Copy link

Have only skimmed through this thread lightly (so take this with a grain of salt), but now that Go modules are here, doesn't that make the problems 1 and 2 in the original posting go away?

For plugin support in 3, that would also mean that you limit gqlgen to running on Linux and macOS only, since other platforms (including Windows) are currently not supported.

@mathewbyrne
Copy link
Contributor Author

Correct, there is an update to this that wasn't referenced, but the next branch reinstates this binary in #553, since Go Modules support mean it's now not an issue to be running the correct version of the binary.

#228 tracks plugin support. I suspect that we will require you to roll your own binary for custom plugins, and we'll do our best to make this as seamless as possible.

@janflyborg
Copy link

Excellent! Thanks for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants