Render templates from embed.FS

[dominik-lekse]

Implements #88

[unrolled]

Hello and thanks for the PR! I love the idea of adding the embed to the package, but I'm little hesitant to bump the required go version. I did a quick GitHub search and found a few projects using render with a lower version than 1.16.

I haven't tested this, but could we use build tags to get this in without bumping the go.mod version? If we added // +build go1.16 at the top of the fs_embed.go file, it should only include when it can (possible needed in the test file as well)?

[dominik-lekse]

Thanks @unrolled for the quick review. I agree that bumping the go version might not be the best idea.

I tried the approach you suggested, i.e. add build tag and keep the original version in go.mod. Refer to commit bfb890b

Unfortunately, the go 1.16 compiler is not happy with the go:embed directive together wit go 1.12 in go.mod and gives the following error:

./fs_embed_test.go:12:3: go:embed requires go1.16 or later (-lang was set to go1.12; check go.mod)

This has been discussed in golang/go#43980

The expectation is that you don't use embed until you are ready to bump your users forward. Build tags are not sufficient, precisely because of tools that need to analyze everything, such as go mod tidy.

Originally posted by @rsc in golang/go#43980 (comment)

The go:embed directive is required to load the fixtures for the tests not for the EmbedFileSystem itself.

In the recent commit, I have pushed a possible solution with the following changes:

• Keep module version of github.com/unrolled/render at 1.12
• Move fs_embed_test.go into a sub module github.com/unrolled/render/embed which requires go 1.16
• Copy of fixtures/ at embed/fixtures/ because //go:embed cannot .. in file system hierarchy

At a first glance these changes do not seem to look the cleanest approach, but they are a compromise which comply with these objectives:

• Keep module backwards compatibility
• Allow Go 1.16 projects to use github.com/unrolled/render.EmbedFileSystem
• Have unit tests for EmbedFileSystem (although in a sub module)

I would be happy with a more simple, straightforward solution.

[unrolled]

This is looking awesome, the idea of a separate module for tests is smart! 👍

Couple of notes:

• Can we change the embed folder and package name to embedtests? I think it would be confusing for some folks as they might try to look for the the embed code in that folder. This would just save some confusion and make it super clear what the folder/package is doing.
• We can remove the build tags from the embed test files since the module declares go1.16.
• I'm thinking a bash script in the embedtests folder would be an ideal way of running the tests without duplicating the fixtures directory... something like embedtests/test.sh:

#!/bin/bash
set -o errexit

# Copy fixtures from one level up.
rm -rf ./fixtures
cp -r ../fixtures ./fixtures

# Run tests.
go test -v -race -tags=integration

• Likely need to add a gitignore rule for that fixtures folder so we don't accidentally commit it when running tests locally.
• I gave you permission to run the tests on GitHub Actions, but the older versions are failing. We are going to need a second test run config for the embed stuff with it's own version matrix (but just 1.16 for now).

[rsc]

Bumping the go version in go.mod does not require all users to have that Go version.
It only "unlocks" the use of newer Go features when using a new toolchain.
https://golang.org/ref/mod#go-mod-file-go has some details.

The go mod tidy comment does still apply, though.

[rsc]

Actually, I have been corrected. Go 1.15.10 and Go 1.16.3 understand to ignore missing standard library packages in go mod tidy. But you will still have the problem in Go 1.14 (no longer supported).

[unrolled]

@rsc Would bumping the version in go.mod to Go 1.16, and adding the build tag to our new fs_embed.go file be the correct solution here? I'm thinking without the build tag some IDEs would show incorrect error messages about compatibility.

[rsc]

Yes, to adopt embed for Go 1.16 you need to both bump the go.mod go version.

To keep Go 1.15 and earlier builds happy you also need a build tag to not use
that file in pre-Go 1.16 builds (and presumably an alternate pre-Go 1.16-only file).

The part about "go mod tidy" not liking this setup is fixed as of Go 1.15.10,
but it's also true that you should only run "go mod tidy" using a single version
of Go, because the details can change from release to release. Once you bump
to "go 1.16" in the go.mod it would make sense to only run "go mod tidy" in a
Go 1.16 toolchain.

[unrolled]

@rcs Thanks for your insights, this makes a lot more sense now.

@dominik-lekse Your first set of changes was pretty much perfect, sorry for the confusion! We just need the build tag at the top of the fs_embed.go and fs_embed_test.go files. I pulled down your changes to test it out with multiple versions of go (with go.mod set to Go 1.16) and everything looks awesome:

go mod tidy
go version
    > go version go1.16.3 darwin/amd64
go test -race -tags=integration
    > PASS
    > ok  	github.com/unrolled/render	0.604s

go get golang.org/dl/go1.14
go1.14 download
go1.14 version
    > go version go1.14 darwin/amd64
go1.14 test -race -tags=integration
    > PASS
    > ok  	github.com/unrolled/render	0.288s

go get golang.org/dl/go1.12
go1.12 download
go1.12 version
    > go version go1.12 darwin/amd64
go1.12 test -race -tags=integration
    > PASS
    > ok  	github.com/unrolled/render	1.278s

[dominik-lekse]

Thanks @rsc for joining the issue and clarifying role of the version in go.mod.

To summarize my understanding from the discussion: The version provided in go.mod is the minimum version of the go tool chain required to build the module. Other modules, despite having a lower version in go.mod, can still depend on a library module, if the library module ensures backwards compatibility of the sources using build tags. In other words, the go.mod version of a library alone will not require the consuming module to upgrade its tool chain.

@unrolled The PR now includes the initial proposal + build tag constraints in fs_embed.go and fs_embed_test.go as you proposed. The changes are force pushed so that there is a more clean history. GitHub archeologists find the commits which have been emerged earlier in this discussion but are not longer part of the PR in the branch https://github.com/dominik-lekse/render/tree/feature/embed-fs-go1.12.

To confirm, I also have run tests with previous go versions:

$ docker run --rm -v "$(pwd):/src" -e CGO_ENABLED=0 -w /src golang:1.16.4-alpine3.13 go test -tags=integration

PASS
ok      github.com/unrolled/render      0.115s

$ docker run --rm -v "$(pwd):/src" -e CGO_ENABLED=0 -w /src golang:1.15.12-alpine3.13 go test -tags=integration

PASS
ok      github.com/unrolled/render      0.190s

$ docker run --rm -v "$(pwd):/src" -e CGO_ENABLED=0 -w /src golang:1.14.15-alpine3.13 go test -tags=integration

PASS
ok      github.com/unrolled/render      0.166s

$ docker run --rm -v "$(pwd):/src" -e CGO_ENABLED=0 -w /src golang:1.13.15-alpine3.12 go test -tags=integration

PASS
ok      github.com/unrolled/render      0.170s

[unrolled]

@dominik-lekse Thanks for all the time you put into this!