close
Skip to content

Revert "build: extract common code from NODE_EXE/_G_EXE"#22458

Closed
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:revert-build-node-exe
Closed

Revert "build: extract common code from NODE_EXE/_G_EXE"#22458
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:revert-build-node-exe

Conversation

@danbev
Copy link
Copy Markdown
Contributor

@danbev danbev commented Aug 22, 2018

This reverts commit 4e2fa8b.

Refs: #22457

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

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 22, 2018

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

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 22, 2018
@danbev
Copy link
Copy Markdown
Contributor Author

danbev commented Aug 22, 2018

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

@jasnell
Copy link
Copy Markdown
Member

jasnell commented Aug 22, 2018

Please 👍 to approve fast-track

@jasnell jasnell added fast-track PRs that do not need to wait for 72 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 22, 2018
addaleax
addaleax approved these changes Aug 22, 2018
View reviewed changes
Copy link
Copy Markdown
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.

Resume build CI: https://ci.nodejs.org/job/node-test-pull-request/16689/

@refack
Copy link
Copy Markdown
Contributor

refack commented Aug 22, 2018

Landed in 9d9f691

@refack refack closed this Aug 22, 2018
refack pushed a commit that referenced this pull request Aug 22, 2018
@danbev @refack
Revert "build: extract common code from NODE_EXE/_G_EXE"
9d9f691 
This reverts commit 4e2fa8b.

Refs: #22457

PR-URL: #22458
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@refack refack mentioned this pull request Aug 22, 2018
Closed
@danbev
Copy link
Copy Markdown
Contributor Author

danbev commented Aug 23, 2018

Thanks everyone, and sorry for causing the extra work.

@essen
Copy link
Copy Markdown

essen commented Nov 26, 2018

Search engine lead me here about the jobserver unavailable warning. I have no stakes in this project but figured out the issue so if that can help anyone, read on.

The problem comes from the fact that a define is used and that there's two levels of expansion. Make needs the $(MAKE) variable to make it to the target, rather than its value, so instead of:

define build_node_exe
$(MAKE) -C out BUILDTYPE=$1 V=$(V)

It should have been:

define build_node_exe
$$(MAKE) -C out BUILDTYPE=$1 V=$(V)

This can be thought of as escaping the $ character, and makes the $(MAKE) survive the $(call ...). Then Make can properly translate the $(MAKE) variable and everything works correctly.

Hope that helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@richardlau richardlau richardlau approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 72 hours to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.