-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: simplify execution of built binary #1955
Conversation
Since we aleady have a variable with path to the newly built binary, use that instead of prefixing path. This also allows us to pass a different path through the environment (NODE=)
@@ -141,7 +141,7 @@ test-npm: $(NODE_EXE) | |||
NODE_EXE=$(NODE_EXE) tools/test-npm.sh |
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.
perhaps this should also go down in to test-npm.sh? / @Fishrock123
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.
Yes, I unrelatedly figured out that this was broken after quickly discussing it at nodeconf. I'll open a PR.
The usage of both NODE and NODE_EXE throughout the makefile is slightly confusing.
|
@orangemocha I'm all for making it more readable. I'd prefer to do it in another PR so we can land this and get a linter jenkins slave up and running. |
Ping @nodejs/build or other collaborators. I'd like to land this so we can get the linter going. |
LGTM |
Here's a run through the new linter project: https://jenkins-iojs.nodesource.com/job/iojs+linter/3/ |
lgtm, :makeitso: |
Since we aleady have a variable with path to the newly built binary, use that instead of prefixing path. This also allows us to pass a different path through the environment (NODE=) PR-URL: #1955 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
Merged in 1ec53c0. Thanks for the review. Proceeding to setting the linter job up! |
Since we aleady have a variable with path to the newly built binary, use that instead of prefixing path. This also allows us to pass a different path through the environment (NODE=) PR-URL: nodejs#1955 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
Since we aleady have a variable with path to the newly built binary, use that instead of prefixing path. This also allows us to pass a different path through the environment (NODE=) PR-URL: #1955 PORT-PR-URL: #2101 PORT-FROM: 1ec53c0 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
Since we aleady have a variable with path to the newly built binary, use that instead of prefixing path. This also allows us to pass a different path through the environment (NODE=).
R=@nodejs/build