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

WIP - test: Fix the path to llnode plugin #111

Closed
wants to merge 2 commits into from

Conversation

hhellyer
Copy link
Contributor

@hhellyer hhellyer commented Jul 3, 2017

Load the llnode plugin from the location npm install creates it at.

Load the llnode plugin from the location npm install creates it at.
@hhellyer
Copy link
Contributor Author

hhellyer commented Jul 3, 2017

This fix causes npm test to load the plugin library from the location npm install creates it at. This is probably correct but it is a different location to the location ./gyp_llnode && make -C out/ will use so now the tests fail after building that way. (Previously they worked that way but failed if npm install was used to build.)

This gets the test code running on travis but the tests are still failing:
https://travis-ci.org/nodejs/llnode/jobs/249579889
(I'll see if I can figure out why quickly and maybe add another commit to this.)

@cjihrig
Copy link
Contributor

cjihrig commented Jul 3, 2017

This is probably correct but it is a different location to the location ./gyp_llnode && make -C out/ will use so now the tests fail after building that way.

Would it make sense to try to detect the location in common.js, or maybe even use an environment variable to define the location.

@hhellyer
Copy link
Contributor Author

hhellyer commented Jul 3, 2017

@cjihrig - It might do, there's not that many places to look. I guess the problem is which to pick if both are found. (That could just result in a test failure that reminds you to delete one of them though!)

@cjihrig
Copy link
Contributor

cjihrig commented Jul 3, 2017

We could emit a warning or complain loudly in some other way if they are both found.

cjihrig

This comment was marked as off-topic.

@hhellyer hhellyer changed the title test: Fix the path to llnode plugin WIP - test: Fix the path to llnode plugin Jul 5, 2017
@hhellyer
Copy link
Contributor Author

hhellyer commented Jul 5, 2017

Slightly awkwardly the travis build runs npm install which builds the plugin then runs the _travis make target which runs gyp_llnode and compiles the plugin. This change blocks all the tests running on travis:
https://travis-ci.org/nodejs/llnode/jobs/249994477

I'll figure out what to do next, I have to push the changes to a branch on a PR so that travis picks them up though so I've added a WIP tag to this PR!

@joyeecheung
Copy link
Member

Travis is now working with the plugin built with npm install. Should this be closed, or updated to fallback to the plugin in out/Release when there isn't one in the project directory, or something else?

@bnoordhuis
Copy link
Member

Ping @hhellyer. Can you update or close?

@hhellyer
Copy link
Contributor Author

I'll close, it's been a bit too long since I looked at this and I'm not in a position to update it at the moment. Sorry for leaving it lying around!

@hhellyer hhellyer closed this Jan 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants