build: Don't regenerate node->out/*/node symlink if already there #9827

sxa · 2016-11-28T18:52:28Z

Description of change

Checklist

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

Affected core subsystem(s)

build

Description of change

This PR will stop the "ln -sf out/Release node node" (and it's Debug equivalent) from occurring if it is already in place. This appears to be causing a problem when it is executed in parallel with the node-gyp execution for the build-addons target as it appears to be triggering a race condition (visible in the AIX CI as per the referenced issue although I haven't replicated it on my own system) whereby the deletion of the symlink (A result of the -f) is leaving a small window before it is recreated where the addons build runs, which requires the node symlink to be in place. I can see no obvious reason why this would have ONLY showed up on the v4 release as the same condition can occur on master as far as I can tell.

The disadvantage of the solution in this PR is that if "node" already existed and wasn't previously a symlink to out/Release node then it won't overwrite it. I'm open to alternative suggestions on this - checking the output from "diff node out/Release/node" before replacing would work for example. Tagging @bnoordhuis as he has made some comments in Makefile around thisagainst the build-addons target in the makefile. The problem also goes away if the $(NODE_EXE) target is not listed as ".PHONY" but since that was done for sensible reasons I assume we need to leave that in place, and this is really just a "workaround" for the fact it's not a true dependency.

bnoordhuis

LGTM with a request and a suggestion.

bnoordhuis · 2016-11-28T20:53:35Z

Makefile

@@ -66,11 +66,11 @@ endif

 $(NODE_EXE): config.gypi out/Makefile
 	$(MAKE) -C out BUILDTYPE=Release V=$(V)
-	ln -fs out/Release/$(NODE_EXE) $@
+	if [ ! -r $(NODE_EXE) ]; then ln -fs out/Release/$(NODE_EXE) $@; fi


Wouldn't -L (symbolic link) make more sense?

Can you add a comment here and below explaining why the check is necessary? It's going to look superfluous to the casual reader.

Yep I'll add a comment - that's a fair point.
Regarding -L, to be honest I wasn't certain it was standard sh syntax and also it would stop someone deliberately overriding it with an executable if they wanted to for any reason, although it appears to work on all the systems I've tried (AIX, Solaris, OS X, and of course linux) so I can add that in along with the -r (Since -L in itself will pass if it's a symlink that points to nothing which wouldn't be useful!)

The node -> out/*/node symlink is getting recreated in parallel with other targets in the makefile which require it (e.g. test-ci) and this seems to be causing a race condition which is showing up on AIX Ref: nodejs#9825

sxa · 2016-11-29T12:11:35Z

@bnoordhuis Done - have added both checks as per my comment on your review.
@mhdawson I believe this change will fix the AIX failures in V4 staging if you want to try and pull them in and test once this one's approved

gibfahn

LGTM, it'd be nice to have something that checks that the symlink points to out/Release/node, but there doesn't seem to be a good POSIX way of doing it.

gibfahn · 2016-11-29T21:18:53Z

@sxa555 Could we use the -ef test operator? I understand it's not POSIX, but if it works on dash, ksh, bash and zsh (see SO link, I haven't verified) then it might cover all the platforms we care about.

sxa · 2016-11-30T10:52:45Z

We could do, and while I'm not 100% against it I'm somewhat reluctant to use something that could limit portability (it doesn't work on a pure bourne shell).

mhdawson

LGTM

gibfahn · 2016-11-30T22:45:07Z

@sxa555 Makes sense. In the future if not doing the check does cause issues for someone we can consider using something like -ef, but it's not currently a problem.

mhdawson · 2016-12-05T17:34:36Z

Think this is ready to land. CI run here: https://ci.nodejs.org/job/node-test-pull-request/5206/

mhdawson · 2016-12-05T20:17:32Z

Only failure in CI is on ARM and it looks like the machine had a connection problem as opposed to any specific tests failing. Will land.

mhdawson · 2016-12-05T20:27:27Z

Landed as f3d613e

The node -> out/*/node symlink is getting recreated in parallel with other targets in the makefile which require it (e.g. test-ci) and this seems to be causing a race condition which is showing up on AIX Fixes: #9825 PR-URL: #9827 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-by: Michael Dawson <[email protected]>

addaleax · 2016-12-06T18:54:37Z

This needs to land together with #10153 wherever it lands

Currently when running make node_g the following error is displayed: if [ ! -r node -o ! -L ]; then ln -fs out/Debug/node node_g; fi /bin/sh: line 0: [: argument expected It looks like there was a typo for the NODE_EXE where node became lowercase instead of uppercase. Ref: #9827 PR-URL: #10153 Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>

The node -> out/*/node symlink is getting recreated in parallel with other targets in the makefile which require it (e.g. test-ci) and this seems to be causing a race condition which is showing up on AIX Fixes: #9825 PR-URL: #9827 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-by: Michael Dawson <[email protected]>

Currently when running make node_g the following error is displayed: if [ ! -r node -o ! -L ]; then ln -fs out/Debug/node node_g; fi /bin/sh: line 0: [: argument expected It looks like there was a typo for the NODE_EXE where node became lowercase instead of uppercase. Ref: #9827 PR-URL: #10153 Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>

The node -> out/*/node symlink is getting recreated in parallel with other targets in the makefile which require it (e.g. test-ci) and this seems to be causing a race condition which is showing up on AIX Fixes: nodejs#9825 PR-URL: nodejs#9827 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-by: Michael Dawson <[email protected]>