680625 - Refactor configure's compiler checks

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Matheus Kerschbaum]

Currently configure has a lot of code for figuring out if a compiler supports C++ features required to build Firefox (i.e http://mxr.mozilla.org/mozilla-central/source/configure.in#8056) and as a result we end up with a lot of crap that hasn't done anything useful for over 10 years (i.e http://mxr.mozilla.org/mozilla-central/source/configure.in#4063) because no one knows if it's safe to remove them.

Thus I propose we switch configure to a whitelist of known good compilers and simply bail-out early if the user's compiler doesn't match the whitelist.

The bad side of this change is that we'll probably (unintentionally) break tier 3 platforms in the process and supporting new compilers will necessarily require changing configure, where previously it was possible (although unlikely) to get away with no changes.

The good side is that this will be a substantial shrinkage of configure, and that when deciding to no longer support a compiler we can just change one line of code and error out in configure rather than fail when building a random file.

Comment 1

[(not currently active) Ted Mielczarek]

I don't think a whitelist of compilers is the right approach here. People do regularly build Firefox with some compilers that you wouldn't think of, like xlC for AIX and aCC for HP-UX.

I would support blacklisting some compilers, like GCC < 4.2. We could also probably remove a bunch of the really outdated checks, since people are unlikely to be using compilers that fail those checks.

Comment 2

[Mike Hommey [:glandium]]

I don't think a white list of compilers is the right approach either, but I do agree we have too much cruft around. I also think it's hard, if not impossible, to remove anything if we want to keep things building for those people building with sun studio, xlC, aCC, icc, etc.

So I think we should remove all compiler/toolchain related stuff. All of it. Then add stuff to manage all Mozilla builds with. And then get those people that do these builds with esoteric toolchains to add their own bits. I can at least point to those for Sun Studio and OS/2, I think I also may be able to find in my mailbox who to contact for AIX.

Comment 3

[Matheus Kerschbaum]

(In reply to Ted Mielczarek [:ted, :luser] from comment #1)
> I don't think a whitelist of compilers is the right approach here. People do
> regularly build Firefox with some compilers that you wouldn't think of, like
> xlC for AIX and aCC for HP-UX.

I don't understand your concern here. If we know people might use those compilers, can't we whitelist them as well?

(In reply to Mike Hommey [:glandium] from comment #2)
> So I think we should remove all compiler/toolchain related stuff. All of it.

Do you mean removing them from configure or removing it altogether? We still need to workaround a few differences between MSVC and GCC as well as bugs in those compilers (i.e http://mxr.mozilla.org/mozilla-central/source/configure.in#7226).

Comment 4

[Mike Hommey [:glandium]]

I mean removing all the cruft and start over.

Comment 5

[Matheus Kerschbaum]

Attachment: part 0: Remove checks and workarounds for ancient compilers.

Comment 6

[Mike Hommey [:glandium]]

And we should do that in a separate .m4 file.

Comment 7

[Matheus Kerschbaum]

Attachment: part 1 wip: Whitelist approach

This is basically the whitelist approach I was considering. Right now the only supported compilers are msvc2005, 2008 and 2010 on Windows.

There's also a --disable-compiler-whitelist flag for people who want to bypass the whitelist to test a new version of a compiler or simply because we forgot to whitelist a very specific compiler in a tier 3 platform.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 554699 [details] [diff] [review]
part 0: Remove checks and workarounds for ancient compilers.

Want to get rid of these STATIC_CAST and REINTERPRET_CAST macros too?

Comment 9

[Matheus Kerschbaum]

Attachment: part 0b: Remove more checks and workarounds for ancient compilers

Comment 10

[Benjamin Smedberg]

I really don't think we should have a whitelist. We should either have no checks, or just get rid of the old checks that don't make sense any more, but sanity-checking compilers isn't a bad thing in general.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 554699 [details] [diff] [review]
part 0: Remove checks and workarounds for ancient compilers.

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

I think these should all be safe to remove. They're pretty ancient checks.

::: xpcom/tests/TestCOMPtr.cpp
@@ +41,5 @@
>  #include "nsCOMPtr.h"
>  #include "nsISupports.h"
>  
> +#define STATIC_CAST(T,x)  static_cast<T>(x)
> +#define REINTERPRET_CAST(T,x) reinterpret_cast<T>(x)

Can you just replace STATIC_CAST(T,x) in these files with static_cast<T>(x)? (And similarly for REINTERPRET.)

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 554759 [details] [diff] [review]
part 0b: Remove more checks and workarounds for ancient compilers

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

This looks fine, but I'm curious why the Html5RefPtr file has those IRIX hacks. Just a copy of nsCOMPtr code?

::: xpcom/glue/nsCOMPtr.h
@@ +122,5 @@
>  #if defined(NSCAP_DISABLE_DEBUG_PTR_TYPES)
>    #define NSCAP_FEATURE_USE_BASE
>  #endif
>  
> +typedef bool NSCAP_BOOL;

Can you just remove this and s/NSCAP_BOOL/bool/ in the rest of the file?

::: xpcom/string/public/nsCharTraits.h
@@ +76,5 @@
>    // for NS_ASSERTION
>  #endif
>  #endif
>  
> +typedef bool nsCharTraits_bool;

Same thing here.

Comment 13

[Justin Wood (:Callek)]

(In reply to Ted Mielczarek [:ted, :luser] from comment #12)
> ::: xpcom/glue/nsCOMPtr.h
> @@ +122,5 @@
> >  #if defined(NSCAP_DISABLE_DEBUG_PTR_TYPES)
> >    #define NSCAP_FEATURE_USE_BASE
> >  #endif
> >  
> > +typedef bool NSCAP_BOOL;
> 
> Can you just remove this and s/NSCAP_BOOL/bool/ in the rest of the file?

Warning this is also used in a few more files...

http://mxr.mozilla.org/comm-central/search?string=NSCAP_BOOL&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=comm-central

Comment 14

[Matheus Kerschbaum]

Attachment: part 0: Remove checks and workarounds for ancient compilers.

Addressed review comments.

Comment 15

[Matheus Kerschbaum]

Attachment: part 0b: Remove more checks and workarounds for ancient compilers

Addressed review comments.

Comment 16

[Ed Morley [:emorley]]

Part 0: http://hg.mozilla.org/projects/build-system/rev/706c0a4feec2
Part 0b: http://hg.mozilla.org/projects/build-system/rev/3d4b14bd24e3

Leaving open for remaining patch.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/706c0a4feec2
http://hg.mozilla.org/mozilla-central/rev/3d4b14bd24e3

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 554700 [details] [diff] [review]
part 1 wip: Whitelist approach

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

Yeah, I don't think we want to do this. Maintaining this list seems like more work than just maintaining the list of feature checks we care about. Removing really old checks (like the other patches in this bug) is the sensible path.