Refactor configure's compiler checks

RESOLVED FIXED in mozilla9

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Matheus Kerschbaum, Assigned: Matheus Kerschbaum)

Tracking

Trunk
mozilla9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.
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.
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.
(Assignee)

Comment 3

6 years ago
(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).
I mean removing all the cruft and start over.
(Assignee)

Comment 5

6 years ago
Created attachment 554699 [details] [diff] [review]
part 0: Remove checks and workarounds for ancient compilers.
Attachment #554699 - Flags: review?(ted.mielczarek)
And we should do that in a separate .m4 file.
(Assignee)

Comment 7

6 years ago
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.
Attachment #554700 - Flags: feedback?(ted.mielczarek)
Attachment #554700 - Flags: feedback?(mh+mozilla)
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?
(Assignee)

Updated

6 years ago
Depends on: 680792
(Assignee)

Updated

6 years ago
Depends on: 680795
(Assignee)

Comment 9

6 years ago
Created attachment 554759 [details] [diff] [review]
part 0b: Remove more checks and workarounds for ancient compilers
Attachment #554759 - Flags: review?(ted.mielczarek)
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.
(Assignee)

Updated

6 years ago
Blocks: 671465
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.)
Attachment #554699 - Flags: review?(ted.mielczarek) → review+
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.
Attachment #554759 - Flags: review?(ted.mielczarek) → review+
(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
(Assignee)

Comment 14

6 years ago
Created attachment 555882 [details] [diff] [review]
part 0: Remove checks and workarounds for ancient compilers.

Addressed review comments.
Attachment #554699 - Attachment is obsolete: true
Attachment #555882 - Flags: review+
Attachment #555882 - Flags: checkin?
(Assignee)

Comment 15

6 years ago
Created attachment 555883 [details] [diff] [review]
part 0b: Remove more checks and workarounds for ancient compilers

Addressed review comments.
Attachment #554759 - Attachment is obsolete: true
Attachment #555883 - Flags: review+
Attachment #555883 - Flags: checkin?
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
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.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: partially fixed-in-bs
Target Milestone: --- → mozilla9

Updated

6 years ago
Attachment #555882 - Flags: checkin? → checkin+

Updated

6 years ago
Attachment #555883 - Flags: checkin? → checkin+
http://hg.mozilla.org/mozilla-central/rev/706c0a4feec2
http://hg.mozilla.org/mozilla-central/rev/3d4b14bd24e3
Whiteboard: partially fixed-in-bs
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.
Attachment #554700 - Flags: feedback?(ted.mielczarek) → feedback-
(Assignee)

Updated

6 years ago
Attachment #554700 - Attachment is obsolete: true
Attachment #554700 - Flags: feedback?(mh+mozilla)
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.