Synthesize .npmrc from Makefile; Fix bsdtar on older Debian/Ubuntu/macOS #15256
Conversation
|
Some related concerns:
|
GiteaBot
added
the
lgtm/need 2
lunny
added
the
topic/build
Fix cache/path issue with npm 7
Yes, but it was already broken. A fix has been pushed after quite some testing. Should at least be an improvement to this PR (if not the best solution to the issues).
No, not really. It was more for formal consistency across platforms. GNU Tar is indeed more conservative with options hidden behind flags. The summary in OP may look scary and seemingly prone to breakages, but the solution is rather uncomplicated. As for using |
|
Sorry for not posting this in #14578 as I was informed that commenting on closed PR is bad practice. I did refer to the first patch in OP and thought that would be enough. |
| TAR := $(shell hash bsdtar > /dev/null 2>&1 && echo "bsdtar" || echo "tar" ) | ||
| ifneq ($(shell $(TAR) --version | grep "bsdtar 3"),) | ||
| TAR += --format=gnutar | ||
| endif |
There was a problem hiding this comment.
I vote we do not have any more bsdtar stuff here. By all means allow bsdtar users to set an environment variable to use it like this:
| TAR := $(shell hash bsdtar > /dev/null 2>&1 && echo "bsdtar" || echo "tar" ) | |
| ifneq ($(shell $(TAR) --version | grep "bsdtar 3"),) | |
| TAR += --format=gnutar | |
| endif | |
| TAR ?= tar |
So that if you run: TAR="bsdtar --no-xattrs --format=gnutar" make ... you use it.
I'd even state that this:
$(eval EXCL := --exclude=$(shell [ ! "$(TAR)" = "tar" ] && echo "^" )./)
needs to go too but I'm not averse to hacks like detecting if the provided tar is bsdtar e.g.
$(eval EXCL := --exclude=$(shell [ "$(TAR)" = bsdtar* ] && echo "^" )./)
Why are we even still going through this - what are the real benefits to using bsdtar? if you're going to use the --format=gnutar does it even have any improvement anymore?
There was a problem hiding this comment.
It started because macOS doesn't ship GNU Tar and aliases bsdtar as tar, so the original script led to wrong files being included (which your new fix doesn't handle). The last PR was very close to a good solution already. I'm not against removing it from .drone.yml anyway.
bsdtar/libarchive still comes with all the benefits with file handling and compression and the like when using a different output stream writer, and as stated the (good) compatibility of these archives are documented clearly.
There was a problem hiding this comment.
Cf. relevant change suggested to #15273 for fixing Mac issue singularly.
| rm -rf .npm-cache | ||
| $(eval ESBUILD_VERSION := $(shell node -p "require('./package-lock.json').dependencies.esbuild.version")) | ||
| npm config --userconfig=.npmrc set cache=.npm-cache | ||
| $(eval ESBUILD_PLATFORMS := darwin-64 $(foreach arch,arm arm64 32 64,linux-${arch}) $(foreach arch,32 64,windows-${arch})) |
There was a problem hiding this comment.
Seriously just use the foreach again here to append the ESBUILD_VERSION
| $(eval ESBUILD_PLATFORMS := darwin-64 $(foreach arch,arm arm64 32 64,linux-${arch}) $(foreach arch,32 64,windows-${arch})) | |
| $(eval ESBUILD_PLATFORMS := darwin-64 $(foreach arch,arm arm64 32 64,linux-${arch}) $(foreach arch,32 64,windows-${arch})) | |
| $(eval ESBUILD_BUILDS := $(foreach platform, $(ESBUILD_PLATFORMS), esbuild-@$(ESBUILD_VERSION) |
There was a problem hiding this comment.
As stated in OP, that'll mean these stuff gets output to the console. I'm not a fan of overly long lines either in source or log. Using xargs wouldn't have an impact on performance AFAICT.
| npm config --userconfig=$(FOMANTIC_WORK_DIR)/.npmrc set cache=../../.npm-cache | ||
| echo $(foreach build, darwin-64 $(foreach arch,arm arm64 32 64,linux-${arch}) $(foreach arch,32 64,windows-${arch}), esbuild-${build}@$(ESBUILD_VERSION)) | tr " " "\n" | xargs -n 1 -P 4 npm cache add | ||
| rm -rf $(FOMANTIC_WORK_DIR)/node_modules | ||
| echo $(ESBUILD_PLATFORMS) | xargs printf "esbuild-%s@$(ESBUILD_VERSION)\n" | xargs -n 1 -P 4 npm cache add |
There was a problem hiding this comment.
| echo $(ESBUILD_PLATFORMS) | xargs printf "esbuild-%s@$(ESBUILD_VERSION)\n" | xargs -n 1 -P 4 npm cache add | |
| echo $(ESBUILD_BUILDS) | xargs -n 1 -P 4 npm cache add |
|
@lunny Probably other Mac-focused users could share their thoughts. |
|
Not really happy with the I'll see what can be done for #14578 (comment). I much prefer to keep things simple and not support UI rebuilds from tarballs. |
|
Alternative: #15273 |
- Don't package node_modules in tarballs, they are not cross-platform anymore and npm cache should not be messed with directly. Instead, require an internet connection to rebuild the UI, which is not necessary in the general use case because prebuilt UI files are shipped in the public directory. - Simplify the fomantic build and make the target phony. We don't need anything more for something that is rarely ran. - Use regular tar again to build tarballs and add variable for excludes - Disable annoying npm update notifications Fixes: go-gitea#14578 Fixes: go-gitea#15256 Fixes: go-gitea#15262
- Don't package node_modules in tarballs, they are not cross-platform anymore and npm cache should not be messed with directly. Instead, require an internet connection to rebuild the UI, which is not necessary in the general use case because prebuilt UI files are shipped in the public directory. - Simplify the fomantic build and make the target phony. We don't need anything more for something that is rarely ran. - Use regular tar again to build tarballs and add variable for excludes - Disable annoying npm update notifications Fixes: #14578 Fixes: #15256 Fixes: #15262 Co-authored-by: 6543 <6543@obermui.de>
- Don't package node_modules in tarballs, they are not cross-platform anymore and npm cache should not be messed with directly. Instead, require an internet connection to rebuild the UI, which is not necessary in the general use case because prebuilt UI files are shipped in the public directory. - Simplify the fomantic build and make the target phony. We don't need anything more for something that is rarely ran. - Use regular tar again to build tarballs and add variable for excludes - Disable annoying npm update notifications Fixes: go-gitea#14578 Fixes: go-gitea#15256 Fixes: go-gitea#15262 Co-authored-by: 6543 <6543@obermui.de>
Summary on
bsdtar:bsdtar(3.3.x+) and, when SELinux is enabled (by default on Fedora), requires--no-xattrsor a--formatflag with similar effectbsdtar(3.1.x/3.2.x) until recently (Debian 10/Ubuntu 20.04), which does not support--no-xattrs, but supports--format=gnutarwhich is largely the same:bsdtar/libarchive by default, but uses a different style for storing extended attributes (disabled by default for compatibility)gnutarwriter of libarchive will ignore thembsdtarfrom 2010 (2.8.3) which by default doesn't store extended attributes at all (unless--format=paxis passed), so no flag is needed at allbsdtar3.3.x+, which is functionally equivalent to Linux distros shipping this version and thus handled as case 1Note that
bsdtaris achieving better compression (among other benefits) even with--format=gnutarenabledSummary on changes to
Makefile:.npmrcis now synthesized right before initialnpm install(but will be included in the tarball), so edits of npm configs should now take place inMakefile(these files are added to .gitignore).npm-cacheis present), orgit update-index --skip-worktree(decoupling the file from working tree if it has uncommitted changes, breaking all kinds of stuff)printfis leveraged instead oftrto avoid expansion partially, reducing console outputFix: #15251
Edit: Refer to comment for details on the merged fix: CL-Jeremy#7 (comment)
Edit 2: Reference regarding TAR/libarchive features and effectiveness of
--format=gnutaroption: libarchive/libarchive#242 http://cdrtools.sourceforge.net/private/portability-of-tar-features.html