deps,v8: link with `atomic` for platforms lacking CAS by refack · Pull Request #23286 · nodejs/node

refack · 2018-10-05T23:30:24Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Major V8 updates are usually API/ABI incompatible with previous versions. This commit adapts NODE_MODULE_VERSION for V8 7.1. Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md

until 81a3c699d6eef936452ac3d10c7c59a2c1e38c0c

until 4274d2f1905b5e4c3cf613635a0db79fb99a9409 missing: 408896a8b41751fea92e482c5eb4b858e7ffe68d

enable v8_enable_embedded_builtins reorder conditions proccessing for `run_mksnapshot`

Fixes: nodejs/node-v8#81

nodejs-github-bot · 2018-10-05T23:30:25Z

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1133/pipeline

addaleax

LGTM (apart from the Makefile change)

addaleax · 2018-10-05T23:35:22Z

Makefile

 #   to be instrumented, see `coverage`.
 run-ci: build-ci
-	$(MAKE) test-ci
+	$(MAKE) test-ci -j1


Is this WIP?

Yes, and no. for the canary branch I float this as 6e24dcc
It's either this or #23257. One of them will need to land upstream, since now we see #22006 on a lot of platforms (make test-ci rebuilds a few files and relinks the node binary, in parallel to the script that builds the addons, so it's a race to the binary)

It's either this or #23257.

I’m not sure I follow … that PR landed, right?

I don’t think -j1 is something we want in CI…

I’m not sure I follow … that PR landed, right?

#23257 was a revert of 5d8373a, because there was a bug on macOS.

IMHO running just the make part of test-ci with -j1 is not that bad. Most of the steps have parallelism built in

node/Makefile

Lines 457 to 467 in 2ba19ff

test-ci: | clear-stalled build-addons build-addons-napi doc-only

out/Release/cctest --gtest_output=tap:cctest.tap

$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \

--mode=$(BUILDTYPE_LOWER) --flaky-tests=$(FLAKY_TESTS) \

$(TEST_CI_ARGS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES) $(CI_DOC)

@echo "Clean up any leftover processes, error if found."

ps awwx | grep Release/node | grep -v grep | cat

@PS_OUT=`ps awwx | grep Release/node | grep -v grep | awk '{print $$1}'`; \

if [ "$${PS_OUT}" ]; then \

echo $${PS_OUT} | xargs kill -9; exit 1; \

fi

For build-addons && build-addons-napi you added the script that does multi-proc. doc-only had it for a long time. and the test uses $JOBS for parallelism level...

addaleax · 2018-10-05T23:35:49Z

deps/v8/gypfiles/v8.gyp

            '../src/builtins/builtins-intl-gen.cc',
          ],
        }],
+        # Platforms that don't have CAS support need to link atomic library


I realize this is copied from the upstream comment, but it might help to spell out CAS?

refack · 2018-10-05T23:47:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/17652/

refack · 2018-10-06T00:22:24Z

I think some of the fails are real integration bugs:

19:49:52 if [ ! -r node -o ! -L node ]; then ln -fs out/Release/node node; fi
19:49:52 cd tools/doc && if [ -x /home/iojs/build/workspace/node-test-commit-linux/nodes/alpine-last-latest-x64/./node ] && [ -e /home/iojs/build/workspace/node-test-commit-linux/nodes/alpine-last-latest-x64/./node ]; then /home/iojs/build/workspace/node-test-commit-linux/nodes/alpine-last-latest-x64/./node /home/iojs/build/workspace/node-test-commit-linux/nodes/alpine-last-latest-x64/./deps/npm/bin/npm-cli.js ci; elif [ -x `which node` ] && [ -e `which node` ] && [ `which node` ]; then `which node` /home/iojs/build/workspace/node-test-commit-linux/nodes/alpine-last-latest-x64/./deps/npm/bin/npm-cli.js ci; else echo "No available node, cannot run \"node /home/iojs/build/workspace/node-test-commit-linux/nodes/alpine-last-latest-x64/./deps/npm/bin/npm-cli.js ci\""; exit 1; fi;
19:49:53 Segmentation fault (core dumped)
19:49:53 make[1]: *** [Makefile:609: tools/doc/node_modules] Error 139
19:49:53 make: *** [Makefile:484: run-ci] Error 2
19:49:53 Build step 'Execute shell' marked build as failure

targos · 2018-10-06T08:09:13Z

I think some of the fails are real integration bugs

They're unrelated to this change, I think. Maybe a temporary bug in V8.

targos · 2018-10-06T08:28:38Z

Landed in 5982e7a and 47835e2.

targos · 2018-10-06T08:43:47Z

Post-update CI: https://ci.nodejs.org/view/MyJobs/job/node-test-commit-node-v8/108/

targos and others added 7 commits October 5, 2018 07:52

deps: update V8 to 7.1.0

c679d80

build: reset embedder string to "-node.0"

7adef39

src: update NODE_MODULE_VERSION to 68

37bb409

Major V8 updates are usually API/ABI incompatible with previous versions. This commit adapts NODE_MODULE_VERSION for V8 7.1. Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md

deps: update v8.gyp

193ee73

until 81a3c699d6eef936452ac3d10c7c59a2c1e38c0c

deps: update v8.gyp

34671d2

until 4274d2f1905b5e4c3cf613635a0db79fb99a9409 missing: 408896a8b41751fea92e482c5eb4b858e7ffe68d

deps: V8 sync gypfiles with 7.1

d172625

enable v8_enable_embedded_builtins reorder conditions proccessing for `run_mksnapshot`

deps,v8: link with atomic for platforms lacking CAS

f84d485

Fixes: nodejs/node-v8#81

refack requested a review from targos October 5, 2018 23:30

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Oct 5, 2018

temprary fix for CI

6e24dcc

addaleax approved these changes Oct 5, 2018

View reviewed changes

fixup! 1

24ffb72

refack added the build Issues and PRs related to build files or the CI. label Oct 5, 2018

targos approved these changes Oct 6, 2018

View reviewed changes

targos force-pushed the canary-base branch from d172625 to 5982e7a Compare October 6, 2018 08:26

targos closed this Oct 6, 2018

refack deleted the mips-atomics branch October 6, 2018 13:43

refack mentioned this pull request Oct 18, 2018

build: spawn make test-ci with -j1 #23733

Merged

3 tasks

	test-ci: \| clear-stalled build-addons build-addons-napi doc-only
	out/Release/cctest --gtest_output=tap:cctest.tap
	$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
	--mode=$(BUILDTYPE_LOWER) --flaky-tests=$(FLAKY_TESTS) \
	$(TEST_CI_ARGS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES) $(CI_DOC)
	@echo "Clean up any leftover processes, error if found."
	ps awwx \| grep Release/node \| grep -v grep \| cat
	@PS_OUT=`ps awwx \| grep Release/node \| grep -v grep \| awk '{print $$1}'`; \
	if [ "$${PS_OUT}" ]; then \
	echo $${PS_OUT} \| xargs kill -9; exit 1; \
	fi

Uh oh!

Conversation

refack commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Oct 5, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

refack Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

refack Oct 6, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

refack Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

refack commented Oct 5, 2018

Uh oh!

refack commented Oct 6, 2018

Uh oh!

targos commented Oct 6, 2018

Uh oh!

targos commented Oct 6, 2018

Uh oh!

targos commented Oct 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

refack commented Oct 5, 2018 •

edited

Loading