1407678 - Make windows_toolchain.py support VS2017

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Comment 1

[(Away)]

Attachment: Part 0: Abort if you have the wrong SDK version

Comment 2

[(Away)]

On second thought, I'm going to widen this bug to do everything needed to support VS2017. I don't want to have patches scattered among a bunch of bugs.

Comment 4

[(Away)]

Attachment: Part 1: Update JarWriter usage to match the param change in bug 1208320

Comment 5

[(Away)]

Attachment: Part 2: Separate the DIA, VC, and Redist paths

This looks odd at the moment, but this will make the next patch more clear since the organization of the paths changed in VS2017.

Comment 6

[(Away)]

Attachment: Part 3: Update paths for VS2017

Comment 7

[(Away)]

Attachment: Part 4: Update to Windows SDK 10.0.16299.0

It's unclear whether we actually need this, but is there any reason not to?

Comment 8

[(Away)]

Attachment: Part 4: Update to Windows SDK 10.0.16299.0

Turns out, in SDK 10.0.16299.0, the interesting binaries live in bin/$SDKVER/x64 rather than bin/x64.

Comment 9

[Michael Shal [:mshal]]

Comment on attachment 8917968 [details] [diff] [review]
Part 2: Separate the DIA, VC, and Redist paths

>+    dia_path = os.path.join(vs_path, 'DIA SDK')
>+    assert os.path.exists(dia_path)
>+    paths.append((dia_path, 'DIA SDK', DIA_PATTERNS))
>+
>+    vc_path = os.path.join(vs_path, 'VC')
>+    assert os.path.exists(vc_path)
>+    paths.append((vc_path, 'VC', VC_PATTERNS))
>+
>+    redist_path = os.path.join(vs_path, 'VC', 'redist')
>+    assert os.path.exists(redist_path)
>+    paths.append((redist_path, 'VC/redist', REDIST_PATTERNS))

This might be simpler if you just had a single dict or list of tuples called PATTERNS that you could iterate over. You could use %{pf}s and such as placeholders to do the path manipulation in this function.

Comment 10

[Michael Shal [:mshal]]

Comment on attachment 8917970 [details] [diff] [review]
Part 3: Update paths for VS2017

Do we need to support both 2015 & 2017 until everything uses 2017? The instructions at build/docs/toolchains.rst still say to use 2015, for example, which presumably won't work with the new paths.

Comment 11

[(Away)]

