Closed Bug 1407678 Opened 2 years ago Closed 2 years ago

Make windows_toolchain.py support VS2017

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: dmajor, Assigned: dmajor)

References

Details

Attachments

(5 files, 4 obsolete files)

No description provided.
Assignee: nobody → dmajor
Attachment #8917440 - Flags: review?(ted)
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.
Attachment #8917440 - Attachment description: sdkver → Abort if you have the wrong SDK version
Summary: Make windows_toolchain.py abort if you have the wrong SDK version → Make windows_toolchain.py support VS2017
Blocks: 1318193
Duplicate of this bug: 1407667
This looks odd at the moment, but this will make the next patch more clear since the organization of the paths changed in VS2017.
Attachment #8917440 - Attachment description: Abort if you have the wrong SDK version → Part 0: Abort if you have the wrong SDK version
Attachment #8917967 - Attachment description: Update JarWriter usage to match the param change in bug 1208320 → Part 1: Update JarWriter usage to match the param change in bug 1208320
Attachment #8917968 - Attachment description: Separate the DIA, VC, and Redist paths → Part 2: Separate the DIA, VC, and Redist paths
Attached patch Part 3: Update paths for VS2017 (obsolete) — Splinter Review
It's unclear whether we actually need this, but is there any reason not to?
Attachment #8917971 - Attachment description: Update to Windows SDK 10.0.16299.0 → Part 4: Update to Windows SDK 10.0.16299.0
Attachment #8917440 - Flags: review?(ted) → review?(core-build-config-reviews)
Attachment #8917967 - Flags: review?(core-build-config-reviews)
Attachment #8917968 - Flags: review?(core-build-config-reviews)
Attachment #8917970 - Flags: review?(core-build-config-reviews)
Attachment #8917971 - Flags: review?(core-build-config-reviews)
Turns out, in SDK 10.0.16299.0, the interesting binaries live in bin/$SDKVER/x64 rather than bin/x64.
Attachment #8917971 - Attachment is obsolete: true
Attachment #8917971 - Flags: review?(core-build-config-reviews)
Attachment #8918412 - Flags: review?(core-build-config-reviews)
Attachment #8917440 - Flags: review?(core-build-config-reviews) → review+
Attachment #8917967 - Flags: review?(core-build-config-reviews) → review+
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()?
Flags: needinfo?(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.
Flags: needinfo?(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)
(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.
Attachment #8917970 - Flags: review?(core-build-config-reviews) → review+
Attachment #8918412 - Flags: review?(core-build-config-reviews) → review+
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.
Attachment #8917968 - Flags: review?(core-build-config-reviews)
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.
Attachment #8917968 - Attachment is obsolete: true
Attachment #8921568 - Flags: review?(mshal)
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.
Attachment #8921568 - Flags: review?(mshal) → review+
Rebased, and added x86/pgort140.dll
Attachment #8917970 - Attachment is obsolete: true
Attachment #8921627 - Flags: review+
Rebased
Attachment #8918412 - Attachment is obsolete: true
Attachment #8921628 - Flags: review+
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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.