build: extract common code from NODE_EXE/_G_EXE #22310
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
the
build
maclover7
approved these changes
Aug 14, 2018
JungMinu
approved these changes
Aug 14, 2018
|
/cc @nodejs/build-files |
refack
approved these changes
Aug 14, 2018
joyeecheung
approved these changes
Aug 14, 2018
richardlau
approved these changes
Aug 14, 2018
This commit extracts common parts of the NODE_EXE, and NODE_G_EXE recipes into a canned reciepe to reduce some code duplication.
danbev
force-pushed
the
build_node_exe_target
branch
from
e8e965d to
db723a1
Compare
|
Landed in 4e2fa8b. |
danbev
added a commit
that referenced
this pull request
Aug 22, 2018
This commit extracts common parts of the NODE_EXE, and NODE_G_EXE recipes into a canned reciepe to reduce some code duplication. PR-URL: #22310 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Richard Lau <[email protected]>
refack
reviewed
Aug 22, 2018
| # The -r/-L check stops it recreating the link if it is already in place, | ||
| # otherwise $(NODE_EXE) being a .PHONY target means it is always re-run. | ||
| # Without the check there is a race condition between the link being deleted | ||
| # and recreated which can break the addons build when running test-ci | ||
| # See comments on the build-addons target for some more info | ||
| if [ ! -r $@ -o ! -L $@ ]; then ln -fs out/$1/$(NODE_EXE) $@; fi | ||
| endef | ||
| $(NODE_EXE): config.gypi out/Makefile |
There was a problem hiding this comment.
AFAICT $BUILDTYPE is already defined.
Lines 64 to 68 in db723a1
so this could be:
$(NODE_EXE) $(NODE_G_EXE): config.gypi out/Makefile
$(MAKE) -C out
if [ ! -r $@ -o ! -L $@ ]; then ln -fs out/$(BUILDTYPE)/$(NODE_EXE) $@; fi
removing
BUILDTYPE is defined so no need to pass is explicitly
V=$(V) this is just 🤷♂️
and putting $(BUILDTYPE) in the link target
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit extracts common parts of the NODE_EXE, and NODE_G_EXE
recipes into a canned reciepe to reduce some code duplication.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes