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

Update MSVC support for Microsoft Visual Studio Build Tools 2017 #995

Merged
merged 1 commit into from Apr 8, 2017
Merged

Update MSVC support for Microsoft Visual Studio Build Tools 2017 #995

merged 1 commit into from Apr 8, 2017

Conversation

@JGoutin
Copy link
Contributor

@JGoutin JGoutin commented Mar 20, 2017

No description provided.

@AraHaan
Copy link

@AraHaan AraHaan commented Apr 4, 2017

As much as I like this what if the person still has Visual Studio 2015 installed? Will it have some sort of fallback to VS2015 if VS2017 is not installed or no?

@JGoutin
Copy link
Contributor Author

@JGoutin JGoutin commented Apr 5, 2017

If "Visual Studio 2015" is installed, distutils will work as intended, so this patch will not apply. Maybe this need to be changed for give priority to VS2017 ?

If "Visual C++ BuildTools 2015" and VS2017 are installed, the patch will detect Visual C++ compiler (Theoretically 2017, but this need to be checked, I'll test this).

I there is VS2017 is not installed, it will detect "Visual C++ BuildTools 2015".

guess_vc = os.path.join(self.VSInstallDir, default)
# Subdir with VC exact version as name
try:
vc_exact_ver = os.listdir(guess_vc)[-1]

This comment has been minimized.

@jaraco

jaraco Apr 7, 2017
Member

This literal -1 makes me a little uneasy. What's happening here? Is it assuming that listdir is returning the directories sorted alphanumerically and that the latest version will also be the largest alphanumeric? What sorts of values does one expect to find in os.listdir(guess_vc)?

# Default path
default = r'Microsoft Visual Studio %0.1f\VC' % self.vc_ver
guess_vc = os.path.join(self.ProgramFilesx86, default)
self.VSInstallDir

This comment has been minimized.

@jaraco

jaraco Apr 7, 2017
Member

I appreciate your forward thinking in this block to give precedence to the future and indicate the past as legacy. Nice work.

One change I would make here is to implement these two techniques as a single assignment, like:

guess_vc = self._guess_vc() or self._guess_vc_legacy()
bin_dir = 'Bin' if self.vc_ver <= 11.0 else r'Bin\x86'
tools = [os.path.join(self.si.WindowsSdkDir, bin_dir)]
else:
tools = []

This comment has been minimized.

@jaraco

jaraco Apr 7, 2017
Member

This probably deserves a comment - why are there no tools for vc_ver < 15?

This comment has been minimized.

@jaraco

jaraco Apr 7, 2017
Member

Oh, I see now, you initialize it empty so you can populate it later. This is getting complicated.

@@ -1170,14 +1231,16 @@ def _unique_everseen(self, iterable, key=None):
seen_add(k)
yield element

def _get_content_dirname(self, path):
def _get_content_dirname(self, path, slash=True):

This comment has been minimized.

@jaraco

jaraco Apr 7, 2017
Member

One should not use boolean parameters to twiddle inner behavior on functions. Instead of passing slash=False, just use _get_content_dirname().rstrip('\\').

This comment has been minimized.

@jaraco

jaraco Apr 7, 2017
Member

Even better would be to unconditionally return the value without the slash and let the caller add a slash when appropriate.

@jaraco
Copy link
Member

@jaraco jaraco commented Apr 8, 2017

My comments above are largely stylistic, so I'd like to get these changes incorporated soon. I'll pull in the branch and adapt to my tastes and get this out.

@jaraco jaraco merged commit 20b3b3e into pypa:master Apr 8, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jaraco added a commit that referenced this pull request Apr 8, 2017
jaraco added a commit that referenced this pull request Apr 8, 2017
jaraco added a commit that referenced this pull request Apr 8, 2017
jaraco added a commit that referenced this pull request Apr 8, 2017
@JGoutin JGoutin deleted the JGoutin:Feature/MSBuildTools2017 branch Apr 8, 2017
@jaraco
Copy link
Member

@jaraco jaraco commented Apr 8, 2017

I've rolled out these changes in 34.4.0. Please test it out and report any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants