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

Jump to bottom

deps,v8: link with atomic for platforms lacking CAS #23286

Closed
wants to merge 9 commits into from
Closed

deps,v8: link with atomic for platforms lacking CAS #23286

wants to merge 9 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 5, 2018 •
edited
Loading

Fixes: nodejs/node-v8#81

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

targos and others added 7 commits October 5, 2018 07:52
@targos
deps: update V8 to 7.1.0
c679d80
@targos
build: reset embedder string to "-node.0"
7adef39
@targos
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
@targos
deps: update v8.gyp
193ee73 
until 81a3c699d6eef936452ac3d10c7c59a2c1e38c0c
@targos
deps: update v8.gyp
34671d2 
until 4274d2f1905b5e4c3cf613635a0db79fb99a9409

missing: 408896a8b41751fea92e482c5eb4b858e7ffe68d
@refack @targos
deps: V8 sync gypfiles with 7.1
d172625 
enable v8_enable_embedded_builtins
reorder conditions proccessing for `run_mksnapshot`
@refack
deps,v8: link with atomic for platforms lacking CAS
f84d485 
Fixes: nodejs/node-v8#81
@refack refack requested a review from targos October 5, 2018 23:30
@nodejs-github-bot
Copy link
Collaborator

@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

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Oct 5, 2018
addaleax
addaleax approved these changes Oct 5, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM (apart from the Makefile change)

Makefile
@@ -481,7 +481,7 @@ build-ci:
# - node-test-commit-linux-coverage: where the build and the tests need
# to be instrumented, see `coverage`.
run-ci: build-ci
$(MAKE) test-ci
$(MAKE) test-ci -j1
Copy link
Member

Choose a reason for hiding this comment

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

Is this WIP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

deps/v8/gypfiles/v8.gyp Outdated
@@ -361,6 +361,18 @@
'../src/builtins/builtins-intl-gen.cc',
],
}],
# Platforms that don't have CAS support need to link atomic library
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

@refack
Copy link
Contributor Author

refack commented Oct 5, 2018

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

@refack refack added the build Issues and PRs related to build files or the CI. label Oct 5, 2018
@refack
Copy link
Contributor Author

refack commented Oct 6, 2018

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
Copy link
Member

targos commented Oct 6, 2018

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
Copy link
Member

targos commented Oct 6, 2018

Landed in 5982e7a and 47835e2.

@targos targos closed this Oct 6, 2018
@targos
Copy link
Member

targos commented Oct 6, 2018

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

@refack refack deleted the mips-atomics branch October 6, 2018 13:43
@refack refack mentioned this pull request Oct 18, 2018
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@targos targos targos approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@refack @nodejs-github-bot @targos @addaleax