Last Comment Bug 680625 - Refactor configure's compiler checks
: Refactor configure's compiler checks
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
-- normal with 1 vote (vote)
: mozilla9
Assigned To: Matheus Kerschbaum
:
: Gregory Szorc [:gps] (away until 2017-03-20)
Mentors:
Depends on: 680792 680795
Blocks: 671465
  Show dependency treegraph
 
Reported: 2011-08-19 20:02 PDT by Matheus Kerschbaum
Modified: 2011-09-27 15:27 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 0: Remove checks and workarounds for ancient compilers. (31.40 KB, patch)
2011-08-20 23:58 PDT, Matheus Kerschbaum
ted: review+
Details | Diff | Splinter Review
part 1 wip: Whitelist approach (1.83 KB, patch)
2011-08-21 00:03 PDT, Matheus Kerschbaum
ted: feedback-
Details | Diff | Splinter Review
part 0b: Remove more checks and workarounds for ancient compilers (19.83 KB, patch)
2011-08-21 15:08 PDT, Matheus Kerschbaum
ted: review+
Details | Diff | Splinter Review
part 0: Remove checks and workarounds for ancient compilers. (42.11 KB, patch)
2011-08-25 16:20 PDT, Matheus Kerschbaum
matjk7: review+
emorley: checkin+
Details | Diff | Splinter Review
part 0b: Remove more checks and workarounds for ancient compilers (42.54 KB, patch)
2011-08-25 16:21 PDT, Matheus Kerschbaum
matjk7: review+
emorley: checkin+
Details | Diff | Splinter Review

Description User image Matheus Kerschbaum 2011-08-19 20:02:17 PDT
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 User image Ted Mielczarek [:ted.mielczarek] 2011-08-20 05:15:23 PDT
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 User image Mike Hommey [:glandium] 2011-08-20 05:41:27 PDT
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 User image Matheus Kerschbaum 2011-08-20 23:55:48 PDT
(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 User image Mike Hommey [:glandium] 2011-08-20 23:58:04 PDT
I mean removing all the cruft and start over.
Comment 5 User image Matheus Kerschbaum 2011-08-20 23:58:19 PDT
Created attachment 554699 [details] [diff] [review]
part 0: Remove checks and workarounds for ancient compilers.
Comment 6 User image Mike Hommey [:glandium] 2011-08-20 23:58:44 PDT
And we should do that in a separate .m4 file.
Comment 7 User image Matheus Kerschbaum 2011-08-21 00:03:03 PDT
Created attachment 554700 [details] [diff] [review]
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 User image :Ms2ger (⌚ UTC+1/+2) 2011-08-21 02:58:13 PDT
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 User image Matheus Kerschbaum 2011-08-21 15:08:50 PDT
Created attachment 554759 [details] [diff] [review]
part 0b: Remove more checks and workarounds for ancient compilers
Comment 10 User image Benjamin Smedberg [:bsmedberg] 2011-08-22 06:19:28 PDT
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 User image Ted Mielczarek [:ted.mielczarek] 2011-08-25 12:50:29 PDT
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 User image Ted Mielczarek [:ted.mielczarek] 2011-08-25 12:58:35 PDT
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 User image Justin Wood (:Callek) [away until Feb 27] 2011-08-25 13:01:01 PDT
(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 User image Matheus Kerschbaum 2011-08-25 16:20:58 PDT
Created attachment 555882 [details] [diff] [review]
part 0: Remove checks and workarounds for ancient compilers.

Addressed review comments.
Comment 15 User image Matheus Kerschbaum 2011-08-25 16:21:41 PDT
Created attachment 555883 [details] [diff] [review]
part 0b: Remove more checks and workarounds for ancient compilers

Addressed review comments.
Comment 16 User image Ed Morley [:emorley] 2011-08-25 16:44:02 PDT
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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-28 05:59:25 PDT
http://hg.mozilla.org/mozilla-central/rev/706c0a4feec2
http://hg.mozilla.org/mozilla-central/rev/3d4b14bd24e3
Comment 18 User image Ted Mielczarek [:ted.mielczarek] 2011-09-27 12:42:24 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.