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.

Sign up for GitHub

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

Synthesize .npmrc from Makefile; Fix bsdtar on older Debian/Ubuntu/macOS #15256

Closed
wants to merge 5 commits into from
Closed

Synthesize .npmrc from Makefile; Fix bsdtar on older Debian/Ubuntu/macOS #15256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
935bfe3
Synthesize .npmrc only before initial npm install
CL-Jeremy Apr 2, 2021
cfa1b63
Fix bsdtar on Debian/Ubuntu and legacy macOS
CL-Jeremy Apr 2, 2021
780c8ab
Fix cache/path issue with npm 7
CL-Jeremy Apr 3, 2021
bf4214d
Merge pull request #7 from CL-Jeremy/fix-npm-7-cache
CL-Jeremy Apr 3, 2021
e6174ca
Do not use bsdtar in CI
CL-Jeremy Apr 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .drone.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,7 @@ steps:
pull: always
image: techknowlogick/xgo:go-1.16.x
commands:
- curl -sL https://deb.nodesource.com/setup_14.x | bash - && apt -y install nodejs bsdtar\|libarchive-tools
- curl -sL https://deb.nodesource.com/setup_14.x | bash - && apt-get install -y nodejs
- export PATH=$PATH:$GOPATH/bin
- make release
environment:
Expand Down Expand Up @@ -591,7 +591,7 @@ steps:
pull: always
image: techknowlogick/xgo:go-1.16.x
commands:
- curl -sL https://deb.nodesource.com/setup_14.x | bash - && apt-get install -y nodejs bsdtar\|libarchive-tools
- curl -sL https://deb.nodesource.com/setup_14.x | bash - && apt-get install -y nodejs
- export PATH=$PATH:$GOPATH/bin
- make release
environment:
Expand Down
8 changes: 6 additions & 2 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,19 @@ coverage.all
/integrations/mysql8.ini
/integrations/pgsql.ini
/integrations/mssql.ini
/node_modules

# Frontend
.npmrc
.npm-cache
node_modules
/yarn.lock
/public/js
/public/serviceworker.js
/public/css
/public/fonts
/public/img/webpack
/web_src/fomantic/node_modules
/web_src/fomantic/package-lock.json
/web_src/fomantic/semantic.json
/web_src/fomantic/build/*
!/web_src/fomantic/build/semantic.js
Expand All @@ -95,7 +100,6 @@ coverage.all
!/web_src/fomantic/build/themes/default/assets/fonts/outline-icons.woff2
/VERSION
/.air
/.npm-cache

# Snapcraft
snap/.snapcraft/
Expand Down
5 changes: 0 additions & 5 deletions .npmrc
View file
Open in desktop

This file was deleted.

29 changes: 17 additions & 12 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ else

# This is the "normal" part of the Makefile

TAR := $(shell hash bsdtar > /dev/null 2>&1 && echo "bsdtar --no-xattrs" || echo "tar" )
TAR := $(shell hash bsdtar > /dev/null 2>&1 && echo "bsdtar" || echo "tar" )
ifneq ($(shell $(TAR) --version | grep "bsdtar 3"),)
TAR += --format=gnutar
endif
Comment on lines +17 to +20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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?

Copy link
Contributor Author

@CL-Jeremy CL-Jeremy Apr 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cf. relevant change suggested to #15273 for fixing Mac issue singularly.


DIST := dist
DIST_DIRS := $(DIST)/binaries $(DIST)/release
Expand Down Expand Up @@ -662,27 +665,29 @@ docs:
fi
cd docs; make trans-copy clean build-offline;

node_modules: package-lock.json
.npmrc:
echo "audit=false fund=false package-lock=true save-exact=true " | tr " " "\n" > $@

node_modules: package-lock.json .npmrc
npm install --no-save
@touch node_modules

.PHONY: npm-cache
npm-cache: .npm-cache $(FOMANTIC_WORK_DIR)/node_modules/fomantic-ui

.npm-cache: package-lock.json
.npm-cache: package-lock.json .npmrc
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}))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seriously just use the foreach again here to append the ESBUILD_VERSION

Suggested change
$(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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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=.npmrc rm cache; echo cache=.npm-cache >> .npmrc
rm -rf node_modules && npm install --no-save
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

rm -rf $(FOMANTIC_WORK_DIR)/node_modules $(FOMANTIC_WORK_DIR)/package-lock.json
@touch .npm-cache

.PHONY: npm-uncache
npm-uncache:
rm -rf .npm-cache
npm config --userconfig=$(FOMANTIC_WORK_DIR)/.npmrc rm cache
npm config --userconfig=.npmrc rm cache

.PHONY: npm-update
Expand All @@ -696,17 +701,17 @@ npm-update: node-check | node_modules
fomantic: $(FOMANTIC_DEST)

$(FOMANTIC_WORK_DIR)/node_modules/fomantic-ui:
ln -sf ../../semantic.json $(FOMANTIC_WORK_DIR)
cd $(FOMANTIC_WORK_DIR); \
cp "$$OLDPWD/semantic.json" ./; \
rm -rf node_modules && mkdir node_modules && \
npm install fomantic-ui; \
npm install --no-optional --cache="$$OLDPWD/.npm-cache"; \
rm -f semantic.json
@touch $(FOMANTIC_WORK_DIR)/node_modules

$(FOMANTIC_DEST): $(FOMANTIC_CONFIGS) $(FOMANTIC_WORK_DIR)/node_modules/fomantic-ui
ln -sf ../../semantic.json $(FOMANTIC_WORK_DIR)
rm -rf $(FOMANTIC_WORK_DIR)/build
cd $(FOMANTIC_WORK_DIR); \
rm -rf build; \
cp "$$OLDPWD/semantic.json" ./; \
cp -f theme.config.less node_modules/fomantic-ui/src/theme.config; \
cp -rf _site node_modules/fomantic-ui/src/; \
npx gulp -f node_modules/fomantic-ui/gulpfile.js build; \
Expand Down
3 changes: 0 additions & 3 deletions web_src/fomantic/.npmrc
View file
Open in desktop

This file was deleted.

5 changes: 5 additions & 0 deletions web_src/fomantic/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"dependencies": {
"fomantic-ui": "^2.8.7"
}
}