build: allow running configure from any directory #17321

gibfahn · 2017-11-26T13:33:46Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

build

jasnell · 2017-11-26T17:00:06Z

One thing: this should either chdir back to the original directory or should print a warning to the console that the directory was changed, just as a friendly heads up to the user.

bnoordhuis

@jasnell Not necessary, the pwd is a per-process property.

(That's coincidentally why cd is always a shell built-in, you can't implement it as a separate binary.)

jasnell · 2017-11-26T17:40:38Z

heh... oh bah, that's right ;-) ...

refack · 2017-11-27T01:30:43Z

@jasnell might have been thinking about batch files, or shell scripts.

refack · 2017-11-27T01:10:40Z

configure

@@ -35,12 +35,15 @@ import subprocess
 import shutil
 import string

+# If not run from node/, cd to that directory.
+root_dir = os.path.dirname(os.path.abspath(__file__))


You're shifting from rel-path to abs for root_dir, it might have impact WRT spaces or unicode in the base path, and it's hard to test...

Good point.

I tried it with spaces+UTF-8, and it works on macOS, could you maybe try something similar for windows?

~/wrk/诺德/node

D:\code\node-u诺德github-desktop$ vcbuild x64 Looking for Visual Studio 2017 Found MSVS version 15.0 "C:\bin\dev\python27\python.exe" configure --dest-cpu=x64 --tag= Traceback (most recent call last): File "configure", line 40, in <module> os.chdir(root_dir) WindowsError: [Error 123] The filename, directory name, or volume label syntax is incorrect: 'D:\\code\\node-u? ?github-desktop' Failed to create vc project files.

With or without PYTHONIOENCODING=UTF-8

I've tried messing around with this, but didn't find the right incantation.

So that works with the relative path but not the absolute one? If so I guess we should go back to relative paths.

Well, this works:

root_dir = os.path.dirname(__file__) root_dir and os.chdir(root_dir)

That doesn't work for me unless you then unset root_dir. Pushed a commit with that change.

▶▶▶ node/configure ~/wrk/com Traceback (most recent call last): File "node/configure", line 49, in <module> from gyp.common import GetFlavor ImportError: No module named gyp.common

jasnell · 2017-11-27T01:34:54Z

might have been thinking about batch files, or shell scripts.

I was :)

gibfahn · 2017-11-28T16:57:39Z

Okay, so AFAICT we don't need root_dir any more, so I've removed it. If you've already approved this PR please take another look, it's quite different now.

refack

nice

refack · 2017-11-28T17:08:26Z

we don't need root_dir any more, so I've removed it

That's a great proof that if we figure out path idiosyncrasies in configure we can keep them from affecting the downstream process.

bnoordhuis · 2017-11-29T10:51:05Z

configure

@@ -35,21 +35,24 @@ import subprocess
 import shutil
 import string

+# If not run from node/, cd to that directory.
+if os.path.dirname(__file__):
+  os.chdir(os.path.dirname(__file__))


You could write this as:

os.chdir(os.path.dirname(__file__) or '.')

Which might make it more obvious that os.path.dirname('configure') == ''.

Or maybe that's just me.

I like it, added.

gibfahn · 2017-12-10T10:46:43Z

CI: https://ci.nodejs.org/job/node-test-commit/14727/

PR-URL: nodejs#17321 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #17321 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable Changes: * build: - configure can now be run from any directory (Gibson Fahnestock) #17321 PR-URL: #17776

PR-URL: #17774 Notable Changes: * deps: * upgrade npm to 5.6.0 (Kat Marchán) [#17535](#17535) * build: * configure can now be run from any directory (Gibson Fahnestock) [#17321](#17321)

gibfahn added build Issues and PRs related to build files or the CI. python PRs and issues that require attention from people who are familiar with Python. labels Nov 26, 2017

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 26, 2017

richardlau approved these changes Nov 26, 2017

View reviewed changes

maclover7 approved these changes Nov 26, 2017

View reviewed changes

bnoordhuis approved these changes Nov 26, 2017

View reviewed changes

jasnell approved these changes Nov 26, 2017

View reviewed changes

cjihrig approved these changes Nov 26, 2017

View reviewed changes

addaleax approved these changes Nov 27, 2017

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 27, 2017

refack reviewed Nov 27, 2017

View reviewed changes

refack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017

refack approved these changes Nov 28, 2017

View reviewed changes

bnoordhuis approved these changes Nov 29, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 10, 2017

BridgeAR approved these changes Dec 10, 2017

View reviewed changes

gibfahn force-pushed the configure-cd branch from b12d169 to b7ff3c0 Compare December 10, 2017 15:51

gibfahn merged commit b7ff3c0 into nodejs:master Dec 10, 2017

gibfahn deleted the configure-cd branch December 10, 2017 18:34

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017

gibfahn added land-on-v6.x labels Dec 20, 2017

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17772

Closed

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

MylesBorins mentioned this pull request Dec 20, 2017

v6.12.3 proposal #17776

Merged

MylesBorins added a commit that referenced this pull request Jan 2, 2018

2018-01-02, Version 6.12.3 'Boron' (LTS)

d5fbd25

Notable Changes: * build: - configure can now be run from any directory (Gibson Fahnestock) #17321 PR-URL: #17776

MylesBorins added a commit that referenced this pull request Jan 2, 2018

2018-01-02, Version 6.12.3 'Boron' (LTS)

02e4c8d

Notable Changes: * build: - configure can now be run from any directory (Gibson Fahnestock) #17321 PR-URL: #17776

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: allow running configure from any directory #17321

build: allow running configure from any directory #17321

gibfahn commented Nov 26, 2017

jasnell commented Nov 26, 2017

bnoordhuis left a comment

jasnell commented Nov 26, 2017

refack commented Nov 27, 2017

refack Nov 27, 2017

gibfahn Nov 27, 2017

refack Nov 27, 2017

refack Nov 28, 2017

gibfahn Nov 28, 2017

refack Nov 28, 2017

gibfahn Nov 28, 2017 •

edited

Loading

jasnell commented Nov 27, 2017

gibfahn commented Nov 28, 2017

refack left a comment

refack commented Nov 28, 2017

bnoordhuis Nov 29, 2017

gibfahn Nov 29, 2017

gibfahn commented Dec 10, 2017

build: allow running configure from any directory #17321

build: allow running configure from any directory #17321

Conversation

gibfahn commented Nov 26, 2017

Checklist

Affected core subsystem(s)

jasnell commented Nov 26, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

jasnell commented Nov 26, 2017

refack commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

jasnell commented Nov 27, 2017

gibfahn commented Nov 28, 2017

refack left a comment

Choose a reason for hiding this comment

refack commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn commented Dec 10, 2017

gibfahn Nov 28, 2017 •

edited

Loading