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

Use go:embed for embedding files instead of vfsgen #17352

Open
techknowlogick opened this issue Oct 18, 2021 · 18 comments
Open

Use go:embed for embedding files instead of vfsgen #17352

techknowlogick opened this issue Oct 18, 2021 · 18 comments
Labels
topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile

Comments

@techknowlogick
Copy link
Member

Feature Description

Now that we have a minimum of go1.16 set, we can use go's embed functionality to embed templates/web assets (css/js), and translations.

Screenshots

No response

@6543
Copy link
Member

6543 commented Oct 18, 2021

@lafriks
Copy link
Member

lafriks commented Oct 18, 2021

only we will have to do some restucture of code as you can not embed files that are not in the same directory or in child directory where golang code is where you are embedding it.

@lunny
Copy link
Member

lunny commented Oct 19, 2021

go:embed don't support ../ on path, the directory structure have to be changed.

@techknowlogick techknowlogick changed the title Use go:embed for embedding files instead of vfsfen Use go:embed for embedding files instead of vfsgen Oct 19, 2021
@silverwind
Copy link
Member

Maybe symlink before build?

@jolheiser
Copy link
Member

We can add a small go file in those locations to grab the assets.

@silverwind
Copy link
Member

silverwind commented Oct 19, 2021

Another thing to consider is that vfsgen provides the gzip versions of assets which we directly serve from the http server if the client indicates gzip support. I think it's crucial for performance to retain the option to serve pre-compiled gzip assets.

Edit:

See https://github.com/vearutop/statigz for one approach to solve it. I also maintain a JS module to generate .gz and .br files at https://github.com/silverwind/precompress.

@rahulsurwade08
Copy link

Hey @techknowlogick , I would love to contribute to this issue under Hacktoberfest. How can I get started?

@6543
Copy link
Member

6543 commented Oct 6, 2022

the main concern is that if we ose std embeded the compressed files would be decompressed and compressed on each request that indicated gzip support ...

... @techknowlogick should we close this issue?

@wxiaoguang
Copy link
Contributor

There could be a comment in code for why the vfsgen is chosen, to help future developers know the background. Otherwise months later the knowledge may be lost.

@jolheiser
Copy link
Member

As well, this is still a valid issue. silverwind posted above a potential https://github.com/vearutop/statigz that allows serving embedded files via gzip

@silverwind
Copy link
Member

silverwind commented Oct 7, 2022

Yes, I think it's still a goal to use go:embed as it greatly simplifies the build by eliminating go generate and there are ways to get gzip to work by combining tools like precompress (to be ran after webpack) and statigz.

@silverwind silverwind reopened this Oct 20, 2022
@silverwind
Copy link
Member

silverwind commented Oct 20, 2022

We really need to migrate to go:embed. I think statigz isn't even needed as handling Accept-Encoding manually is rather trivial.

the main concern is that if we ose std embeded the compressed files would be decompressed and compressed on each request that indicated gzip support ...

If we have .gz and .br files in the embed, we can serve their bytes on-the-fly without ever decompressing. This will reduce both server load (it never needs to decompress) and optimize transfer size.

@lunny
Copy link
Member

lunny commented Oct 20, 2022

precompress

Doesn't vfsgen have done that?

@silverwind
Copy link
Member

silverwind commented Oct 20, 2022

Yes, but only gzip. We want Brotli as well for another ~30% size reduction over gzip.

Using go:embed with all three variants (plain,gz,br) would be best for client performance, binary size will increase a bit, but there's no more runtime decompression involved as opposed to vfsgen.

@lunny lunny added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Oct 20, 2022
@silverwind
Copy link
Member

silverwind commented Dec 22, 2022

Are we fine with dropping support for non-bindata builds (e.g. always embed, don't support non-embedded assets in the file system, except custom files). I fail to see a real use case for non-bindata builds and it just complicates the implementation a lot.

@delvh
Copy link
Member

delvh commented Dec 22, 2022

Assuming make watch will still work: I'm okay with it.
I noticed that when I compile with TAGS="bindata …" make watch,
the frontend doesn't work anymore and as I only get the raw HTML and some browser alerts, but no CSS or JS…

@silverwind
Copy link
Member

silverwind commented Dec 22, 2022

Indeed, watch mode depends on files in the file system. One solution is to have go run a reverse proxy on the assets path in dev mode, which would proxy asset requests to webpack-dev-server or similar.

@paralin
Copy link

paralin commented Feb 13, 2023

Done in #22887

paralin added a commit to paralin/gitea that referenced this issue Feb 23, 2023
go:embed is part of the standard library and does not require an extra
go:generate step.

Migrate all usages of bindata to go:embed.

Make embedded the new default (previously required bindata tag).

Add new tag "servedynamic" which serves from filesystem (old !bindata).

Accept-Encoding compression has been dropped. The assets in go:embed are not
available in a compressed form. The compression could be enabled again by adding
a compress middleware: https://github.com/vearutop/statigz

Drop vfsgen dependency (no longer required).

Fixes go-gitea#17352

Signed-off-by: Christian Stewart <christian@paral.in>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants