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: fix Makefile wrt finding node executable #18040

Closed
wants to merge 1 commit into from

Conversation

hashseed
Copy link
Member

@hashseed hashseed commented Jan 8, 2018

Not all shells set PWD, so use CURDIR instead.

`which node` may return the empty string, so guard against
that too. Consider:

if [ -x `which velociraptor` ] && [ -e `which velociprator` ];\
then echo "run"; else echo "keep calm"; fi;
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

Not all shells set PWD, so use CURDIR instead.

`which node` may return the empty string, so guard against
that too. Consider:
if [ -x `which velociraptor` ] && [ -e `which velociprator` ];\
then echo "run"; else echo "keep calm"; fi;
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jan 8, 2018
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI is happy

@joyeecheung
Copy link
Member

@gibfahn
Copy link
Member

gibfahn commented Jan 10, 2018

Not all shells set PWD, so use CURDIR instead.

Which shells don't set $PWD?

@hashseed
Copy link
Member Author

Which shells don't set $PWD?

Apparently most shells do. Some of V8's test bots do not.

In addition, $PWD can cause confusion, if the user runs make from a different directory than the Makefile via make -C. This is often benign because Node.js' Makefile calls make recursively for some targets, by which time $PWD is implicitly set to $CURDIR for that recursive invocation. But in other cases, $PWD diverges from $CURDIR.

@gibfahn
Copy link
Member

gibfahn commented Jan 10, 2018

Apparently most shells do. Some of V8's test bots do not.

Yeah, I was just curious as to which bots (and shells) they were.

In addition, $PWD can cause confusion, if the user runs make from a different directory than the Makefile via make -C.

That makes sense, and I have no issue with this change. Does it enable make -C /path/to/Makefile at the top level?

@hashseed
Copy link
Member Author

That makes sense, and I have no issue with this change. Does it enable make -C /path/to/Makefile at the top level?

Yup. It indeed does.

hashseed added a commit that referenced this pull request Jan 11, 2018
Not all shells set PWD, so use CURDIR instead.

`which node` may return the empty string, so guard against
that too. Consider:
if [ -x `which velociraptor` ] && [ -e `which velociprator` ];\
then echo "run"; else echo "keep calm"; fi;

PR-URL: #18040
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@hashseed
Copy link
Member Author

Landed as a2c7085. Closing.

@hashseed hashseed closed this Jan 11, 2018
@hashseed hashseed deleted the fixmakefile branch January 11, 2018 06:57
evanlucas pushed a commit that referenced this pull request Jan 22, 2018
Not all shells set PWD, so use CURDIR instead.

`which node` may return the empty string, so guard against
that too. Consider:
if [ -x `which velociraptor` ] && [ -e `which velociprator` ];\
then echo "run"; else echo "keep calm"; fi;

PR-URL: #18040
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
Not all shells set PWD, so use CURDIR instead.

`which node` may return the empty string, so guard against
that too. Consider:
if [ -x `which velociraptor` ] && [ -e `which velociprator` ];\
then echo "run"; else echo "keep calm"; fi;

PR-URL: #18040
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
Not all shells set PWD, so use CURDIR instead.

`which node` may return the empty string, so guard against
that too. Consider:
if [ -x `which velociraptor` ] && [ -e `which velociprator` ];\
then echo "run"; else echo "keep calm"; fi;

PR-URL: #18040
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins
Copy link
Contributor

This lands cleanly on v8.x but will require a backport for v6.x

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.

8 participants