Closed
Bug 1371465
Opened 8 years ago
Closed 8 years ago
Visual Studio 2015 projects are generated when using Visual Studio 2017
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(firefox56 fixed)
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: gps, Assigned: gps)
References
Details
Attachments
(1 file)
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) |
Comment 2•8 years ago
|
||
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•8 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.
Comment hidden (mozreview-request) |
Comment 5•8 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)
Comment 6•8 years ago
|
||
(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•8 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•8 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•8 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•8 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
![]() |
||
Comment 14•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•