search

project-everest / everest

https://project-everest.github.io/
Apache License 2.0
193 stars 29 forks source link

Make sure Python and scons are all amd64 versions, also fix paths acc… #42

Closed andreasdotorg closed 7 years ago

andreasdotorg commented 7 years ago

…ordingly. Provide download link for VS2015.

msftclas commented 7 years ago

@andreas23, Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request. Thanks, Microsoft Pull Request Bot

msprotz commented 7 years ago

Thanks! Out of curiosity, why is it important that Python27 be amd64?

andreasbogk commented 7 years ago

It's important that both Python and SCons match in their versions. Currently, they don't: Python will be installed as 64 bit, SCons as 32 bit. Also, the directory name of the Python installation is wrong.

Main goal of this patch is to make everything consistent, one way or another. Given it's 2017, I've opted for 64 bit as the default.

msprotz commented 7 years ago

This looks good to me and makes sense. Thanks for the explanation!

I just checked and the python amd64 msi that we point to chooses c:\python27 as a default install directory ("install for all users"). Is there a flag to make it choose c:\python27amd64? Otherwise, I'm afraid it's going to be hard to get everyone to remember to pick a non-default setup path.

andreasbogk commented 7 years ago

Actually, C:\Python27amd64 is the default. Nice catch by BarryBo.

msprotz commented 7 years ago

we must be using different installers... this is what I see:

image

andreasbogk commented 7 years ago

I have the nagging suspicion that this is VS 2017 and its Python support that is at fault here. This is about the only other piece of software I installed on this machine, and just noticed it supports Python too.

The installer then picks up from the registry entry and goes with the different path for me, I guess. Will look into this tomorrow in depth.

I guess the registry is also where the answer to this lies: HKEY_CURRENT_USER\Software\Python\PythonCore\2.7\InstallPath, if I am not mistaken, this should be checked instead of assuming a path.

andreasdotorg commented 7 years ago

Ok, so my diagnosis was right: VS 2017 Python installs into a different path. So the script really should look at the registry, This will also care for the case that the user decides to install into a non-standard location.

There are four cases: the product of per-user/local-machine install with 32/64 bit. Depending on this, we Python installation path is to be found with one of:

regtool get \HKLM\Software\Python\PythonCore\2.7\InstallPath\ regtool get \HKCU\Software\Python\PythonCore\2.7\InstallPath\ regtool get \HKLM\Software\WOW6432Node\Python\PythonCore\2.7\InstallPath\ regtool get \HKCU\Software\WOW6432Node\Python\PythonCore\2.7\InstallPath\

Depending on which Python we find, we need to select the right scons installation package.

Also, we don't even support 32 bit machines, and can conveniently ignore this case, right?

msprotz commented 7 years ago

Yes, we can ignore 32-bit machines for now. Eventually, we want to be able to easily produce 32-bit builds, but none of our tooling supports it and it's currently a super manual process.

We already have some registry querying in our Makefiles (to find the location of msbuild), so it's fine to add a few invocations of reg.exe in the Everest script.

andreasdotorg commented 7 years ago

I've reverted to the actual installation path, i.e., "C:\Python27". I think these changes are good for now. Actually talking to the registry will be a separate pull request.

msprotz commented 7 years ago

Thanks Andreas!