Check for Python27 on path when building on Win32

[jrsconfitto]

the (not a big deal) problem

Previous to 0b5b741 i was building atom on Win32 fine. The other day i fetched new commits from atom and my builds wouldn't work anymore 😢

H:\src\atom [master]> .\script\build
You must have Python 2.7 installed at 'C:\Python27\'

i thought i broke something. i commented out that check and everything built fine because i do have python on my machine and its installed location on my path.

EDIT: See this much more better restatement of the problem

See this comment for a better statement of the problem.

i propose

This PR adds a check for Python27 on the path on win32 machines and if it isn't, script/build says:

You must have Python 2.7 in your path

My initial solution is just to tell win32 builders that Python should be on the path. It may not be the best way, but it seems like a step forward to me. That seems like a reasonable thing to ask someone trying to build this 😄. i wish i knew about a which python-like solution for node.

history and previous discussions

i'm conscious of this conversation and this one, both mentioning that Python isn't installed on the path by default, so i'm aware that this isn't a perfect solution or may not be what you're looking for (the idea has already been floated for what i've done).

If this isn't the way to go about it, maybe we can 🔨 that out here?

A brief aside

This isn't a big deal for me, and i could be wrong, so i'll quote Mr. Paul Betts's comment out of context:

If you don't have a C: drive, you're smart enough to figure out what to patch, 👎 to considering this

This isn't hard for me to patch on my own. i'm 🆒 with just doing that.

For much personal improvement's sake: Next time i'm just going to install python in the default directory!

[jrsconfitto]

My comment:

i wish i knew about a which python-like solution for node.

bugged me, so i found that something like this exists. It's node-which. i tested it on my machine to see if python was present and it worked well.

Should i include it and go down the road of running which.sync('python') to test for python on win32 machines instead of doing a path check?

[gustavohenke]

I would personally do the following:

• check if python is in the %PATH%;
• check if %PYTHON% is defined and pointing to python.exe (I've seen it commented somewhere in this repo)
• finally, check if the default drive (which may be C: or even H:) contains a Python27 directory (I'm sure that there's a special env var for the default drive...)

[avdg]

This was discussed in #2211 too. Also I wouldn't check if the path contains python27, I prefer to actually run the executable with python --version as a user can give it any name it wants...

[probablycorey]

The python requirement comes from node-gyp. It has some code that might be helpful in this discussion https://github.com/TooTallNate/node-gyp/blob/56dcb889091a6a0594854ccc517e498b21078d5f/lib/configure.js#L25.

[jrsconfitto]

Thanks for the link, @probablycorey. There's a lot of code there that looks helpful. i'll look into utilizing what i can from that here.

[probablycorey]

Would something like this be good enough for now?

// Make sure python2.7 is installedif(process.platform=='win32'){varsystemDrive=process.env.SystemDrive||'C:\\'varpythonPath=path.join(systemDrive,'Python27')if(!fs.existsSync(pythonPath)){console.warn("You must have Python 2.7 installed at '"+pythonPath+"'");process.exit(1);}}

It will still fail if the python in your path has a version >= 3. But that could be fixed by building setting the PYTHON env variable.

[avdg]

@probablycorey Nice for pointing out the actual python check in node-gyp. That's the code we should have been used from the actual beginning.

And

// Make sure python2.7 is installedif(process.platform=='win32'){varsystemDrive=process.env.SystemDrive||'C:\\'varpythonPath=path.join(systemDrive,'Python27')if(!fs.existsSync(pythonPath)){console.warn("You must have Python 2.7 installed at '"+pythonPath+"'");process.exit(1);}}

Is probably a good start for now (meh... its just another variation of whats been posted here), but the code is already written here... https://github.com/TooTallNate/node-gyp/blob/56dcb889091a6a0594854ccc517e498b21078d5f/lib/configure.js#L34

@jugglingnutcase Having python not installed at its default location, but still having it in the system path sounds to me like a good testcase.

Since node-gyp actually does support executing from path locations, the question is if the script should support that too.

---

Ok, just to be clear I want to show what happens if python was not installed (I just gave the python directory a temporary different name).

This happened: https://imgur.com/uBXeEUu
And this isn't a clear error btw so we better avoid this.
Note that python isn't even mentioned at all... It also makes me wonder what happens with other errors but I guess that's outside the scope of this issue.

[probablycorey]

@avdg shouldn't the code sample you provided protect against the error you posted? I'm assuming you renamed C:\\Python27 to something like C:\\TEMP_Python27

[avdg]

Hmm, let me start with this... this pretty simple problem is just complicated because of the many cases it has.

What I forgot was that python was still in my include path. (and yeah, you can test it with c:\\TEMP_Python, I took C:\\Python26 because I felt like evil).

@probablycorey Whatever... I guess your code is the best anyway (yes it should prevent the error) as it is exactly the same as the code used on node-gyp (edit: when python is not found in $path). (and yes, it does still check for the python27 in the path, so you still can't use something like c:\\python\\python.exe but I guess we should with what we have right now).

(I'm just dreaming of re-using node-gyp's checker code, but that dream is I guess technically too hard to realize.)

[avdg]

I guess I should make the pull and just copy their code.

Code is up.

[jrsconfitto]

Sorry for not responding earlier. I was on vacation this past weekend and am just getting back from work now. I'll be able to continue working on this PR over the next few days.

I'm taking a look at all this now and will add some more comments throughout the evening!

[probablycorey]

One thing to keep in mind is that script/bootstrap won't have access to any node modules via npm. The only thing we can rely on are the builtin node packages, so we can't use modules like which.

[avdg]

This prototype has no path checker as that code depends on https://www.npmjs.org/package/which. I believe the bootstrap code should be run without any npm dependencies.

The best I can do is to add a copy of that code too in script/bootstrap, which makes me go yuck. Any alternatives?

Pls don't use this code in the current state as I'm sure it won't work on osx and linux. Checks only windows

• Should check python at the default install location
• Should check version
• Should check python in path

Pls review: avdg@3d1dcd1

[jrsconfitto]

Would something like this be good enough for now?

// Make sure python2.7 is installedif(process.platform=='win32'){varsystemDrive=process.env.SystemDrive||'C:\\'varpythonPath=path.join(systemDrive,'Python27')if(!fs.existsSync(pythonPath)){console.warn("You must have Python 2.7 installed at '"+pythonPath+"'");process.exit(1);}}

If I understand that code properly, @probablycorey, i'd prefer that we don't (just) use this because then I'll still have to patch script\bootstrap to run stock atom.

I originally created this PR because the current check blows up unfairly (imo)! I have python and my path set up properly to make sure node-gyp is happy, but atom's build script tells me I don't.

my separate experience with node-gyp and python

Today, on a different machine, I ran npm install level, which uses node-gyp and received an error saying that I didn't have python on the machine correctly. I took the opportunity to install python in a weird location and set it in my path. It worked fine! Node-gyp doesn't require that python be in C:\python27, why should atom?

towards a resolution?

this pretty simple problem is just complicated because of the many cases it has.

Yeah, I think that because it is a conceptually simple problem with a (fairly) simple solution (install and reference python in a way that node-gyp likes!) we should go the route of removing this Python check completely and adding a section to the troubleshooting document explaining what a missing python looks like. It is a prerequisite after all. (Though I've spent a fair amount of time not following directions!)

Edit: forget the following!

If we want to do more, because of what @avdg found when running without python (thanks for that), I don't mind adding a fuller check along the lines of what is happening in node-gyp (thanks for the reference @probablycorey).

(I'm just dreaming of re-using node-gyp's checker code, but that dream is I guess technically too hard to realize.)

I don't think that's too hard to realize! I want to give it a shot (if we don't just go the troubleshooting route).

[jrsconfitto]

@probablycorey what about vendoring the node-which code? It looks like which's code can work using only the stock fs module.

[jrsconfitto]

💩 did not mean to do that. Sorry 😦

[avdg]

Vendoring sounds like a great idea, even though I'm not familiar with that word. Looking into it as just copy-pasting the code has lead to a few problems where I'm not sure what happened...

[avdg]

See #2426

What you should expect:

• Should check python at the default install location
• Should check version
• Should check python in path

Any comments?

[probablycorey]

I think we are getting close, but we need to restate the problem.

GOAL Give people more information about why script\build failed and how to fix it.

Existing Problem

1. node-gyp requires python 2.7
2. If you don't have python 2.7 installed node-gyp fails with confusing error messages.
3. We want people to know script\bootstrap has failed because of the lack of python 2.7 so we should have some basic python requirement checks.
4. script\bootstrap should not fail if node-gyp would not fail (for example, if python is installed in H:\\Python27

I'd like to avoid relying on the documentation as the only way to diagnose this problem because many people don't read the documentation. So getting something in script\bootstrap is my preferred solution.

[probablycorey]

Also, a question for @jugglingnutcase. Did my example code in #2351 (comment) fail because python is not located at the default directory?

[avdg]

Hmm...

1. Actually node-gyp recommends python 2.7, but does not support any python 3.x binaries (this might be fixed by tuning the regex a bit).
2. I like to point out that I ended up in inserting more error messages in node-gyp. I'm sure all node-gyp users would benefit if these error messages are getting upstreamed.
3. Hmm any comments about that in my patch?
4. Requires testing and see number 1. There's a job to be done (but I guess its production ready) :-)

Though users are likely not to read documentation I guess it would be nice to still have some documentation.

I'll fix the issue number 1 and (done) I'm hoping the code is ready for pushing.

[jrsconfitto]

@probablycorey, i'm on board with all those points. Thanks for restating the goal, i found that incredibly helpful and refreshing.

Did my example code in #2351 (comment) fail because python is not located at the default directory?

IIRC, yeah, it failed for that exact reason. My python was installed at C:\Program Files\Python.

i'll leave my current doc edits for now and try to include @avdg's work into this PR.

[jrsconfitto]

@avdg, i'm wavering between pulling your work into this PR, or just recommending we go with your PR. i'll take a look into your work first, but i'll have to wait until i get some free time later. If your stuff looks good to me, maybe i'll just recommend we close this and go with your PR.