build: allow running configure from any directory#17321
Uh oh!
There was an error while loading. Please reload this page.
Conversation
gibfahn added build 
nodejs-github-bot added the build 
jasnell commented Nov 26, 2017
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 left a comment
bnoordhuis left a comment There was a problem hiding this comment.
@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 commented Nov 26, 2017
heh... oh bah, that's right ;-) ... |
refack commented Nov 27, 2017
@jasnell might have been thinking about batch files, or shell scripts. |
addaleax added the author ready 
configure Outdated
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Good point.
I tried it with spaces+UTF-8, and it works on macOS, could you maybe try something similar for windows?
~/wrk/诺 德/node
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I've tried messing around with this, but didn't find the right incantation.
There was a problem hiding this comment.
So that works with the relative path but not the absolute one? If so I guess we should go back to relative paths.
There was a problem hiding this comment.
Well, this works:
root_dir=os.path.dirname(__file__) root_dirandos.chdir(root_dir)There was a problem hiding this comment.
That doesn't work for me unless you then unset root_dir. Pushed a commit with that change.
▶▶▶ node/configure~/wrk/comTraceback (mostrecentcalllast): File"node/configure", line49, in<module>fromgyp.commonimportGetFlavorImportError: Nomodulenamedgyp.commonjasnell commented Nov 27, 2017
I was :) |
refack removed the author ready 
gibfahn commented Nov 28, 2017
Okay, so AFAICT we don't need |
refack commented Nov 28, 2017
That's a great proof that if we figure out path idiosyncrasies in |
configure Outdated
There was a problem hiding this comment.
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.
addaleax added the author ready 
gibfahn commented Dec 10, 2017
PR-URL: nodejs#17321 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17321 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17321 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax removed the author ready PR-URL: #17321 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17321 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17321 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17321 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
10 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build