Bug 109932 – [GTK] Limit the supported compilers to GCC >= 4.7 and Clang >= 3.0

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[GTK] Limit the supported compilers to GCC >= 4.7 and Clang >= 3.0

[Zan Dobersek]

Given that more and more C++11 features will be coming into WebKit2[1] I think it would make sense to support that by compiling WebKit2 with C++11 support enabled. At the moment this limits us to choose between two compilers - GCC >= 4.7 and Clang >= 3.0.

Given the near stable release branching I'd like to see this get included, the reasoning being that it would probably ease merging fixes in WebKit2 into the stable branch. I'm not sure though how this would pass through with packagers and distributors. Could such an abrupt and late decision cause problems for them?

The developers on Windows (based on the information I've gathered so far) can use GCC 4.7 with C++11 support. I'd still like to double-check with them though.

[1] https://lists.webkit.org/pipermail/webkit-dev/2013-January/023527.html

[Zan Dobersek]

Created attachment 188549 [details]
Provisional patch

Just a provisional patch, I'd like to first clear things up regarding the possibility of already restricting for the stable release.

[Martin Robinson]

Comment on attachment 188549 [details]
Provisional patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188549&action=review

I think this is a pretty good decision. I guess it deserves a mail to the list though.

> Source/autotools/CheckSystemAndBasicDependencies.m4:96
> +# Check that an appropriate C compiler is available.
> +AC_LANG_PUSH([C])
> +AC_COMPILE_IFELSE([AC_LANG_PROGRAM([
> +#if !(defined(__GNUC__) && !defined(__clang__) && __GNUC__ >= 4 && __GNUC_MINOR__ >= 7) \
> +    && !(defined(__clang__) && __clang_major__ >= 3 && __clang_minor__ >= 0)
> +#error Unsupported compiler
> +#endif
> +],[])],[],[AC_MSG_ERROR([Compiler GCC >= 4.7 or Clang >= 3.0 is required for C compilation])])
> +AC_LANG_POP([C])

Hrm. There's really no macro for checking via the command-line?

> Source/autotools/CheckSystemAndBasicDependencies.m4:111
> +# Use C99 as the language standard for C code.
> +CFLAGS="$CFLAGS -std=c99"
> +# Do not warn about C++11 incompatibilities and extensions.
> +CXXFLAGS="$CXXFLAGS -Wno-c++11-compat -Wno-c++11-extensions"

See below.

> Source/autotools/CheckSystemAndBasicDependencies.m4:127
> +# Clang requires suppression of unused arguments warning.
> +if test "$CC" = "clang"; then
> +    CFLAGS="$CFLAGS -Qunused-arguments"
> +fi
> +# libstdc++ is at the moment the only option as the C++ standard library.
> +if test "$CXX" = "clang++"; then

Probably good to move these to SetupCompilerFlags.m4

[Zan Dobersek]

Comment on attachment 188549 [details]
Provisional patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188549&action=review

>> Source/autotools/CheckSystemAndBasicDependencies.m4:96
>> +AC_LANG_POP([C])
> 
> Hrm. There's really no macro for checking via the command-line?

What kind of check that would be performed via command line did you have in mind?

>> Source/autotools/CheckSystemAndBasicDependencies.m4:127
>> +if test "$CXX" = "clang++"; then
> 
> Probably good to move these to SetupCompilerFlags.m4

Yup.

[Martin Robinson]

Comment on attachment 188549 [details]
Provisional patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188549&action=review

>>> Source/autotools/CheckSystemAndBasicDependencies.m4:96
>>> +AC_LANG_POP([C])
>> 
>> Hrm. There's really no macro for checking via the command-line?
> 
> What kind of check that would be performed via command line did you have in mind?

It seems there are some autoconf macros that depend on the -dumpversion flag, but I don't really know if that's better or worse than this. :)

[Zan Dobersek]

Comment on attachment 188549 [details]
Provisional patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188549&action=review

>>>> Source/autotools/CheckSystemAndBasicDependencies.m4:96
>>>> +AC_LANG_POP([C])
>>> 
>>> Hrm. There's really no macro for checking via the command-line?
>> 
>> What kind of check that would be performed via command line did you have in mind?
> 
> It seems there are some autoconf macros that depend on the -dumpversion flag, but I don't really know if that's better or worse than this. :)

