Introduce esbuild on webpack

[lunny]

make the frontend build faster and use less memory. It's about 2x faster than before on my macOS.

[silverwind]

esbuild is a native dependency which would break the ability to build offline from the src tarballs on non-Linux systems as the tarballs would only contain the Linux binaries of esbuild.

I guess it's no big issue because src tarballs already contain prebuilt frontend files so a working build is still possible, but I think we would need to document that the user would have to run npm install manually if they intend to rebuild frontend from these tarballs.

[roberChen]

I really appreciate this pull, because by now the frontend cannot be built successfully on my cloud server for lack of resource when webpack is running. I have to build the frontend on my computer and then upload the target files to the server, and then to build the backend.

If this pull request can be merged. I may build the whole project just using one command.

[silverwind]

Should probably just exclude node_modules in the src tarball and just require online to build frontend.

[lunny]

Should probably just exclude node_modules in the src tarball and just require online to build frontend.

Is this related with this PR? All your previous change requests merged.

[CL-Jeremy]

Is this related with this PR? All your previous change requests merged.

esbuild is a native dependency which would break the ability to build offline from the src tarballs on non-Linux systems as the tarballs would only contain the Linux binaries of esbuild.

I proposed a solution in lunny#12. Improvements are highly welcome.

[silverwind]

Is this related with this PR?

It should be because previously, the shipped node_modules was cross-platform compatible being purely JS but this adds a native dependency breaking this compatibility.

I proposed a solution in lunny#12. Improvements are highly welcome.

