Support `embed` package. #1145

visualfc · 2022-09-10T04:52:26Z

support go:embed

gopherjs build/test/serve pkg
gopherjs run/build simple.go

dep pkg github.com/visualfc/goembed

Fixes #997

nevkontakte

@visualfc thank you for this request, it is something I wished to do for a long time, although couldn't find time for.

I left a new minor comments, and I have a few more general questions and suggestions:

Have you attempted to measure how much more space the embedded data occupies in the compiled source compared to the original? Although this isn't something we have to do in this PR, investigating if we could compress the embedded data somehow would be a great improvement (from the issue #136 perspective especially).
To help me better understand how this PR works, could you post the generated js_embed.go source for some package? embed/internal/embedtest would be a good test subject.
How does gopherjs handle //go:embed instructions in files that do not import the embed package? Does it ignore them or error out? Is it the same as the official compiler?
A good first step to test this PR would be removing embed/internal/embedtest package from .std_test_pkg_exclusions and making sure the CI still passes.

build/build.go

nevkontakte · 2022-09-10T10:54:43Z

build/build.go

+	root := dirList[0]
+	ctx := build.Default
+	ctx.UseAllFiles = true
+	ctx.ReadDir = func(dir string) (infos []fs.FileInfo, err error) {
+		for _, f := range filenames {
+			info, err := os.Stat(f)
+			if err != nil {
+				return nil, err
+			}
+			infos = append(infos, info)
+		}
+		return
+	}
+	p, err := ctx.Import(".", root, 0)
+	if err != nil {
+		return err
+	}


Interesting, am I correctly understanding that what this does is limits the list of files the build context will see when importing the package to only the files listed in the command line, and then delegate the rest of the build.Package construction?

I think this new code might behave differently from the old one in case the user lists a mix of regular and test sources. We might have to combine all of the GoFiles flavors to get the correct behavior.

@visualfc can you please confirm I understood this code correctly?

build/build.go

build/embed.go

nevkontakte · 2022-09-10T15:09:35Z

Also cc @flimzy who at some point planned to work on this feature, in case he has additional insights.

flimzy

I'm really happy to see this PR! Thanks for doing it!

I haven't tried running any of it yet, but left a couple comments inline, mostly stylistic.

It looks like all the heavy lifting is done in github.com/visualfc/goembed. I see it's being used by at least a couple other packages, but I do wonder if it would make sense to copy that logic into gopherjs directly. I'm curious of @nevkontakte 's thoughts.

I may be wrong, but I don't think we have any precedent for core compiler logic in an external package that doesn't come from upstream. But maybe that's not important anyway.

build/build.go

build/embed.go

compiler/natives/src/embed/embed.go

visualfc · 2022-09-11T04:39:54Z

this PR current embed generate code on gopherjs/build used by ast.
not support embed/internal/embedtest.TestAliases.
maybe move to gopherjs/compiler for used by types.Type to support aliases.
remove github.com/visualfc/goembed. like linkname parse //go:embed to direct translate js pkg init code.

embed go1.16 : use ast
embed go1.17 : use types for support aliases

// +build go 1.17
var (
	//go:embed "testdata/hello.txt"
	helloT []T
)

type T byte

nevkontakte · 2022-09-11T14:11:00Z

I may be wrong, but I don't think we have any precedent for core compiler logic in an external package that doesn't come from upstream. But maybe that's not important anyway.

@flimzy this is not entirely unprecedented, github.com/neelance/astrewrite or github.com/shurcooL/httpfs, which are pretty important to the project. So for the purposes of this PR, I don't mind adding github.com/visualfc/goembed as a dependency, although I am equally open to moving the core logic into the gopherjs repository as an internal package.

this PR current embed generate code on gopherjs/build used by ast. not support embed/internal/embedtest.TestAliases. maybe move to gopherjs/compiler for used by types.Type to support aliases. remove github.com/visualfc/goembed. like linkname parse //go:embed to direct translate js pkg init code.

embed go1.16 : use ast embed go1.17 : use types for support aliases
// +build go 1.17
var (
	//go:embed "testdata/hello.txt"
	helloT []T
)

type T byte

Considering we are only targeting compatibility with a single Go version (1.18.x at this time), I think we only need to worry about Go 1.17+ behavior. That said, I am open to accepting this PR with TestAliases skipped, and then addressing it in a separate PR. Which ever way you prefer :)

visualfc · 2022-09-12T12:39:18Z

@nevkontakte

refactoring embedFiles code and support alias string or []byte, but valid check by next types.check ( compiler ) .

embedFiles rewrite ast for go:embed vars
embedFiles generate js_embed.go only __gopherjs_embed_buildFS__ and go:embed const data

package main

import (
	"embed"
	_ "unsafe"
)

//go:linkname __gopherjs_embed_buildFS__ embed.buildFS
func __gopherjs_embed_buildFS__(list []struct {
	name string
	data string
	hash [16]byte
}) (f embed.FS)

const (
	__gopherjs_embed_74657374646174612f64617461312e747874__ = "\x68\x65\x6c\x6c\x6f\x20\x64\x61\x74\x61\x31"
	__gopherjs_embed_74657374646174612f64617461322e747874__ = "\x68\x65\x6c\x6c\x6f\x20\x64\x61\x74\x61\x32"
	__gopherjs_embed_74657374646174612f7375622f64617461312e747874__ = "\x73\x75\x62\x20\x64\x61\x74\x61\x31"
	__gopherjs_embed_74657374646174612f7375622f64617461322e747874__ = "\x73\x75\x62\x20\x64\x61\x74\x61\x32"
)

