All users were logged out of Bugzilla on October 13th, 2018

Make Visual Studio 2015u3 the minimum supported Windows compiler version

RESOLVED FIXED in Firefox 51

Status

RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: RyanVM, Assigned: RyanVM)

Tracking

Trunk
mozilla53

Firefox Tracking Flags

(firefox51+ fixed, firefox52+ fixed, firefox53+ fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
We've been building with 2015u3 as the default since Fx51, we should enforce it as the minimum version at configure time as well. I also think we should backport this to 52 for the next ESR so we don't find ourselves running into uplift pain down the road when a patch ends up depending on a feature in u3.

Comment 1

2 years ago
I agree with comment #0. But if we change this, let's hold off until next week, as I don't want to disrupt anyone doing hacking over the long weekend.
tracking this for 52 since it looks like bug 1319004 requires 2015u3 and will land in 52
tracking-firefox52: --- → +
(Assignee)

Comment 3

2 years ago
Bug 1319004 was uplifted to 51 and bug 1320471 was WONTFIXed. This continues to cause issues for people building on Windows, so I think we need to get this landed and uplifted to 51+ ASAP.
status-firefox51: fix-optional → affected
tracking-firefox51: --- → ?
tracking-firefox53: --- → ?
(Assignee)

Comment 4

2 years ago
Created attachment 8819153 [details] [diff] [review]
[52+] Enforce vs2015u3 as the minimum Windows compiler version

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c90cd4dc3d4ee8c2039ddb643c29fe638bfd48f9

This applies cleanly to Aurora as well. Nate says he'll look at the clang-cl bustage tomorrow, but I at least want to get the review going on this in the mean time.
Assignee: nobody → ryanvm
Status: NEW → ASSIGNED
Attachment #8819153 - Flags: review?(gps)
Track 51+ as build issue on Windows.
tracking-firefox51: ? → +
(In reply to Ryan VanderMeulen [:RyanVM] from comment #4)
> This applies cleanly to Aurora as well. Nate says he'll look at the clang-cl
> bustage tomorrow, but I at least want to get the review going on this in the
> mean time.

OK, I think what's happening here is this change is causing the problem:

https://hg.mozilla.org/try/rev/5290117bf86c3bec4dde3a4d77fbf107ff7376f6#l2.12

The check says: "if we're clang-cl, and we don't seem to be emulating the correct MSVC version, then add these flags".  Unfortunately, "add these flags" is not only "make us emulate the correct MSVC version" but also "add -fallback so we always fallback to MSVC if clang-cl can't compile something".  It assumes that clang-cl starts out not emulating the MSVC version we need.  But if clang-cl is *already* emulating the correct MSVC version without any extra flags--there seems to be some logic for figuring out what the default should be in clang-cl--then we're never going to enter that case and therefore never going to add -fallback.

There are two different ways to address this problem, one of which definitely works and one of which I am not sure about.

1) Somebody can review my patch in bug 1321379, which sets up the compilation environment for nss's gyp files correctly for clang-cl.  This ensures that nss's build system does the right thing when using clang-cl.  I am reasonably confident this works because I was testing it last week.

2) We can move the addition of -fallback up to where |flags| is initialized:

https://hg.mozilla.org/try/file/5290117bf86c/build/moz.configure/toolchain.configure#l340

So it would be something like:

    flags = []
    if info.type == 'clang-cl':
        flags.append('-fallback')

I think this works, but it would have to be tested, because tests, I think (?), invoke check_compiler twice, compare the results, and complain if they are not identical.  They *should* be, with the above, but one would need to make sure.
(Assignee)

Updated

2 years ago
Attachment #8819153 - Flags: review?(gps) → review?(ted)
Comment on attachment 8819153 [details] [diff] [review]
[52+] Enforce vs2015u3 as the minimum Windows compiler version

Review of attachment 8819153 [details] [diff] [review]:
-----------------------------------------------------------------

We sure do repeat the version number a lot around here. It'd be nice if we didn't have to do that.

