Visual Studio 2015 projects are generated when using Visual Studio 2017

RESOLVED FIXED in Firefox 56

Status

RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: gps, Assigned: gps)

Tracking

Trunk
mozilla56
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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 hidden (mozreview-request)
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.
(Assignee)

Comment 3

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1371485
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
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).
Attachment #8875933 - Flags: review?(mh+mozilla)
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
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.
Attachment #8875933 - Flags: review?(mh+mozilla)
Comment hidden (mozreview-request)

Comment 11

2 years ago
mozreview-review
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?
Attachment #8875933 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 12

2 years ago
mozreview-review-reply
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

2 years ago
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/398d9d496775
Move MSVS_VERSION to moz.configure and properly define for vs2017; r=glandium
https://hg.mozilla.org/mozilla-central/rev/398d9d496775
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

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