enable embed/internal/embedtest. skip TestUninitialized reflect.DeepEqual error for empty slice and variadic #1147

nevkontakte · 2022-09-13T21:17:59Z

@visualfc thank you for making these fixes. This week has been kind of busy for me so far, but I'll try to take a detailed look at your PR and so some additional testing in the next couple of days. Sorry about the delay.

build/build.go

build/embed.go

nevkontakte · 2022-09-18T13:53:07Z

@visualfc I left what I think is the final round of comments, all of which a minor style suggestions.

I also familiarized myself with the with the github.com/visualfc/goembed and I think I mostly understand what's going on :)

One thing to note though. https://github.com/visualfc/goembed/blob/main/fsys/fsys.go seems to be derived from https://github.com/golang/go/blob/master/src/cmd/go/internal/fsys/fsys.go. Would you mind adding the appropriate attribution notice to it, similar to what you have in https://github.com/visualfc/goembed/blob/master/resolve/resolve.go?

compiler/natives/src/embed/internal/embedtest/embed_test.go

nevkontakte

The pull request looks good to me! 🎉

@visualfc could you please squash all commits into one? We should be good to merge then.

visualfc · 2022-09-21T12:43:58Z

The pull request looks good to me! 🎉

@visualfc could you please squash all commits into one? We should be good to merge then.

create new PR #1153

nevkontakte · 2022-09-21T21:25:37Z

Force-pushing into this PR would have been fine too, but in any case #1153 has been merged, so I'm closing this one. Thank you again for your contribution!

visualfc added 5 commits September 9, 2022 21:38

compiler/natives/src: add embed

61801ed

compiler/natives: go generate

b0db6f5

build: check and generate embed file for build/test

522585d

build: BuildFiles support check embed file for build/run

561171d

tests/testdata/legacy_syscall: fix go:build

7dd9288

nevkontakte requested changes Sep 10, 2022

View reviewed changes

nevkontakte changed the title ~~support embed~~ Support embed package. Sep 10, 2022

flimzy reviewed Sep 10, 2022

View reviewed changes

build/build.go Outdated Show resolved Hide resolved

build/embed.go Outdated Show resolved Hide resolved

build/embed.go Outdated Show resolved Hide resolved

compiler/natives/src/embed/embed.go Outdated Show resolved Hide resolved

goembed v0.2.3

2a8f084

checkEmbed: check load file error

092a24b

visualfc added 6 commits September 11, 2022 22:22

build: move checkEmbed to BuildPackage

4316b6d

build: checkEmbed unknown type check error

b4fad18

build: embedFiles

7c483c8

compiler/natives/src: prealloc embed

aec0b57

build: embedFiles rewrite ast for go:embed vars

7cb07f3

compiler/natives/src: add embed/internal/embedtest

f4c2418

visualfc added 3 commits September 12, 2022 20:51

build: BuildFiles ctx.ReadDir prealloc

0c19332

build: joinEmbedPatternPos

625d4cc

goembed v0.2.5

1e0f65a

visualfc added 6 commits September 14, 2022 15:48

goembed v0.2.7

a80e720

goembed v0.2.8

2a6f9de

goembed v0.3.0 go:embed check multiple file error if kind not EmbedFiles

fd891b2

goembed v0.3.1

ffe4555

build: embedFiles check goembed.EmbedMaybeAlias

c5140d5

x

b612190

goembed v0.3.2

a9d764a

nevkontakte requested changes Sep 18, 2022

View reviewed changes

build/build.go Outdated Show resolved Hide resolved

build/build.go Show resolved Hide resolved

build/embed.go Outdated Show resolved Hide resolved

build/embed.go Outdated Show resolved Hide resolved

build/embed.go Show resolved Hide resolved

nevkontakte reviewed Sep 18, 2022

View reviewed changes

compiler/natives/src/embed/internal/embedtest/embed_test.go Outdated Show resolved Hide resolved

visualfc added 3 commits September 19, 2022 08:34

Merge remote-tracking branch 'upstream/master' into embed

b799041

goembed v0.3.3

8fb8e58

remove compiler/natives/src/embed/internal/embedtest

c552a3d

nevkontakte approved these changes Sep 21, 2022

View reviewed changes

nevkontakte mentioned this pull request Sep 21, 2022

build: support go:embed #1153

Merged

nevkontakte closed this Sep 21, 2022

Support embed package. #1145

Support embed package. #1145

Uh oh!

Conversation

visualfc commented Sep 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nevkontakte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nevkontakte Sep 10, 2022

Choose a reason for hiding this comment

Uh oh!

nevkontakte Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nevkontakte commented Sep 10, 2022

Uh oh!

flimzy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

visualfc commented Sep 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nevkontakte commented Sep 11, 2022

Uh oh!

visualfc commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nevkontakte commented Sep 13, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nevkontakte commented Sep 18, 2022

Uh oh!

Uh oh!

nevkontakte left a comment

Choose a reason for hiding this comment

Uh oh!

visualfc commented Sep 21, 2022

Uh oh!

nevkontakte commented Sep 21, 2022

Uh oh!

Uh oh!

Support `embed` package. #1145

Support `embed` package. #1145

visualfc commented Sep 10, 2022 •

edited

Loading

visualfc commented Sep 11, 2022 •

edited

Loading

visualfc commented Sep 12, 2022 •

edited

Loading