-
Notifications
You must be signed in to change notification settings - Fork 75
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
Add support for the build_from_source npm config #99
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add tests to rc-test.js
?
I have updated the tests. Seems like this line is no longer required for my build, removing this line also passes the existing tests. Might be a breaking change for some peaople, leaving it as is for now. Edit: I am ending my day, feel free to edit or add new commits. 👋 |
Yeah, I'm also a little fuzzy on the precedence here, I wanna take a closer look myself. |
How should it work when you have |
The settings from |
Generally they don't. And for var source_build = gyp.opts['build-from-source'] || gyp.opts.build_from_source So to stay in line with Lines 12 to 26 in ba21172
And move this logic accordingly: Lines 32 to 33 in ba21172
But we can move those tasks to a follow-up PR, because it's about a separate feature. Before that, perhaps we can change (some of) the rc tests to functional tests - i.e. running |
Co-Authored-By: adipascu <1521321+adipascu@users.noreply.github.com>
@vweevers how can I help you merge this? |
I need some more eyes on this. |
5.3.0 |
Thanks guys, nice working with you. |
Implements #98