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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

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 :)

Comment 1

[(not currently active) Ted Mielczarek]

Unless it's going to cause code not to compile I don't think we should bother.

Comment 2

[(Away)]

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!

Comment 3

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 4

[Mike Hommey [:glandium]]

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.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

What about the mochitest issue? Also, FWIW, the community edition comes with Update 4 out of the box.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 8

[(Away)]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

configure.in has a _CC_MINOR_VERSION detection in it, so presumably we can use that. I think I can take this one.

Comment 10

[(Away)]

I believe _CC_MINOR_VERSION contains the "00" piece, so there may be some extra work to get the actual build number.

Comment 11

[Mike Hommey [:glandium]]

Indeed, _CC_MINOR_VERSION is the "00" piece.

Comment 12

[(not currently active) Ted Mielczarek]

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}

Comment 13

[(not currently active) Ted Mielczarek]

Attachment: require-vc2013-update3

This errors on my local machine where I didn't get around to installing update 3.

Comment 14

[(not currently active) Ted Mielczarek]

jimm tested this patch on his machine with update 4 and said it configures successfully.

Comment 15

[(Away)]

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 16

[Mike Hommey [:glandium]]

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.

Comment 17

[(not currently active) Ted Mielczarek]

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

Comment 18

[(not currently active) Ted Mielczarek]

Attachment: require-vc2013-update3

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b60d1a95bfa3

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Which according to bug 1057229 comment 5, is...something...

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(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

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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