::: python/mozbuild/mozbuild/test/configure/test_toolchain_configure.py
@@ +768,4 @@
>          'See https://developer.mozilla.org/en/Windows_Build_Prerequisites')
> +    VS_2015u2_RESULT = (
> +        'This version (19.00.23918) of the MSVC compiler is not supported.\n'
> +        'You must install Visual C++ 2015 Update 3 or newer in order to build.\n'

Could you factor these error messages out into a common template so we don't have to keep making this change every time we bump the minimum supported version? Like

```
MSVC_ERROR_TEMPLATE = ''''This version ({}) of the MSVC compiler is not supported.
...
'''

VS_2013u2_RESULT = MSVC_ERROR_TEMPLATE.format(...)
```

Maybe you could stick a `.version` property on the `FakeCompiler` instances the `VS` function returns so you could just write `VS_2013u2.version` there:
https://dxr.mozilla.org/mozilla-central/rev/63b447888a6469b9f6ae8f76ac5f0d7c6ea239da/python/mozbuild/mozbuild/test/configure/test_toolchain_configure.py#200
Attachment #8819153 - Flags: review?(ted) → review+
(Assignee)

Comment 10

2 years ago
Thanks for the review, Ted! I'm going to file a follow-up bug for addressing the review comments in the interest of getting this landed ASAP (ongoing developer frustration), but I'll happily take that bug too.

Comment 11

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ebebbf1339e5
Enforce Visual Studio 2015 Update 3 as the minimum supported compiler version for Windows builds. r=ted
(Assignee)

Comment 12

2 years ago
Comment on attachment 8819153 [details] [diff] [review]
[52+] Enforce vs2015u3 as the minimum Windows compiler version

Approval Request Comment
[Feature/Bug causing the regression]: Not really a regression, but bug 1283203 is what made vs2015u3 the default Windows compiler in automation.
[User impact if declined]: Build bustage for developers using vs2015u2 without any clear reason for why. This patch will cause the build to fail during configure with an explicit error message, which is a clear improvement over the status quo.
[Is this code covered by automated tests?]: Yes. And this patch updates them accordingly.
[Has the fix been verified in Nightly?]: It's green in CI and locally. Given that it's a build system patch, that's the best verification we're going to get.
[Needs manual test from QE? If yes, steps to reproduce]: None required.
[List of other uplifts needed for the feature/fix]: None. There was an extra dependency required for trunk clang-cl builds, but those don't run on Aurora/Beta (as proven by green Try runs).
[Is the change risky?]: No, it just enforces the compiler we've been using by default since 51 as the minimum we'll allow people to use.
[Why is the change risky/not risky?]: It's actually less risky than not uplifting IMO. Allowing developers to use older compilers than we use in automation is begging for problems down the road (as already evidenced by bug 1321094).
[String changes made/needed]: None.
Attachment #8819153 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 13

2 years ago
Comment on attachment 8819154 [details] [diff] [review]
[51] Enforce vs2015u3 as the minimum Windows compiler version

See comment 12.
Attachment #8819154 - Flags: approval-mozilla-beta?
Comment on attachment 8819153 [details] [diff] [review]
[52+] Enforce vs2015u3 as the minimum Windows compiler version

enforce minimum VS version at configure time rather than failing to build, take in aurora52
Attachment #8819153 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ebebbf1339e5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8819154 [details] [diff] [review]
[51] Enforce vs2015u3 as the minimum Windows compiler version

Make Visual Studio 2015u3 the minimum supported Windows compiler version. Beta51+. Should be in 51 beta 9.
Attachment #8819154 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 17

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/06bdb65a9a46
status-firefox52: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/be8b4d81d7cf
status-firefox51: affected → fixed
tracking-firefox53: ? → +
With this update I actually have 19.00.24210, not 24213..

Comment 20

2 years ago
(In reply to Bas Schouten (:bas.schouten) from comment #19)
> With this update I actually have 19.00.24210, not 24213..

same here with the german community edition ... the RTM version of 2015u3 is 19.00.24210.
(Assignee)

Comment 21

2 years ago
Interesting! What version do you end up at if you install the KB3165756 update? It should be available via the in-product update. Otherwise, you can get it from the link below.
https://msdn.microsoft.com/en-us/library/mt752379.aspx

Comment 22

2 years ago
I nether used the Visual Studio IDE (I'm only using it for building Firefox) and the update was not offered within windows update. Now, after starting the IDE and installing the update, the version is 19.00.2415.1
I /thought/ the standalone Visual Studio installer would automatically install the latest versions and you wouldn't need to use the in-product updater.

Anyway, KB3165756 is somewhat important from a bug fix perspective - there are references to it in bug 1295977 and bug 1283203. I have a pretty strong opinion that we should require that as the minimum version. Perhaps we could improve the error message to include instructions on updating VS2015.
Also, `mach bootstrap` works on Windows now. I /think/ we should be able to invoke the Visual Studio installer in headless mode to install any updates.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)
> Interesting! What version do you end up at if you install the KB3165756
> update? It should be available via the in-product update. Otherwise, you can
> get it from the link below.
> https://msdn.microsoft.com/en-us/library/mt752379.aspx

I didn't see any in-product updates, but that KB fixed the issue for me. I added a note to MDN.
(In reply to Bas Schouten (:bas.schouten) from comment #19)
> With this update I actually have 19.00.24210, not 24213..

Can confirm I also have 19.00.24210, and my ability to do builds is now kaput without reverting this patch.

I use Visual Studio 2015 (Update 3) Express for Desktop.
Flags: needinfo?(ryanvm)
Couldn't force VS to update any other way than through installing KB3165756 manually, but that seems to have resolved the issue for me.
Flags: needinfo?(ryanvm)

Updated

8 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.