bpo-30687: Fixes build scripts to find msbuild.exe and stop relying on vcvarsall.bat #2252

zooba · 2017-06-16T21:56:45Z

Fixes build scripts to find msbuild.exe and stop relying on vcvarsall.bat
Also fixes bdist_wininst.vcxproj to use correct version in generated name.

….bat Also fixes bdist_wininst.vcxproj to use correct version in generated name.

mention-bot · 2017-06-16T21:56:47Z

@zooba, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zware and @briancurtin to be potential reviewers.

zooba · 2017-06-16T22:02:06Z

Decided this isn't really trivial, so I'll create an issue. Not a lot to discuss right now I don't think, but there could be when this all goes wrong :)

zooba · 2017-06-16T22:18:02Z

@tjguk This should fix up the trouble you were having earlier 👍

zware

I like the idea. Haven't tested it yet, and have a few questions, below.

zware · 2017-06-16T22:23:40Z

PCbuild/fix_lineendings.py

@@ -0,0 +1,27 @@
+#! /usr/bin/env python3
+#
+# Fixes line endings of batch files to ensure they remain LF.


This isn't right, they should be CRLF :). Also, a del .git\index && git reset from a clean (no changes, no untracked or ignored files) checkout should give you proper line endings according to the rules in .gitattributes.

Well, they weren't CRLF, because Mercurial (I'm using hggit on this machine) fixed them all and they all got changed.

I can fix them back, but then the review is going to be unreadable, so check it all out before I do it :)

Hmm. I just checked it out and got the right line endings even on your new find_msbuild.bat file. I think this file can just be removed and we hope .gitattributes makes Git do the right thing :)

