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.

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

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

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

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 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.

(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.

(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 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.

(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)

(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 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.

(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 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.

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.

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 on attachment 8921568 [details] [diff] [review] Part 2 (v2): Refactor the paths logic to be more extensible. LGTM! Thanks for updating this.

Rebased, and added x86/pgort140.dll

Rebased

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

Pushed by dmajor@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3f56c67e2a2a Make windows_toolchain.py support VS2017. r=mshal DONTBUILD

Pushed by archaeopteryx@coole-files.de: 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

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