Hmm, I'm not sure it's worth the effort as that method would tightly couple us to esbuild and it'd be prone to future breakage if urls or file structure change in esbuild. I'm still thinking we better exclude node_modules in the src tarball as distributors will not attempt to rebuild the frontend (they often don't even have Node.js installed), so they will just use the pre-built public files.

[CL-Jeremy]

Hmm, I'm not sure it's worth the effort as that method would tightly couple us to esbuild and it'd be prone to future breakage if urls or file structure change in esbuild. I'm still thinking we better exclude node_modules in the src tarball as distributors will not attempt to rebuild the frontend (they often don't even have Node.js installed), so they will just use the pre-built public files.

I agree then. I just had the problem with symlinks under Windows with MSYS2 (cross-compilation works) since npm works differently under Windows (creates cmd.exe and PowerShell shims instead) rather than symlinking them at node_modules\.bin. This means tarballs for *nix would error out on Windows. Not a good thing to have. (This problem wasn't obvious to me before testing the solution proposed above, as npx would just opportunistically reinstall the package temporarily and execute from there if the binary in .bin is missing or unusable, totally transparent to the user whenever Internet is available.)

[silverwind]

Yeah stuff like this is gets tricky fast. I think all that's needed here is removal of node_modules from the tar command in the Makefile and a note somewhere (probably README.md) that Node.js and an Internet connection is required to download dependencies and build the frontend.

[lunny]

Yeah stuff like this is gets tricky fast. I think all that's needed here is removal of node_modules from the tar command in the Makefile and a note somewhere (probably README.md) that Node.js and an Internet connection is required to download dependencies and build the frontend.

Did you mean:

.PHONY: release-sources
release-sources: | $(DIST_DIRS) node_modules
	echo $(VERSION) > $(STORED_VERSION_FILE)
+++	tar --exclude=./$(DIST) --exclude=./.git --exclude=./$(MAKE_EVIDENCE_DIR) --exclude=./node_modules --exclude=./$(AIR_TMP_DIR) -czf $(DIST)/release/gitea-src-$(VERSION).tar.gz .
---	tar --exclude=./$(DIST) --exclude=./.git --exclude=./$(MAKE_EVIDENCE_DIR) --exclude=./node_modules/.cache --exclude=./$(AIR_TMP_DIR) -czf $(DIST)/release/gitea-src-$(VERSION).tar.gz .

?

[silverwind]

Yes, and

diff --git a/README.md b/README.md
index ea0d83c90..4655629ad 100644
--- a/README.md
+++ b/README.md
@@ -76,9 +76,9 @@ or if sqlite support is required:

 The `build` target is split into two sub-targets:

 - `make backend` which requires [Go 1.13](https://golang.org/dl/) or greater.
-- `make frontend` which requires [Node.js 10.13](https://nodejs.org/en/download/) or greater.
+- `make frontend` which requires [Node.js 10.13](https://nodejs.org/en/download/) or greater and an Internet connection.

 If pre-built frontend files are present it is possible to only build the backend:

     TAGS="bindata" make backend

[silverwind]

Also, babel-loader will be unused after this so I think we can drop it and all babel-related dependencies with it. Also, browserslist (specified in package.json) might also be no longer in use with this.

[lunny]

Also, babel-loader will be unused after this so I think we can drop it and all babel-related dependencies with it. Also, browserslist (specified in package.json) might also be no longer in use with this.

So Gitea will not be compiled without Internet any more?

[CL-Jeremy]

So Gitea will not be compiled without Internet any more?

I think since the official Gitea tarball is only compilable under *nix, consistency wins here (i. e., if not buildable offline in Windows, then leave out the feature completely).

We could still resolve to caching all packages (not just esbuild) in a local npm cache through .npmrc though, based on the versions in package-lock.json. This way npm would install all package from cache and ensure everything happens exactly as if there were internet available. The special handling in my PR for esbuild would then be kept alongside this change.

[silverwind]

So Gitea will not be compiled without Internet any more?

It will because frontend (public dir) is shipped pre-compiled in the tarballs and go modules are also present in vendored form. What would not be possible without internet is a rebuild of the frontend.

I'm not totally opposed to lunny#12 any more to retain offline build on all platforms, but it's unfurtunately quite complex.

[CL-Jeremy]

I've update the above PR to reflect latest changes. Including a fix for building Fomantic UI (currently fails on every platform due to recent dependency update to less@4, the fix puts the entire build process into web_src/fomantic, which now only reads the cache from project root, semantic.json is symlinked every time it's needed).

The package cache has been tested to work fine on Mac, Windows and Linux (all on x64, didn't have time to test other archs) for building fomantic, frontend (webpack/esbuild) and go.

[CL-Jeremy]

Just so nobody wonders why I chose to use bsdtar: many programs on Linux prefer it over GNU tar since it's more modern (rightfully renamed libarchive-tools by Debian, a complete rewrite to BSD's original version). I'm using the regex match for apt install so if one day CI goes Ubuntu Focal the code would still work (right now bsdtar acts as a transitioning package).

The main concern at my side is of course macOS support (I work mostly on Macs): bsdtar matches differently when using --exclude <PATTERN>, and macOS only ships bsdtar and symlinks tar the former, just like gcc is actually clang. I was quite confused initially about the resulting size difference when testing the release-sources target under macOS and Linux, and wondered why initially the packages in node_modules (before this PR) all lacked ./dist when the tarball was generated on macOS. The change here ensures usability of tarballs manually generated on all supported platforms (MSYS2 also ships bsdtar by default when its dev base package is installed).

[CL-Jeremy]

Any news on this? Have been using this for over a month now. No issues so far, and webpack became blazingly fast on the workstation I'm using (~35 seconds). I mean some sort of milestone would be nice. Also, though hardly anybody is building Fomantic (I did because I wanted to have bold = 600 but that's just me), it's still a bug fixed by this.

[lunny]

Any news on this? Have been using this for over a month now. No issues so far, and webpack became blazingly fast on the workstation I'm using (~35 seconds). I mean some sort of milestone would be nice. Also, though hardly anybody is building Fomantic (I did because I wanted to have bold = 600 but that's just me), it's still a bug fixed by this.

I think it's ready to review and we need two approvals.

[zeripath]

the backticks in the lines above this are also suspect...

[silverwind]

Indeed, that's also not very make-ish.

[CL-Jeremy]

Sorry my bad... Debian uses ash and not the bash-emulated sh as various other dists (as well as macOS) do.

[CL-Jeremy]

On the other hand, backticks should be POSIX-compliant AFAIK. The bash syntax would have been $$() in Makefile.

[silverwind]

backticks should be POSIX-compliant AFAIK

They work in Makefile but it seems to have the side-effect that the command is evaluated on every variable resolution so that node command needlessly ran like 7 times. Using $(shell cmd) did only run once.

[CL-Jeremy]

Well, it seems this is actually a bug with running dash (Debian Almquist Shell) in certain environments (like Docker), as both glob and backticks should be POSIX-compliant and portable. What I aimed was to make the console output both concise and comprehendible for the user.

They work in Makefile but it seems to have the side-effect that the command is evaluated on every variable resolution so that node command needlessly ran like 7 times. Using $(shell cmd) did only run once.

This was completely unexpected. I expected this to not have to run at all until it actually gets called in the final command. To my understanding, expansion of this should happen in one go with backticks evaluation preceding other things (like glob). If this is actually the case, it should be considered as a bug as well...

[silverwind]

release-latest still borked, https://drone.gitea.io/go-gitea/gitea/37862/4/3:

bsdtar --no-xattrs --exclude=^./dist --exclude=^./.git --exclude=^./.make_evidence --exclude=node_modules --exclude=^./.air -czf dist/release/gitea-src-master.tar.gz .
bsdtar: Option --no-xattrs is not supported

[silverwind]

I think we don't need to run the fomantic-ui dependency installation on the release task at all, it's just pointless anyways with the output files checked in. The fomantic rebuild is purely a development task and does not need to be performed from release tarballs.

[silverwind]

Overall, I think we are better off by not shipping npm_cache or node_modules in the tarballs and just state that a offline UI rebuild is not supported from those tarballs (which already contain the pre-built UI).

[CL-Jeremy]

I think these issues are really just minor ones due to me not testing things on Debian/Ubuntu. I'll fix them in the next PR.

[silverwind]

Please check the bsdtar error in #14578 (comment). The version on Ubuntu 18.04 does not support --no-xattrs.

[CL-Jeremy]

Please check the bsdtar error in #14578 (comment). The version on Ubuntu 18.04 does not support --no-xattrs.

Sure. I've pushed a branch in https://github.com/CL-Jeremy/gitea some time ago, but am currently working on a rebase.

[zeripath]

I think we just have to drop the bsdtar route on the .drone.yml.

bsdtar --help doesn't give any useful information to determine if the option is supported.

we can't rely on man bsdtar being present.

and I don't think it really adds much.

[CL-Jeremy]

I think we just have to drop the bsdtar route on the .drone.yml.

bsdtar --help doesn't give any useful information to determine if the option is supported.

we can't rely on man bsdtar being present.

and I don't think it really adds much.

I've already pushed another fix. Please comment on that PR: #15256