I needed the script to fix up the problems when hggit did the wrong thing. Maybe I just need to switch to git completely (though then I'm never going to backport anything, because it takes 10x as long)

Would Tools/scripts/crlf.py be enough for you?

zware · 2017-06-16T22:24:58Z

PCbuild/find_msbuild.bat

+@rem
+
+@rem No arguments provided means do full search
+@if '%1' EQU '' goto :begin_search


Does this work, or should these be double quotes? I've been bitten by trying to use single quotes in batch files a couple of times, but I'm not convinced that I actually understand the quoting rules.

The quoting rules are horrible here, because I want to handle where users have done:

set MSBUILD=C:\path to\MSBuild.exe set MSBUILD="C:\path to\MSBuild.exe"

Which breaks in a whole lot of ridiculous ways. Using single quotes here at least handles the two sane cases without causing syntax errors.

zware · 2017-06-16T22:29:04Z

PCbuild/find_msbuild.bat

+@if exist "%MSBUILD%" set MSBUILD="%MSBUILD%" & (set _SOURCE=PATH) & goto :found
+
+@rem VS 2017 sets exactly one install as the "main" install, so we may find MSBuild in there.
+@for /F "tokens=1,2*" %%i in ('reg query "HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\VisualStudio\SxS\VS7" /v 15.0 /reg:32') DO @(


Yep. One of those names that stuck for back-compat reasons...

zware · 2017-06-16T22:31:19Z

PCbuild/build.bat

@@ -115,7 +115,8 @@ if exist "%GIT%" set GITProperty=/p:GIT="%GIT%"
 if not exist "%GIT%" echo Cannot find Git on PATH & set GITProperty=

 rem Setup the environment
-call "%dir%env.bat" %vs_platf% >nul


Can vs_platf go away entirely now?

Yep, sure can!

But then I think the cross-compilation for PGO warning has to go away too... not sure of the full impact there yet, let me try it.

It shouldn't; the vs_platf stuff just gets removed from around it.

Looks like there are some missing environment pieces, but I can add those into python.bat on build. Looks like that's how we launch for PGO build anyway.

Ah, I see what you mean about keeping the warning in there. I'll bring that back

Skips UCRT check for PGO builds.

zooba · 2017-06-17T18:40:04Z

@ned-deily Can we consider this for 3.6.2? It has no impact on the resulting build, but will make the sdist (or forks) easier to use on Windows.

ned-deily · 2017-06-17T19:11:57Z

I trust your judgement on the impact and importance. As you know, 3.6.2rc1 is already being "manufactured". Since this is more than a trivial change and presumably could have end-user impact, I don't think it would be appropriate to try to drop it into a 3.6.2final without exposure. There are a couple of options we can discuss off-line but, in any case, the change needs to first be merged into the normal 3.6 branch.

zooba · 2017-06-17T20:25:16Z

I'm just waiting on @zware to approve (and merge, if there are no changes needed). The backport to the 3.6 branch should be trivial, and probably 3.5 as well.

tjguk · 2017-06-17T20:28:01Z

I'm looking at this but it's likely I won't have a chance to test tonight (9.30pm my time now). Obviously if @zware can test it out, then great!

tjguk · 2017-06-17T20:40:59Z

Ok; I have tested and confirm the build now works for VS2017. I haven't reviewed the code changes as such, and I presume that @zware is a more appropriate reviewer in any case.

zware

One change needed, one suggestion, one request, and one question.

zware · 2017-06-18T01:39:52Z

PCbuild/build.bat

+    if "%PROCESSOR_ARCHITEW6432%" NEQ "AMD64" if "%PROCESSOR_ARCHITECTURE%" NEQ "AMD64" (
+        echo.ERROR: Cannot cross-compile with PGO 
+        echo.    32bit operating system detected, if this is incorrect, 
+        echo.    make sure the ProgramFiles(x86^) environment variable is set 


Error message needs to be reworded, since we're no longer checking %ProgramFiles(x86)% (and %on_64_bit% should be removed from above :CheckOpts)

zware · 2017-06-18T02:05:33Z

PCbuild/fix_lineendings.py

@@ -0,0 +1,27 @@
+#! /usr/bin/env python3
+#
+# Fixes line endings of batch files to ensure they remain LF.


Would Tools/scripts/crlf.py be enough for you?

zware · 2017-06-18T02:42:12Z

PCbuild/find_msbuild.bat

+@if exist "%MSBUILD%" set MSBUILD="%MSBUILD%" & (set _SOURCE=PATH) & goto :found
+
+@rem VS 2017 sets exactly one install as the "main" install, so we may find MSBuild in there.
+@for /F "tokens=1,2*" %%i in ('reg query "HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\VisualStudio\SxS\VS7" /v 15.0 /reg:32') DO @(


I think it's here that I get ERROR: The system was unable to find the specified registry key or value. on a machine without VS 2017. Can that be suppressed without too much hardship? Probably the same below, but that key was actually found on my machine.

zware · 2017-06-18T02:45:53Z

PCbuild/find_msbuild.bat

+
+:found
+@echo Using %MSBUILD% (found in the %_SOURCE%)
+@set _SOURCE=


Since the idea is to mutate the enclosing environment, _SOURCE should probably be something even less likely to conflict, like _Py_MSBuild_Source.

zooba · 2017-06-19T17:20:01Z

Made the requested changes, now just working on dealing with line endings...

zooba · 2017-06-19T17:25:14Z

I think the line endings problem is that .hgeol converts on update/checkout but not commit, while git converts everything to LF on commits and back on checkout. So the actual files in the repo are LF and become CRLF on checkout, but hggit puts them back into git as CRLF instead of LF and we get stupid diffs.

Once we get a backport bot I'll have less need to use something sensible (i.e. hg) for backporting and can use git for more development. Until then, guess I'll just keep doing these extra commits - luckily they'll squash out.

zware · 2017-06-19T17:26:13Z

What kind of behavior do you get if you just remove .hgeol?

zooba · 2017-06-19T17:27:28Z

remove .hgeol

Then all my .bat files have LF endings. I'm on Win10 everywhere, so that's less of an issue for me - I think it's only Win7 and earlier that don't like them.

tjguk · 2017-06-19T17:27:35Z

On 19/06/2017 18:25, Steve Dower wrote: Once we get a backport bot I'll have less need to use something sensible (i.e. hg) for backporting and can use git for more development. Until then, guess I'll just keep doing these extra commits - luckily they'll squash out.

Makes it harder to review within GH though -- have to manually scan the every-line-has-changed looking for actual changes!

zooba · 2017-06-19T17:29:00Z

Makes it harder to review within GH though

Yep, that's why I have about 8 "Fix line endings" commits, to get around that problem :)

tjguk · 2017-06-19T17:29:50Z

:)

…n vcvarsall.bat (python#2252) * Fixes build scripts to find msbuild.exe and stop relying on vcvarsall.bat Also fixes bdist_wininst.vcxproj to use correct version in generated name.

…n vcvarsall.bat (#2252) (#2280) * Fixes build scripts to find msbuild.exe and stop relying on vcvarsall.bat Also fixes bdist_wininst.vcxproj to use correct version in generated name.

…n vcvarsall.bat (#2252) (#2281) * Fixes build scripts to find msbuild.exe and stop relying on vcvarsall.bat Also fixes bdist_wininst.vcxproj to use correct version in generated name.

…n vcvarsall.bat (python#2252) (python#2280) * Fixes build scripts to find msbuild.exe and stop relying on vcvarsall.bat Also fixes bdist_wininst.vcxproj to use correct version in generated name. (cherry picked from commit 06d6e3d)

…n vcvarsall.bat (python#2252) * Fixes build scripts to find msbuild.exe and stop relying on vcvarsall.bat Also fixes bdist_wininst.vcxproj to use correct version in generated name. (cherry-picked from parts of 40a23e8)

zooba added 4 commits June 16, 2017 14:35

Fixes build scripts to find msbuild.exe and stop relying on vcvarsall…

6cf00ec

….bat Also fixes bdist_wininst.vcxproj to use correct version in generated name.

Fixes line endings and adds script to fix line endings.

8f884e6

Fixes line endings and adds script to fix line endings.

0e911fa

Fixes readme.txt line endings.

6ed6279

zooba added needs backport to 3.5 labels Jun 16, 2017

zooba self-assigned this Jun 16, 2017

zooba requested a review from zware June 16, 2017 21:56

the-knights-who-say-ni added the CLA signed label Jun 16, 2017

zooba removed the trivial label Jun 16, 2017

zooba changed the title ~~Fixes build scripts to find msbuild.exe and stop relying on vcvarsall.bat~~ bpo-30687: Fixes build scripts to find msbuild.exe and stop relying on vcvarsall.bat Jun 16, 2017

zooba requested a review from tjguk June 16, 2017 22:17

zware reviewed Jun 16, 2017

View reviewed changes

zooba and others added 8 commits June 16, 2017 16:06

Fixes missing PATH entry for PGO builds.

7012dcc

Skips UCRT check for PGO builds.

Removes vs_platf variable and PGO warning from build.bat.

6f04251

Add NEWS entry.

8d2d7dc

Normalises line endings again.

502a337

More line ending changes

1506793

Merge from master

c701a70

Restores PGO on Win32 check.

277410d

Change line endings in NEWS file

324cc5c

zware requested changes Jun 18, 2017

View reviewed changes

zooba added 3 commits June 19, 2017 10:10

Fixes from review

bae2ad2

Fix line endings

f169ae4

Fix line endings

3194ce5

zware approved these changes Jun 19, 2017

View reviewed changes

zooba merged commit 40a23e8 into python:master Jun 19, 2017

zooba deleted the msbuild-finder branch June 19, 2017 17:34

This was referenced Jun 19, 2017

[3.6] bpo-30687: Fixes build scripts to find msbuild.exe and stop relying on vcvarsall.bat #2280

Merged

[3.5] bpo-30687: Fixes build scripts to find msbuild.exe and stop relying on vcvarsall.bat #2281

Merged

zooba removed the needs backport to 3.6 label Sep 6, 2017

bpo-30687: Fixes build scripts to find msbuild.exe and stop relying on vcvarsall.bat #2252

bpo-30687: Fixes build scripts to find msbuild.exe and stop relying on vcvarsall.bat #2252

Conversation

zooba commented Jun 16, 2017

mention-bot commented Jun 16, 2017

zooba commented Jun 16, 2017

zooba commented Jun 16, 2017

zware left a 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

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

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

zooba commented Jun 17, 2017

ned-deily commented Jun 17, 2017

zooba commented Jun 17, 2017

tjguk commented Jun 17, 2017 via email

tjguk commented Jun 17, 2017 via email

zware left a 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

zooba commented Jun 19, 2017

zooba commented Jun 19, 2017

zware commented Jun 19, 2017

zooba commented Jun 19, 2017

tjguk commented Jun 19, 2017 via email

zooba commented Jun 19, 2017

tjguk commented Jun 19, 2017 via email