1319901 - Make Visual Studio 2015u3 the minimum supported Windows compiler version

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

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

[Gregory Szorc [:gps]]

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.

Comment 2

[Julien Cristau [:jcristau]]

tracking this for 52 since it looks like bug 1319004 requires 2015u3 and will land in 52

Comment 3

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Attachment: [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.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Attachment: [51] Enforce vs2015u3 as the minimum Windows compiler version

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

Comment 6

[Gerry Chang [:gchang]]

Track 51+ as build issue on Windows.

Comment 7

[Nathan Froyd [:froydnj]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Try push on top of bug 1321379 looks good!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=596f2e5d3e4d4d10e2615d0bdefd84ded795712d&group_state=expanded

Comment 9

[(not currently active) Ted Mielczarek]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

See comment 12.

Comment 14

[Julien Cristau [:jcristau]]

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

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ebebbf1339e5

Comment 16

[Gerry Chang [:gchang]]

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.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/06bdb65a9a46

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/be8b4d81d7cf

Comment 19

[Bas Schouten (:bas.schouten)]

With this update I actually have 19.00.24210, not 24213..

Comment 20

[Ronny Perinke]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

[Ronny Perinke]

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

Comment 23

[Gregory Szorc [:gps]]

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.

Comment 24

[Gregory Szorc [:gps]]

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.

Comment 25

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

Comment 26

[Lee Salzman [:lsalzman]]

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

Comment 27

[Lee Salzman [:lsalzman]]

Couldn't force VS to update any other way than through installing KB3165756 manually, but that seems to have resolved the issue for me.