(In reply to Michael Shal [:mshal] from comment #10)
> Comment on attachment 8917970 [details] [diff] [review]
> Part 3: Update paths for VS2017
> 
> Do we need to support both 2015 & 2017 until everything uses 2017? The
> instructions at build/docs/toolchains.rst still say to use 2015, for
> example, which presumably won't work with the new paths.

It looks like we only use this file on toolchain updates (see bug 1407667 comment 1 for some laughs) and it seems very unlikely that we would take another 2015 update before we switch to 2017.

Comment 12

[(Away)]

(In reply to Michael Shal [:mshal] from comment #9)
> This might be simpler if you just had a single dict or list of tuples called
> PATTERNS that you could iterate over. You could use %{pf}s and such as
> placeholders to do the path manipulation in this function.

I'm not quite clear on what this means. I understand the part about a single PATTERNS, that makes sense, but can you elaborate on what you're suggesting for find_vs_paths()?

Comment 13

[Michael Shal [:mshal]]

Comment on attachment 8917970 [details] [diff] [review]
Part 3: Update paths for VS2017

(In reply to David Major [:dmajor] from comment #11)
> (In reply to Michael Shal [:mshal] from comment #10)
> > Comment on attachment 8917970 [details] [diff] [review]
> > Part 3: Update paths for VS2017
> > 
> > Do we need to support both 2015 & 2017 until everything uses 2017? The
> > instructions at build/docs/toolchains.rst still say to use 2015, for
> > example, which presumably won't work with the new paths.
> 
> It looks like we only use this file on toolchain updates (see bug 1407667
> comment 1 for some laughs) and it seems very unlikely that we would take
> another 2015 update before we switch to 2017.

Fair point :)

-            'lib/arm/**',
             'lib/onecore/**',
-            'lib/store/**',
+            'lib/x64/store/**',
+            'lib/x86/store/**',

Are the 'onecore' and 'store' paths generated later? They don't seem to be present in either the vs2015 tarball or the new 2017 tarball.

>-    vc_path = os.path.join(vs_path, 'VC')
>+    vc_path = os.path.join(vs_path, 'VC', 'Tools', 'MSVC', '14.11.25503')

>-    redist_path = os.path.join(vs_path, 'VC', 'redist')
>+    redist_path = os.path.join(vs_path, 'VC', 'Redist', 'MSVC', '14.11.25325')

The tooltool patches uploaded in bug 1408455 don't have these paths.

Comment 14

[Michael Shal [:mshal]]

(In reply to David Major [:dmajor] from comment #12)
> (In reply to Michael Shal [:mshal] from comment #9)
> > This might be simpler if you just had a single dict or list of tuples called
> > PATTERNS that you could iterate over. You could use %{pf}s and such as
> > placeholders to do the path manipulation in this function.
> 
> I'm not quite clear on what this means. I understand the part about a single
> PATTERNS, that makes sense, but can you elaborate on what you're suggesting
> for find_vs_paths()?

It doesn't look like you can completely hard-code the full struct since some of the paths are dynamically calculated, so I was suggesting setting up PATTERNS like this (if assuming a tuple - it may be worthwhile trying a dict or some class to be explicit too):

PATTERNS = [
   ('%(pf)s/Windows Kits/10', 'SDK', [
     # SDK_PATTERNS here
    ]),
   ('%(vs_path)s/VC', 'VC', [ 
     # VC_PATTERNS here
    ]),
# etc
]

in find_vs_paths, assuming you still have pf and vs_path and your other path checks:

paths = []
for (path, dest, pattern_list) in PATTERNS:
    fullpath = path % {
        'pf': pf,
        'vs_path': vs_path,
    }
    paths.append((fullpath, dest, pattern_list))

(untested, but hopefully that makes sense)

Comment 15

[(Away)]

(In reply to Michael Shal [:mshal] from comment #13)
> Are the 'onecore' and 'store' paths generated later? They don't seem to be
> present in either the vs2015 tarball or the new 2017 tarball.

These are 'ignore' paths. This is the very code that prevents those unwanted onecore files from reaching the tarball. :)

> 
> >-    vc_path = os.path.join(vs_path, 'VC')
> >+    vc_path = os.path.join(vs_path, 'VC', 'Tools', 'MSVC', '14.11.25503')
> 
> >-    redist_path = os.path.join(vs_path, 'VC', 'redist')
> >+    redist_path = os.path.join(vs_path, 'VC', 'Redist', 'MSVC', '14.11.25325')
> 
> The tooltool patches uploaded in bug 1408455 don't have these paths.

Right. So in VS2017, MS added a bunch of extra directory levels to the install paths, like:
> C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.11.25503\bin\HostX64\x64\cl.exe
(and now things are more scattered across a bunch of different directories, to boot :-/)

In the second param of the `paths.append` calls below there, I deliberately kept them mapped to the old-style simple paths in the zip file. Otherwise it would just be a ton of extra typing in places like: https://dxr.mozilla.org/mozilla-central/source/build/win64/mozconfig.vs2015#13

Do you think I should preserve those full paths?

Comment 16

[Michael Shal [:mshal]]

Comment on attachment 8918412 [details] [diff] [review]
Part 4: Update to Windows SDK 10.0.16299.0

> # Files from the Windows 10 SDK to install.
> SDK_PATTERNS = [
>     {
>-        'pattern': 'bin/x64/**',
>+        'pattern': 'bin/%s/x64/**' % SDK_RELEASE,

This also doesn't seem to match the new tooltool archive. I think it's still bin/x64/**? Though there are only two files in there, so maybe something else is wrong.

Comment 17

[Michael Shal [:mshal]]

(In reply to David Major [:dmajor] from comment #15)
> (In reply to Michael Shal [:mshal] from comment #13)
> > Are the 'onecore' and 'store' paths generated later? They don't seem to be
> > present in either the vs2015 tarball or the new 2017 tarball.
> 
> These are 'ignore' paths. This is the very code that prevents those unwanted
> onecore files from reaching the tarball. :)
> 
> > 
> > >-    vc_path = os.path.join(vs_path, 'VC')
> > >+    vc_path = os.path.join(vs_path, 'VC', 'Tools', 'MSVC', '14.11.25503')
> > 
> > >-    redist_path = os.path.join(vs_path, 'VC', 'redist')
> > >+    redist_path = os.path.join(vs_path, 'VC', 'Redist', 'MSVC', '14.11.25325')
> > 
> > The tooltool patches uploaded in bug 1408455 don't have these paths.
> 
> Right. So in VS2017, MS added a bunch of extra directory levels to the
> install paths, like:
> > C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.11.25503\bin\HostX64\x64\cl.exe
> (and now things are more scattered across a bunch of different directories,
> to boot :-/)
> 
> In the second param of the `paths.append` calls below there, I deliberately
> kept them mapped to the old-style simple paths in the zip file. Otherwise it
> would just be a ton of extra typing in places like:
> https://dxr.mozilla.org/mozilla-central/source/build/win64/mozconfig.
> vs2015#13
> 
> Do you think I should preserve those full paths?

Sorry, I'm being dumb. I don't know why I'm thinking of the tooltool file as the input instead of output.

Comment 18

[(Away)]

Comment on attachment 8917968 [details] [diff] [review]
Part 2: Separate the DIA, VC, and Redist paths

(In reply to Michael Shal [:mshal] from comment #14)
> paths = []
> for (path, dest, pattern_list) in PATTERNS:
>     fullpath = path % {
>         'pf': pf,
>         'vs_path': vs_path,
>     }
>     paths.append((fullpath, dest, pattern_list))

Oh, I see what you mean. I like it.

Comment 19

[(Away)]

Attachment: Part 2 (v2): Refactor the paths logic to be more extensible.

This implements the dict suggestion from earlier (sorry for the delay, I was traveling). I've checked that this produces the same checksum as before.

Comment 20

[(Away)]

I was previously missing Hostx86/x86/pgort140.dll from the package. I've added it to my local copy of Part 3 and it's included in Ralph's update above. I'll upload a rebased version once we're done with review on Part 2.

Comment 21

[Michael Shal [:mshal]]

Comment on attachment 8921568 [details] [diff] [review]
Part 2 (v2): Refactor the paths logic to be more extensible.

LGTM! Thanks for updating this.

Comment 22

[(Away)]

Attachment: Part 3: Update paths for VS2017

Rebased, and added x86/pgort140.dll

Comment 23

[(Away)]

Attachment: Part 4: Update to Windows SDK 10.0.16299.0

Rebased

Comment 24

[(Away)]

Oops, I forgot to land this, sorry! I'll include the SDK change from bug 1413675 as well.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f56c67e2a2a
Make windows_toolchain.py support VS2017. r=mshal DONTBUILD

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1a5c40bc27
Make windows_toolchain.py support VS2017: enter empty line to make flake8 happy. r=linting-fix DONTBUILD

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3f56c67e2a2a
https://hg.mozilla.org/mozilla-central/rev/6c1a5c40bc27