The default bug view has changed. See this FAQ.

Explicitly require Update 3 for MSVC 2013 as the minimum supported compiler version

RESOLVED FIXED in mozilla37

Status

()

Core
Build Config
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: RyanVM, Assigned: ted)

Tracking

Trunk
mozilla37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Spun off from the discussion in bug 1117820. In automation, we use MSVC 2013 Update 3. AFAIK, the build system currently only checks the major version and therefore allows earlier versions of MSVC 2013 to work.

Bug 914596 comment 12 and down contains some discussion about bug fixes that shipped in Update 3. I also seem to remember discussion in a bug about different DLL versions for the various Update levels (though I'm having a dickens of a time finding it now), so I suspect it is detectable.

Is the use of older versions something we want to allow? Should we be making Update 3 the minimum supported version? Feel free to WONTFIX this bug if the answer is no :)
Flags: needinfo?(dmajor)
(Assignee)

Comment 1

2 years ago
Unless it's going to cause code not to compile I don't think we should bother.
Update 2 fixes a PGO linker crash and at least one mochitest failure (bug 929834 comes to mind). 

As of a couple of months ago we didn't depend on any bug fixes in Update 3, but who knows, we may inadvertently introduce some dependencies moving forward.

So I guess it's a tradeoff between "help I don't understand this failure" and "the build is mostly fine, don't hassle me". I'll leave that for y'all to decide!
Flags: needinfo?(dmajor)
(Reporter)

Comment 3

2 years ago
Sounds like we need Update 2 at a bare minimum then! Personally, I think we should just go with Update 3 at that point to stay in sync with our automation and avoid the hidden dep problem you mentioned.
If the only problem we know of that is fixed by an update is that PGO linker crash, then it's not worth. Almost nobody does PGO builds. I don't see the point of forcing an update for a bug that affects virtually nobody.
(Reporter)

Comment 5

2 years ago
What about the mochitest issue? Also, FWIW, the community edition comes with Update 4 out of the box.
(Reporter)

Comment 6

2 years ago
But if the mochitest issue isn't enough, then I'm fine with just WONTFIXing this. I just want us to get our ducks in a row for all of this before Gecko 37 merges to Aurora.
I think it would be good to require at least update 3. It is *very* easy for a VS2013 user to update. If you use the IDE, it actually bothers you about doing the update. Thus, updating is unlikely to be a burden for anybody.

Conversely, if we were to (accidentally) add a silent dependency on VS2013 update 3, it would likely be *very* confusing, hard to debug, and time consuming (for the affected contributor, as well as the people helping him) to figure out what is going on and rectify the situation.

RE: comment 4, it's terrible to wait until we hit an overt problem to make these decisions, because we end up wasting quite a few peoples' time. Solving these problems preemptively will pay off in the long term. (Taking this further, in a separate bug we should move to make VS2013 Update 4 the minimum version, because that's the version more and more people will be using soon, since that's what VS2013 Community Edition has.)
Alright, given Ryan's and Brian's points, I think we should do this. Can someone with the necessary build-fu help out?

The magic number for Update 3 in CL's version string is: 18.00.30723
Or in other words, an _MSC_FULL_VER of 180030723.
(Reporter)

Comment 9

2 years ago
configure.in has a _CC_MINOR_VERSION detection in it, so presumably we can use that. I think I can take this one.
Assignee: nobody → ryanvm
Status: NEW → ASSIGNED
I believe _CC_MINOR_VERSION contains the "00" piece, so there may be some extra work to get the actual build number.
Indeed, _CC_MINOR_VERSION is the "00" piece.
(Assignee)

Comment 12

2 years ago
Right now we only use _MSC_VER to get CC_VERSION:
https://hg.mozilla.org/mozilla-central/annotate/33781a3a5201/build/autoconf/toolchain.m4#l18

so it's always XXYY, where the compiler version is XX.YY.ZZZZ.

You could swap that out for _MSC_FULL_VER, which would give you XXYYZZZZ, and then do:
_CC_BUILD_VERSION=`echo ${CC_VERSION} | cut -c 5-`

I'm not sure if adding more digits to CC_VERSION would break anything else, you could always just reset it immediately after by doing:
CC_VERSION=${_CC_MAJOR_VERSION}${_CC_MINOR_VERSION}
(Assignee)

Updated

2 years ago
Assignee: ryanvm → ted
(Assignee)

Comment 13

2 years ago
Created attachment 8545301 [details] [diff] [review]
require-vc2013-update3

This errors on my local machine where I didn't get around to installing update 3.
Attachment #8545301 - Flags: review?(mh+mozilla)
(Assignee)

Comment 14

2 years ago
jimm tested this patch on his machine with update 4 and said it configures successfully.
Comment on attachment 8545301 [details] [diff] [review]
require-vc2013-update3

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

