Skip to content
This repository has been archived by the owner on Jun 18, 2021. It is now read-only.

Fix source directory for install target #42

Closed
wants to merge 1 commit into from
Closed

Fix source directory for install target #42

wants to merge 1 commit into from

Conversation

richardlau
Copy link
Member

Fixes build issues seen on the Node.js CI when trying to build with CITGM: nodejs/citgm#226.

@richardlau
Copy link
Member Author

cc @gdams.

gdams
gdams approved these changes Jan 20, 2017
View reviewed changes
Copy link
Member

@gdams gdams left a comment

Choose a reason for hiding this comment

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

LGTM, confirmed to be working in the citgm CI: https://ci.nodejs.org/view/Node.js-citgm/job/gibfahn-citgm-smoker-more-platforms/84/console

@rnchamberlain
Copy link
Contributor

rnchamberlain commented Jan 20, 2017 •
edited
Loading

This PR builds OK for me on Linux and Windows. Can you explain where PRODUCT_DIR is defined, and why that works for citgm whereas the explicit .../build/Release/... path does not?

@richardlau
Copy link
Member Author

To save having to read all of nodejs/citgm#226, here's the failure on the Node.js CI:

error:                     | make: Entering directory '/tmp/a27a0182-6fb0-4104-9d68-3cd214c96b02/cf253ebd7215116d02b9159f5ba6df40ddee9507/build'                                                                                                                                                                                                                                               
error:                     | CXX(target) Release/obj.target/api/src/module.o                                                                                                                                                                                                                                                                                                                   
error:                     | CXX(target) Release/obj.target/api/src/node_report.o                                                                                                                                                                                                                                                                                                              
error:                     | make: Leaving directory '/tmp/a27a0182-6fb0-4104-9d68-3cd214c96b02/cf253ebd7215116d02b9159f5ba6df40ddee9507/build'                                                                                                                                                                                                                                                
error:                     |                                                                                                                                                                                                                                                                                                                                                                   
error:                     | make: *** No rule to make target '/tmp/a27a0182-6fb0-4104-9d68-3cd214c96b02/cf253ebd7215116d02b9159f5ba6df40ddee9507/build/Release/api.node', needed by '/tmp/a27a0182-6fb0-4104-9d68-3cd214c96b02/cf253ebd7215116d02b9159f5ba6df40ddee9507/api.node'.  Stop.

Failure was inconsistent on the Node.js CI (sometimes passing, other times failing for a given OS/Linux distribution). Was not able to reproduce locally.

@rnchamberlain <(PRODUCT_DIR) is a predefined variable in gyp. Where is <(module_root_dir) (the current variable used) defined? I'm not 100% sure why the problem occurs in the first place (as mentioned in the CITGM PR I haven't been able to reproduce on a system I have access to). Perhaps with the help of @gdams/@gibfahn we could capture the generated Makefiles from gyp from the Node.js CI workspaces and compare?

Alternatively we could remove the install target and not copy api.node and use something like bindings to load api.node.

@richardlau
Copy link
Member Author

Also <(module_root_dir)/build/Release/api.node wouldn't work for a debug build (which would end up in build/Debug/api.node).

@rnchamberlain
Copy link
Contributor

Thanks @richardlau just checking that PRODUCT_DIR was a respectable thing! Works fine for me on Mac + Linux + Win.

module_root_dir is also pre-defined, but it's a node-gyp thing. It does not seem to be documented, was added here:
nodejs/node-v0.x-archive@1520c7b

I did have a look at https://www.npmjs.com/package/bindings a while back but the extra dependency did not seem to bring much extra benefit.

LGTM

rnchamberlain pushed a commit that referenced this pull request Jan 25, 2017
@richardlau @rnchamberlain
Fix source directory for install target
0c5cc92 
PR-URL: #42
Reviewed-By: Richard Chamberlain <richard_chamberlain@uk.ibm.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
@rnchamberlain
Copy link
Contributor

Landed as 0c5cc92

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

@gdams gdams gdams approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@richardlau @rnchamberlain @gdams