message_compiler.py reports differences with Fall Creators Update SDK |
||
Issue descriptionWhen building with the Fall Creators Update SDK some differences are seen compared to the canonical versions - see attached messaging_errors.txt. These do not show up reliably because when the toolchain is switched ninja doesn't realize that the midl.py and message_compiler.py steps need to be re-run. Therefore the errors do not reliably show up unless some other change is introduced to force the issue. crrev.com/c/713580 *should* reliably reproduce the issues. Using that CL, and these gn args: is_component_build = false is_debug = true and this build command: > ninja -C out\win7 gen/base/trace_event/etw_manifest/chrome_events_win.rc should reliably reproduce the issue. Arguably we should also consider fixing the issue that these errors are not reliably detected when switching SDKs (the paths where mc.exe and midl.exe are found) but that is lower priority and no obvious fix comes to mind.
,
Oct 16 2017
To address the second part of the bug (steps not reliably running after a toolchain change) the correct solution would be to have three separate steps: one runs your script, one runs mc.exe or midl.py, and a third step compares the results. The last two steps would be NOPs on non-Windows platforms. Then, the running of mc.exe and midl.py would automatically happen when toolchains change because the command-line would change (at least that's how it works with the compiler and linker).
,
Oct 20 2017
I deliberated what to do here. I first thought that we could have different expectations for different versions of mc.exe checked in, but then those become impossible to update if a .mc or .man input should change. Since the motivation of the check is to remind us to update the checked-in outputs when the inputs change, I think here's what I should do here: - get version of mc.exe (TODO: how? mc.exe doesn't have a /version switch, but maybe it has a VERSIONINFO structure in its PE resource dir, so maybe I could look for that?), and if it's equal to the one in the active SDK, do the comparison, else error out early Then, when we update the default SDK, we need to increase the version number and the golden files shortly after. If we forget to do that, we silently lose checking. Not ideal, but the best I could come up with so far.
,
Oct 20 2017
Here's the output I'm getting btw. I think the output in comment 0 is a different thing that should've been resolved by https://chromium-review.googlesource.com/c/chromium/src/+/697425
,
Oct 20 2017
I think that that plan (only check the results when using the "golden" version of mc.exe) makes sense. When I run mc.exe with no parameters it prints the version number on the first line - currently one of these two: Microsoft (R) Message Compiler Version 10.0.14393 Copyright (c) Microsoft Corporation. All rights reserved. Microsoft (R) Message Compiler Version 10.0.16299 Copyright (c) Microsoft Corporation. All rights reserved. It then prints the usage, but that can be ignored. This also neatly works around the bug where many users end up with the 14393 version in their path (I've filed a VS bug about that) since that will tell you to worry less or not at all about the output.
,
Oct 24 2017
Thanks for pointing that out. I did try to look there before claiming that I can't see the version, but apparently I looked at it and then decided it wasn't there O_o
,
Oct 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/02ba53c693281052d9b52d411f34d125f4bf40a3 commit 02ba53c693281052d9b52d411f34d125f4bf40a3 Author: Nico Weber <thakis@chromium.org> Date: Tue Oct 24 22:18:37 2017 message_compiler.py: Check expected outputs for .man files only for pinned SDK version. mc.exe's output for .man files is changing significantly in the Fall Creator's SDK update. Checking in outputs for multiple outputs means we'll never be able to check in new baselines if the .man input changes, so we have to decide on just one version. So get mc.exe's version and only compare if it's the version that belongs to the SDK hash that's the default in vs_toolchain.py. We'll have to manually remember to update the checked-in output and to bump the version number here after we update to the Fall SDK, else we'll silently lose output checking for .man files, but that seems like the lesser evil. Bug: 774669 Change-Id: I841d10c53f60ea645cca1640e2a7be204ac0f31f Reviewed-on: https://chromium-review.googlesource.com/735073 Reviewed-by: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#511280} [modify] https://crrev.com/02ba53c693281052d9b52d411f34d125f4bf40a3/build/win/message_compiler.py
,
Oct 24 2017
The "we don't re-mc on toolchain rev" thing isn't new, so closing out this bug. (A simpler fix for that would be to just pass the toolchain rev as an argument that's then ignored, or to give message_compiler edges a dep on the env.x86 file)
,
Dec 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fb4046a4b0fd9651c74c7dcd4638688c53937048 commit fb4046a4b0fd9651c74c7dcd4638688c53937048 Author: Bruce Dawson <brucedawson@chromium.org> Date: Tue Dec 05 01:30:00 2017 Don't check message compiler results for .mc files message_compiler.py checks to make sure its results match those generated by mc.exe, but these checks are only valid for a given version of mc.exe, because the output changes with every new Windows 10 SDK version. The checking is disabled for SDK versions other than 10.0.15063 but only for .man files. This change extends the disabling of checking for .mc files also, due to observed failures. Bug: 774669 Change-Id: I618f68d8926b73621192e7933aea5d264fedd214 Reviewed-on: https://chromium-review.googlesource.com/807205 Reviewed-by: Scott Graham <scottmg@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#521577} [modify] https://crrev.com/fb4046a4b0fd9651c74c7dcd4638688c53937048/build/win/message_compiler.py |
||
►
Sign in to add a comment |
||
Comment 1 by thakis@chromium.org
, Oct 13 2017