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

build: simplify execution of built binary #1955

Closed

Conversation

jbergstroem
Copy link
Member

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

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=)
@brendanashworth brendanashworth added the build Issues and PRs related to build files or the CI. label Jun 12, 2015
@@ -141,7 +141,7 @@ test-npm: $(NODE_EXE)
NODE_EXE=$(NODE_EXE) tools/test-npm.sh
Copy link
Member

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

Copy link
Contributor

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.

@orangemocha
Copy link
Contributor

The usage of both NODE and NODE_EXE throughout the makefile is slightly confusing.
Suggestion:

  1. Rename NODE_EXE to something like NODE_FILENAME
  2. Eliminate the use of $(NODE_EXE) (or $(NODE_FILENAME)) in all places where it's not absolutely necessary, in favor of $(NODE). It seems that all the makefile rules could deal - and probably more robustly - with the fully qualified path.

@jbergstroem
Copy link
Member Author

@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.

@jbergstroem
Copy link
Member Author

Ping @nodejs/build or other collaborators. I'd like to land this so we can get the linter going.

@bnoordhuis
Copy link
Member

LGTM

@jbergstroem
Copy link
Member Author

Here's a run through the new linter project: https://jenkins-iojs.nodesource.com/job/iojs+linter/3/

@rvagg
Copy link
Member

rvagg commented Jun 15, 2015

lgtm, :makeitso:

jbergstroem added a commit that referenced this pull request Jun 15, 2015
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 <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@jbergstroem
Copy link
Member Author

Merged in 1ec53c0. Thanks for the review. Proceeding to setting the linter job up!

@rvagg rvagg mentioned this pull request Jun 16, 2015
rvagg pushed a commit to rvagg/io.js that referenced this pull request Jul 3, 2015
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 <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
jbergstroem added a commit that referenced this pull request Jul 3, 2015
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 <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants