1371465 - Visual Studio 2015 projects are generated when using Visual Studio 2017

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Gregory Szorc [:gps]]

We're failing to set MSVS_VERSION=2017 in old-configure.in. I swear this used to work. But it isn't working now. As a result, we don't generate the proper visual studio solution and project files when using 2017. I think the fix is trivial and will attempt to submit a patch shortly...

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1371465 - Move MSVS_VERSION to moz.configure and properly define for vs2017;

Before, MSVS was set in old-configure and could only be unset or
"2015." We move the definition of the variable to moz.configure
and support defining its value as "2017" when VS2017 is being used.

As part of this, I discovered that GYP barfs with a "2017" value.
This is likely a limitation of the legacy version of GYP we have
vendored. Rather than go down the rabbit hole of upgrading GYP,
I added code to convert the value to "2015." This preserves existing
behavior and unblocks us from setting MSVS_VERSION properly. A
warning is emitted to remind us to remove this hack once GYP is
upgraded.

After this commit, we now generate native VS2017 solutions and
projects when building with VS2017.

Review commit: https://reviewboard.mozilla.org/r/147332/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147332/

Comment 2

[(not currently active) Ted Mielczarek]

File a followup to update gyp? It shouldn't be too bad, we're fairly up-to-date and gyp doesn't get that many commits these days.

Comment 3

[Gregory Szorc [:gps]]

GYP is vendored as part of WebRTC (for better or worse). I didn't feel like opening that can of worms. But I can get a bug on file and add an inline TODO referencing it.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8875933 [details]
Bug 1371465 - Move MSVS_VERSION to moz.configure and properly define for vs2017;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147332/diff/1-2/

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8875933 [details]
Bug 1371465 - Move MSVS_VERSION to moz.configure and properly define for vs2017;

https://reviewboard.mozilla.org/r/147332/#review152812

::: build/moz.configure/toolchain.configure:469
(Diff revision 2)
>  def vs_major_version(value):
>      if value:
>          return {'2015': 14,
>                  '2017': 15}[value[0]]
>  
> +@depends(vs_major_version)

you want to be depending on the actual version of MSVC, not the one maybe passed through --with-visual-studio-version (which, in most cases, is not going to be passed anyways)

So, something with @depends(c_compiler) and checking that the .type is "msvc" and what .version it is (which is not going to be 14 or 15, btw, since that's going to be the compiler version, not the msvc version, thank you MS for the confusing version scheme).

Comment 6

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #5)
> you want to be depending on the actual version of MSVC, not the one maybe
> passed through --with-visual-studio-version (which, in most cases, is not
> going to be passed anyways)

BTW, your try is busted on windows because of that.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8875933 [details]
Bug 1371465 - Move MSVS_VERSION to moz.configure and properly define for vs2017;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147332/diff/2-3/

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8875933 [details]
Bug 1371465 - Move MSVS_VERSION to moz.configure and properly define for vs2017;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147332/diff/3-4/

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8875933 [details]
Bug 1371465 - Move MSVS_VERSION to moz.configure and properly define for vs2017;

Still having issues with this. Will need to sit in front of a Windows computer to debug.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8875933 [details]
Bug 1371465 - Move MSVS_VERSION to moz.configure and properly define for vs2017;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147332/diff/4-5/

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8875933 [details]
Bug 1371465 - Move MSVS_VERSION to moz.configure and properly define for vs2017;

https://reviewboard.mozilla.org/r/147332/#review154228

::: build/moz.configure/toolchain.configure:881
(Diff revision 5)
> +@depends(c_compiler)
> +def msvs_version(info):
> +    # clang-cl emulates the same version scheme as cl. And MSVS_VERSION needs to
> +    # be set for GYP on Windows.
> +    if info.type in ('clang-cl', 'msvc'):
> +        if info.version >= '19.10':

should we preemptively abort with newer versions?

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8875933 [details]
Bug 1371465 - Move MSVS_VERSION to moz.configure and properly define for vs2017;

https://reviewboard.mozilla.org/r/147332/#review154228

> should we preemptively abort with newer versions?

I like failing fast. But what would that version be? I think Microsoft surprised everyone by making 2017's C compiler version 19.10 instead of 20.0. Who knows what they'll do with rapid releases. If e.g. they release vs2017u3 as 19.20 or something, I don't want to have to go backporting patches because of an overly strict version check.

If this is problematic, we can always follow up.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/398d9d496775
Move MSVS_VERSION to moz.configure and properly define for vs2017; r=glandium

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/398d9d496775