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
:
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 | Review
part 1 wip: Whitelist approach (1.83 KB, patch)
2011-08-21 00:03 PDT, Matheus Kerschbaum
ted: feedback-
Details | Diff | 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 | 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 | 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 | Review

Description 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 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 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 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 Mike Hommey [:glandium] 2011-08-20 23:58:04 PDT
I mean removing all the cruft and start over.
Comment 5 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 Mike Hommey [:glandium] 2011-08-20 23:58:44 PDT
And we should do that in a separate .m4 file.
Comment 7 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 :Ms2ger 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 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 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 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 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 Justin Wood (:Callek) 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 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 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 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 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 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 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.