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.

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

build: fix building addons in debug mode #18301

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

After cleaning addons, running make build-addons failed
when Node was configured for debug mode,
because the Makefile was expecting build/Release/ addon paths,
not build/Debug/ addon paths.

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

build

After cleaning addons, running `make build-addons` failed
when Node was configured for debug mode,
because the Makefile was expecting `build/Release/` addon paths,
not `build/Debug/` addon paths.
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jan 22, 2018
$(1)build/Debug/.buildstamp: $(1)build/Makefile $(2) $(ADDON_PREREQS)
$(NODE_GYP) --directory=$(1) --debug build
@touch $$@
$(1)build/Debug/binding.node: $(1)build/Debug/.buildstamp
Copy link
Member

Choose a reason for hiding this comment

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

Could we reduce duplication by using $(BUILDTYPE)?

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't make much difference because you still need the rule that passes --debug to node-gyp.

You could unify them if the call to node-gyp build was replaced with make -C $(1)/build but that's somewhat unofficial.

Copy link
Member

Choose a reason for hiding this comment

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

$(NODE_GYP) --directory=$(1) --$(BUILDTYPE) build?

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't work; the argument is case-sensitive. I suppose you could use $(subst $(subst ...), ...) but that's arguably harder to read than ^.

@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

This needs a rebase. Otherwise it seems like it could land @addaleax

@bnoordhuis
Copy link
Member

The original commits have been reverted. I'll reland them after the next libuv upgrade and include this PR.

@BridgeAR BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Feb 1, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Mar 2, 2018

@bnoordhuis #18918 landed with the new libuv version.

@BridgeAR
Copy link
Member

Ping @bnoordhuis

@BridgeAR
Copy link
Member

Ping @bnoordhuis again

@BridgeAR
Copy link
Member

Since this is based on code that does not exist anymore due to the revert and there is no timeframe for it to land again, I am going to close this. Please reopen if someone disagrees.

@BridgeAR BridgeAR closed this Apr 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants