Closed Bug 1265460 Opened 4 years ago Closed 4 years ago

Require configure flag to use VS2013

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1186064

People

(Reporter: gps, Unassigned)

References

Details

Attachments

(1 obsolete file)

Our automation is using VS2015 now. We still soft support VS2013 but there isn't any automation coverage to ensure it still builds on mozilla-central.

Let's add a configure check that aborts when <VS2015u1 is used. Let's create a backdoor so people can define an environment variable or flag to override the check. This allows people to still build on VS2013 and to upgrade on their terms (instead of getting blocked when they pull down the tree).
We don't have automation coverage of VS2013 any more. We want developers
to be using the same toolchain we use in automation.

This patch prods people towards VS2015 by requiring the newly-introduced
--enable-legacy-toolchain flag to build with VS2013.

Yes, the version checking should live in toolchain.configure. However,
the existing code is in old-configure.in and I didn't want to bloat
scope by porting the code to toolchain.configure.

Review commit: https://reviewboard.mozilla.org/r/47201/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47201/
Attachment #8742430 - Flags: review?(ted)
Comment on attachment 8742430 [details]
MozReview Request: Bug 1265460 - Don't allow building with VS2013 unless a configure argument is present; r?ted

https://reviewboard.mozilla.org/r/47201/#review43887

I'm not sure I really like this idea. I understand the motivation, to let people know they're building with something that we're not building in CI, but there are lots of other ways to do a non-standard build that might break, and I don't know that I want to chase that long tail. People are much more likely to wind up with a broken build from `--disable-crashreporter`, which is wildly popular. Should we warn on that?

::: build/moz.configure/toolchain.configure:154
(Diff revision 1)
>  # Compilers
>  # ==============================================================
> +option('--enable-legacy-toolchain', env='MOZ_ENABLE_LEGACY_TOOLCHAIN',
> +       help='Allow building with legacy toolchains')
> +
> +@depends('--enable-legacy-toolchain')

I don't think you actually need this @depends here.

::: old-configure.in:367
(Diff revision 1)
>          AC_DEFINE(_CRT_SECURE_NO_WARNINGS)
>          AC_DEFINE(_CRT_NONSTDC_NO_WARNINGS)
>          AC_DEFINE(_USE_MATH_DEFINES) # Otherwise MSVC's math.h doesn't #define M_PI.
>  
>          if test "$_CC_MAJOR_VERSION" = "18" -a "$_CC_BUILD_VERSION" -ge "30723"; then
> +            if [ -z "$MOZ_ENABLE_LEGACY_TOOLCHAIN" ]; then

You're not using `add_old_configure_assignment` on this value, so this won't work.
Attachment #8742430 - Flags: review?(ted)
I agree with Ted. I understand the sentiment, but this is not doing anything useful IMHO. People who want to upgrade on their terms will add the flag, and have their build broken regularly exactly the same way they would have without having to add the flag.

The only thing this might achieve is having more people upgrade to 2015, but the builds breaking regularly on 2013 can do that just the same.

Seeing how regularly those builds break, we *really* should have 2013 tested on automation. Why not make e.g. the debug builds use 2013?
Why don't you just fix bug 1186064? VC2013 builds already start breaking (bug 1265154, bug 1265158).
(In reply to Masatoshi Kimura [:emk] from comment #4)
> Why don't you just fix bug 1186064? VC2013 builds already start breaking
> (bug 1265154, bug 1265158).

Some combination of {ehsan, froydnj, glandium, ted} insisted we maintain support for VC2013 for a few cycles. I believe this was a #build conversation. Their reasons sounded legit.
It is a common theme that we want to prod/notify people about things that make their build/dev experience sub-optimal only we don't have a good mechanism to do this. It is really frustrating because I can't help but feel that a lot of people get tripped up by preventable issues. I understand people don't want to get interrupted by tools insisting they do things. But at the same time, if we can inform people something is wrong, I think that will save more time in the end.

Perhaps we can add a warning to the post build output. We've done this before for things like high Finder CPU usage. Although, as Ted says, there is a long tail of non-standard builds. Do we want to prompt for them?
Tip doesn't build on 2013 anymore, which just wasted 70 minutes of compiling time for me today.
Then this patch will waste even more time. VS2013 won't build anyway even if the flag is set.
If people have local patches to build Firefox on VS2013 locally, they can just add old-configure.in (or moz.configure) changes to their local patches.
I don't understand how this patch is useful at all.
This isn't needed since we now require VS2015 to build.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1186064
Attachment #8742430 - Attachment is obsolete: true
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.