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

Package templates into the binary #9

Closed
benjaminjkraft opened this issue Apr 22, 2021 · 10 comments · Fixed by #180
Closed

Package templates into the binary #9

benjaminjkraft opened this issue Apr 22, 2021 · 10 comments · Fixed by #180
Labels
bug Something isn't working

Comments

@benjaminjkraft
Copy link
Collaborator

If we can put this off until we're okay with a minimum version of 1.16, we can use stdlib embed.

@benjaminjkraft
Copy link
Collaborator Author

I think the way to do this may be to conditional-compile it for 1.16+, and use the current system for older versions. That is, we support a standalone binary on 1.16+, and 1.13+ as long as we can find the templates (e.g. go run or a build from a local checkout).

@benjaminjkraft
Copy link
Collaborator Author

Update: apparently that doesn't work because go:embed also requires the go.mod version (which is kinda-sorta the module's min-version) to be 1.16+, not just the build tag.

@StevenACoffman do you know if there's a way around this? I think we're sadly at least 6 months, maybe more, from it being reasonable to require 1.16+.

@benjaminjkraft benjaminjkraft removed this from the OSS "launch" milestone Sep 7, 2021
@benjaminjkraft benjaminjkraft added the bug Something isn't working label Sep 11, 2021
benjaminjkraft added a commit that referenced this issue Sep 30, 2021
This is an attempt to fix #9, but doesn't actually work; see the issue
for details.
@StevenACoffman
Copy link
Member

I forgot I never circled back on this because I wasn't sure what your question was. Please forgive me if you know all this and none of it is what you wanted to work around.

As you have already figured out, if a module declares go 1.15 in its go.mod, and then only imports the "embed" package in a Go file guarded by:

//go:build go1.16
// +build go1.16

Note: This guard actually didn't work for a few versions of Go, so for instance in Go 1.15, you need Go 1.15.12 or it would complain anyway. I forget what version of Go 1.14 ignoring build tag guarded embed directives was backported to.

However, you probably want to build this file with Go 1.17 as well. Multiple build tags on the same line in a .go file causes go build to interpret them using OR logic. If we put them on separate lines, then go build will interpret those tags using AND logic, instead, so I think you want to change it to:

//go:build go1.17
// +build go1.16  go1.17

And then I think the build tags for the prior to Go 1.16 versions should be:

//go:build !go1.17
// +build !go1.16
// +build !go1.17

Since //go:build is the new conditional compilation directive introduced in Go 1.17, so that's the only version that is relevant for those lines above.

Also, for the Go 1.15 file, you would need to have an alternative implementation like go-bindata or pkger or do the box thing.

@benjaminjkraft
Copy link
Collaborator Author

Unless I messed something up, the issue was none of those! You can see what I did here: e3e3e8e, and I believe it includes everything you said (except that we don't need a go1.17 tag because the go1.16 tag actually means 1.16+). The problem is that go:embed apparently also depends on the language version in the go.mod, so you can't even use it behind a go:build tag, at least as far as I can see. But I may still have been doing something wrong!

@StevenACoffman
Copy link
Member

StevenACoffman commented Oct 6, 2021

You can use go:embed behind a go:build tag only on Go 1.15.11+ assuming your go.mod also targets Go 1.15.

Assuming you have ASDF installed to manage Go versions (and you have ASDF_GOLANG_VERSION=1.15.13 and ASDF_GOLANG_VERSION=1.16.6) installed, you can demonstrate for yourself this works:

git clone git@gist.github.com:a609fe65f6b76457a620af08b5cbb8d7.git gomod-build-issue
cd gomod-build-issue
./script.sh

I ran it fine on different various different Go versions, but it does break on Go 1.15.10 and lower.

See golang/go#43980 (comment) where in Go 1.15.11:

I believe that the build tag solution now works correctly with 1.15.11 - go mod tidy is now resilient to stdlib imports which don't exist.

@benjaminjkraft
Copy link
Collaborator Author

Ah interesting. Let's just wait until we're ready to drop 1.15, I think that will be simpler than trying to make sure everyone has the right version and this doesn't seem to have caused much trouble in practice.

benjaminjkraft added a commit that referenced this issue Mar 22, 2022
Now that we're on Go 1.16+ we can do this easily! The main advantage is
it means users can build a genqlient binary and use that portably (or we
could distribute one, or whatever). Plus the code is marginally simpler;
the `embed` API is really quite nice.

Fixes #9.

Test plan:
```
make check
go build .
rm -rf generate               # pretend we have no checkout
./genqlient ./internal/integration/genqlient.yaml
./genqlient --init            # fails after generating a default config
```
benjaminjkraft added a commit that referenced this issue Mar 22, 2022
Now that we're on Go 1.16+ we can do this easily! The main advantage is
it means users can build a genqlient binary and use that portably (or we
could distribute one, or whatever). Plus the code is marginally simpler;
the `embed` API is really quite nice.

Fixes #9.

Test plan:
```
make check
go build .
rm -rf generate               # pretend we have no checkout
./genqlient ./internal/integration/genqlient.yaml
./genqlient --init            # fails after generating a default config
```
@clayne11
Copy link

I'm trying to use genqlient with Bazel and am running into issues because of the way templates are looked up. They have a hardcoded file path that isn't accurate when used within a Bazel build environment. Can we revisit this? At this point Go is on version 1.18. Is it reasonable to bump the min version to 1.16?

It looks like you've already fixed this on main. Can you cut a new version?

@benjaminjkraft
Copy link
Collaborator Author

Just released v0.5.0, which includes this. Thanks for the poke!

@clayne11
Copy link

clayne11 commented Jun 17, 2022

Thanks! I'm not seeing it though:

➜  external go list -m -versions github.com/Khan/genqlient
github.com/Khan/genqlient v0.1.0 v0.2.0 v0.3.0 v0.4.0

I did patch the current main commit into my project though and it's working beautifully! The go:embed API is really nice 😄.

@benjaminjkraft
Copy link
Collaborator Author

Whoops, forgot to actually push the tag. Try again now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants