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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

danbev
Copy link
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
Collaborator

@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
Contributor Author

danbev commented Aug 22, 2018

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

@jasnell
Copy link
Member

jasnell commented Aug 22, 2018

Please 👍 to approve fast-track

@jasnell jasnell added fast-track PRs that do not need to wait for 48 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
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
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@refack refack mentioned this pull request Aug 22, 2018
Closed
@danbev
Copy link
Contributor Author

danbev commented Aug 23, 2018

Thanks everyone, and sorry for causing the extra work.

@essen
Copy link

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

@BridgeAR BridgeAR BridgeAR approved these changes

@richardlau richardlau richardlau approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell 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 48 hours to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.