Migrate from bindata to go:embed

[paralin]

Migrate from bindata to go:embed

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

Fixes #17352.

[delvh]

See also #17352.
Does this PR fix what has been discussed there?

[paralin]

@delvh It uses go:embed statements and then references those in other packages. The only thing not addressed is Accept-Encoding compression. The assets in go:embed are not compressed, so I dropped the Accept-Encoding check.

However as mentioned in #17352 it could be added back by adding https://github.com/vearutop/statigz

[wxiaoguang]

I haven't looked into details about all changes. But for bindata part, I think it's better to keep the old behavior and the old tag, reasons:

• Keeping old tag won't break 3rd build pipelines, I know many teams build Gitea by themselves.
• When a developer runs Gitea, they do not need the embedded data, they just want to use local assets and debug quickly, with default tags. For me, I do not want to set a tag to exclude embedded when developing Gitea everytime.

Just my opinion.

[silverwind]

This embeds uncompressed assets so binary size will go up by a few tens of MBs after this. I'm not against landing uncompressed now and doing compression later as having go:embed work at all is good first step towards modernizing the asset embeds.

[lunny]

Sorry, I think we need a better resolution to replace the old system. uncompressed assets are unacceptable to me.

[silverwind]

Then let's add statigz or similar. I think we can get away with only compressing to brotli because pretty much all browsers support it.

I guess https://github.com/webpack-contrib/compression-webpack-plugin#using-brotli would be ideal. For options, you can use

  params: {
    [constants.BROTLI_PARAM_MODE]: constants.BROTLI_MODE_TEXT,
    [constants.BROTLI_PARAM_QUALITY]: constants.BROTLI_MAX_QUALITY,
  }

[silverwind]

Question is thought whether want to still support serving uncompressed assets or if browser brotli support can be made a requirement.

If we want to serve uncompressed with only brotli embedded, it would require a CGO dependency to decompress at runtime, which I think we should not take.

Otherwise we could do gzip like vfsgen, but that is a suboptimal solution in terms of both binary size and web performance. Or maybe even embed both gzip and brotli at the same time.

[silverwind]

Actually https://github.com/andybalholm/brotli might work for a pure golang brotli decoder, avoiding CGO.

[silverwind]

I think we may not actually need statigz at all, we can just:

1. Make webpack output .br files, deleting original output files
2. go:embed the directories as usual
3. On HTTP request, check Accept-Encoding, if it has "brotli" serve byte stream directly from embed (fast path), if it doesn't, decompress at runtime and serve result (slow path).

Depending on how long these decompressions take, it might be good to keep the results in a memory cache, but I imagine it may be fast enough.

[lunny]

I think we may not actually need statigz at all, we can just:

1. Make webpack output .br files, deleting original output files

2. go:embed the directories as usual

3. On HTTP request, check Accept-Encoding, if it has "brotli" serve byte stream directly from embed (fast path), if it doesn't, decompress at runtime and serve result (slow path).

Depending on how long these decompressions take, it might be good to keep the results in a memory cache, but I imagine it may be fast enough.

Sounds good. The last requirement is developers don't want compress when it's in dev mode I think.

[silverwind]

Yes, disable the webpack plugin in development. Development should not use go:embed, but instead use the assets from the filesystem that webpack continously outputs, e.g. same behaviour as now with the bindata tag absent.

[wxiaoguang]

Sorry for bothering, is this PR active? It seems there is no progress for some review comments.

[paralin]

It's active insofar I'm still interested in merging it, but there were a number of comments around wanting to use compression for the assets and I'm not really in a position to implement that right now, so if someone else will have to edit the PR for that.

[wxiaoguang]

Thank you very much for your contribution. Since we have a refactoring PR for "custom/static/builtin" assets, there are a lot of conflicts.

So at the moment I can see some possible decisions for this PR:

1. Keep it open as-is, but it would be stale forever (does it make sense?).
2. Resolve conflicts and keep it open (I guess resolving conflicts would spend more time than starting a new PR from latest main branch, because the "assets" system has changed a lot).
3. Start a clear new PR (according to 2), and at least the "bindata" tag should be kept as before.
4. Or do you have other proposals?

What do you think about it?

[silverwind]

I'd merge this even without compression, but some numbers of binary size before and after would be good, so we can judge the impact of missing compression.

It's an important step to get this done as it simplifies the build a lot. I can collaborate adding at least the compressed from webpack side.

[paralin]

I'll have a look at adjusting this PR to use the bindata tag and resolve conflicts

Edit: I'm quite busy at the moment so it might take a while for me to get around to this, sorry

[lunny]

I'd merge this even without compression, but some numbers of binary size before and after would be good, so we can judge the impact of missing compression.

It's an important step to get this done as it simplifies the build a lot. I can collaborate adding at least the compressed from webpack side.

I cannot see it's necessary to merge without compressing. It's a regression for end-user usage. What is the problem of the old embed method for the end user?

[silverwind]

You're right, it is a regression for the end user if we don't handle compression. Right now they get gzip assets when the browser indicates it in accept-encoding.

So I guess let's get this branch up to date and then we can potentially collaborate on it to add compression back.

For me, it's mostly about the simplification of the build, so it's better development experience.

[SuperSandro2000]

Then let's add statigz or similar. I think we can get away with only compressing to brotli because pretty much all browsers support it.

brotli is only good for text based files. Anything binary like png's is better compressed using gzip.

[wxiaoguang]

brotli is only good for text based files. Anything binary like png's is better compressed using gzip.

Nope. https://stackoverflow.com/questions/11289369/why-png-size-doesnt-change-after-using-http-gzip-compression

[silverwind]

brotli is only good for text based files. Anything binary like png's is better compressed using gzip.

Brotli has non-text modes as well, but we don't really need to dive into it so deeply. I would compress those anyways for simplicity's sake in the first iteration of this as the difference will be neglible and it simplifies the implementation.

[silverwind]

I've added support for generic and font broti modes in precompress.

I think I will also implement a feature to it to take assets from one directory and output compressed ones into another directory with subfolders intact. I guess such an option would be easiest for gitea to implement so we can leave the public directory alone and output into a temporary public-br directory for go:embed to pick up the files during the build.

[wxiaoguang]

Stale and it seems that nobody is interested in this change?

[silverwind]

We need compression before we can merge this, gzip is fine, brotli even better.

[wxiaoguang]

If there is no progress ..... there seems no other choice besides close it (also due to the conflicts)

[silverwind]

To summarize the situation:

1. go:embed does not feature compression which is needed for both ideal binary size and client performance. The only way to get it to work with current go:embed is to pre-compress the assets outside of go in a separate tool (precompress module or webpack plugin), which complicates the build
2. To use brotli (the ideal compression), we need a runtime brotli decoder for better client support and https://github.com/andybalholm/brotli is the only one in existance which is a machine-generated translation from C, which apparently is also very big (5mb+), so not ideal.

https://github.com/thealetheia/broccoli seems to take care of point 1 but seems unmaintained. Point 2 can not be resolved without go:embed gaining compression support, but I can accept it for the sake of better client performance.

[wxiaoguang]

A better solution: Use go:embed with server-side gzip support #26533

[wxiaoguang]

Stale. Free free to reopen if some comments like #22887 (comment) are addressed.