::: build/autoconf/toolchain.m4
@@ +84,5 @@
>      # is based on the CLANG_CL variable defined here, so make sure that we're
>      # getting the right version here manually.
>      CC_VERSION=1800
>      CXX_VERSION=1800
> +    MSVC_FULL_VERSION=180030723

Did you mean MSVC_VERSION_FULL?
Comment on attachment 8545301 [details] [diff] [review]
require-vc2013-update3

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

::: build/autoconf/toolchain.m4
@@ +58,5 @@
> +
> +if test "$compiler" = "msvc"; then
> +     MSVC_VERSION_FULL="$CXX_VERSION"
> +     CC_VERSION=`echo ${CC_VERSION} | cut -c 1-4`
> +     CXX_VERSION=`echo ${CC_VERSION} | cut -c 1-4`

${CXX_VERSION}

@@ +84,5 @@
>      # is based on the CLANG_CL variable defined here, so make sure that we're
>      # getting the right version here manually.
>      CC_VERSION=1800
>      CXX_VERSION=1800
> +    MSVC_FULL_VERSION=180030723

MSVC_VERSION_FULL

::: configure.in
@@ +474,5 @@
>          changequote([,])
>  
>          # Determine compiler version
>          _CC_MAJOR_VERSION=`echo ${CC_VERSION} | cut -c 1-2`
>          _CC_MINOR_VERSION=`echo ${CC_VERSION} | cut -c 3-4`

Maybe use MSVC_VERSION_FULL here too.

@@ +487,5 @@
>  
>          AC_DEFINE(_CRT_SECURE_NO_WARNINGS)
>          AC_DEFINE(_CRT_NONSTDC_NO_WARNINGS)
>  
> +        if test "$_CC_MAJOR_VERSION" = "18" -a "$_CC_BUILD_VERSION" -ge "30723"; then

So, according to http://en.wikipedia.org/wiki/Visual_C%2B%2B, 2013 RTM version is 18.0.21005.1. If that comes out in MSVC_VERSION_FULL as 1800210051, then this test will not work. That said, it looks like from a cursory google search that it's 180021005, although interestingly, intel C++ compiler uses 1800210051 in its msvc 2013 compatibility mode, so I don't know where they got that from (https://software.intel.com/en-us/node/524490).

It would be better to double check with an actual 2013 RTM install.

@@ +493,5 @@
>              MSVS_VERSION=2013
>              MSVC_C_RUNTIME_DLL=msvcr120.dll
>              MSVC_CXX_RUNTIME_DLL=msvcp120.dll
>          else
> +            AC_MSG_ERROR([This version ($CC_VERSION) of the MSVC compiler is unsupported.

Might be better to show MSVC_VERSION_FULL here, in a xx.xx.xxxxx format, so, in fact $_CC_MAJOR_VERSION.$_CC_MINOR_VERSION.$_CC_BUILD_VERSION.
Attachment #8545301 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 17

2 years ago
(In reply to Mike Hommey [:glandium] from comment #16)
> So, according to http://en.wikipedia.org/wiki/Visual_C%2B%2B, 2013 RTM
> version is 18.0.21005.1. If that comes out in MSVC_VERSION_FULL as
> 1800210051, then this test will not work. That said, it looks like from a
> cursory google search that it's 180021005, although interestingly, intel C++
> compiler uses 1800210051 in its msvc 2013 compatibility mode, so I don't
> know where they got that from (https://software.intel.com/en-us/node/524490).
> 
> It would be better to double check with an actual 2013 RTM install.

I actually had RTM installed when I wrote this patch and it doesn't include the final digit. The documentation confirms this:
http://msdn.microsoft.com/en-us/library/b0084kay.aspx
"Evaluates to an integer literal that encodes the major, minor, and build number components of the compiler's version number. The major number is the first component of the period-delimited version number, the minor number is the second component, and the build number is the third component. For example, if the version number of the Visual C++ compiler is 15.00.20706.01, the _MSC_FULL_VER macro evaluates to 150020706."
(Assignee)

Comment 18

2 years ago
Created attachment 8545901 [details] [diff] [review]
require-vc2013-update3
Attachment #8545901 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Reporter)

Updated

2 years ago
Attachment #8545301 - Attachment is obsolete: true
(Reporter)

Comment 19

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b60d1a95bfa3
Keywords: checkin-needed

Updated

2 years ago
Blocks: 1119225
(Reporter)

Comment 20

2 years ago
Just disabled Windows build slave #2 for running the wrong version of MSVC2013. Apparently we have some on version 18.00.30220 still.
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5208839&repo=mozilla-inbound
(Reporter)

Comment 21

2 years ago
Which according to bug 1057229 comment 5, is...something...
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #20)
> Just disabled Windows build slave #2 for running the wrong version of
> MSVC2013. Apparently we have some on version 18.00.30220 still.
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=5208839&repo=mozilla-inbound

#3
https://treeherder.mozilla.org/logviewer.html#?job_id=5222357&repo=mozilla-inbound
https://hg.mozilla.org/mozilla-central/rev/b60d1a95bfa3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.