Looking only at the output it seems this simply prints out `__GNUC__.__GNUC_MINOR__` in gcc/g++ and `__GNUC__.__GNUC_MINOR__.__GNUC_PATCHLEVEL__` in clang/clang++. The problem is that Clang compilers define those macros as well, for compatibility, but with Clang 3.0 the output is 4.2.1 which is not helpful. That's why the AC_COMPILE_IFELSE macros work on code that strictly rules out Clang in case of a defined __GNUC__ macro.

Note that the Intel compilers do the same thing so I'll add a check to rule them out as well.
http://nadeausoftware.com/articles/2012/10/c_c_tip_how_detect_compiler_name_and_version_using_compiler_predefined_macros

> Source/autotools/CheckSystemAndBasicDependencies.m4:101
> +#if !(defined(__GNUC__) && !defined(__clang__) && __GNUC__ >= 4 && __GNUC_MINOR__ >= 7) \

I'll add a further define check for the __GNUG__ macro which should be defined when compiling C++ with GCC. Just in case, really, but I'd like to be thorough here.

[Zan Dobersek]

Created attachment 188618 [details]
Patch

[Gustavo Noronha (kov)]

I approve of this change with both my webkitgtk and debian hats on. gcc 4.7 will be in the next stable release for debian, which is the only one we should care about from now on anyway, so green light from me.

[Martin Robinson]

Comment on attachment 188618 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188618&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

You'll need to remove this line.

[Zan Dobersek]

(In reply to comment #8)
> I approve of this change with both my webkitgtk and debian hats on. gcc 4.7 will be in the next stable release for debian, which is the only one we should care about from now on anyway, so green light from me.

Thanks! Just to clarify the stable release reference, is this OK to merge into the webkit-2.0 branch?

[Martin Robinson]

(In reply to comment #10)
> (In reply to comment #8)
> > I approve of this change with both my webkitgtk and debian hats on. gcc 4.7 will be in the next stable release for debian, which is the only one we should care about from now on anyway, so green light from me.
> 
> Thanks! Just to clarify the stable release reference, is this OK to merge into the webkit-2.0 branch?

I think we should do it.

[Carlos Garcia Campos]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > I approve of this change with both my webkitgtk and debian hats on. gcc 4.7 will be in the next stable release for debian, which is the only one we should care about from now on anyway, so green light from me.
> > 
> > Thanks! Just to clarify the stable release reference, is this OK to merge into the webkit-2.0 branch?
> 
> I think we should do it.

why?

[Martin Robinson]

(In reply to comment #12)
 
> > I think we should do it.
> 
> why?

If we did this at 2.2 we'd be raising our dependency requirements before 3.0, so doing it for 2.0 means we don't have to wait until 3.0.

[Carlos Garcia Campos]

(In reply to comment #13)
> (In reply to comment #12)
> 
> > > I think we should do it.
> > 
> > why?
> 
> If we did this at 2.2 we'd be raising our dependency requirements before 3.0, so doing it for 2.0 means we don't have to wait until 3.0.

Is there any rule that doesn't allow us to bump requirements until we release a 3.0?

[Zan Dobersek]

I reason the merging with the idea that since more and more WebKit2 (and someday WebCore as well) code will use C++11 goodness, along with the fact the 2.0 release series will be (AFAIK) supported in the long term, this would ease merging of future patches (security fixes etc.) into the webkit-2.0 branch.

[Zan Dobersek]

Waiting for nods from both Carlos and Gustavo on comment #10 before landing this.

[Carlos Garcia Campos]

(In reply to comment #16)
> Waiting for nods from both Carlos and Gustavo on comment #10 before landing this.

nod

[Zan Dobersek]

(In reply to comment #16)
> Waiting for nods from both Carlos and Gustavo on comment #10 before landing this.

via IRC:
(05:00:44 PM) kov: zdobersek, yeah, I'm OK with pushing that to webkit-2.0

\o/

[Zan Dobersek]

Added changelogs (dunno why they weren't there in the first place) and landed in r145084.
http://trac.webkit.org/changeset/145084