Skip to content
This repository has been archived by the owner on Jul 23, 2019. It is now read-only.

add relative prefix to build script command #34

Closed
wants to merge 1 commit into from
Closed

add relative prefix to build script command #34

wants to merge 1 commit into from

Conversation

breezykermo
Copy link

@breezykermo breezykermo commented Mar 10, 2018

Running Node 8.9.3 and Rust 1.24.1.

When running npm install from xray_electron as suggested in contributing.md, the napi.js script triggered during the Cargo compilation of xray_node didn't look for target at the outside layer, where the code is built, but for a target folder inside the xray_node folder. Printing pwd from the child process cp confirms that it is executing inside the xray_node folder.

Adding this relative path fixed the issue for me. I'm not sure why { stdio: 'inherit' } doesn't work in the same way between lines 53 and 54. (Am new to rust and Cargo, is it something there?)

I also added two break statements to the other cases for execution clarity in the switch statement.

Running with Node 8.9.3 and Rust 1.24.1, this command didn't look for `target` at the outside layer, where it was built,  but for a `target` folder inside `xray_node`.
@breezykermo
Copy link
Author

breezykermo commented Mar 10, 2018

Note: this actually doesn't work, as in the case of building the nested napi/test-module, I need another .. to step out appropriately. Should the absolute path of the target directory be specified as a command line argument?

Did I miss some configuration step to appropriately locate the target folder?

@as-cii
Copy link
Contributor

as-cii commented Mar 19, 2018

Hello @breezykermo, sorry we let this pull request sit around for 9 days and thanks for this contribution!

It looks like this issue may have been fixed already on master thanks to commit 5ffc386, so I am going to close it for now. If you still can reproduce the problem, please feel free to open a new issue and describe what you are observing. Thanks!

@as-cii as-cii closed this Mar 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants