Bug 109932

Summary: [GTK] Limit the supported compilers to GCC >= 4.7 and Clang >= 3.0
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, gns, mrobinson, pnormand
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Provisional patch
none
Patch mrobinson: review+

Description Zan Dobersek 2013-02-15 06:01:24 PST
[GTK] Limit the supported compilers to GCC >= 4.7 and Clang >= 3.0
Comment 1 Zan Dobersek 2013-02-15 06:22:28 PST
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
Comment 2 Zan Dobersek 2013-02-15 06:26:15 PST
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.
Comment 3 Martin Robinson 2013-02-15 09:11:40 PST
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
Comment 4 Zan Dobersek 2013-02-15 11:53:13 PST
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.
Comment 5 Martin Robinson 2013-02-15 12:00:11 PST
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. :)
Comment 6 Zan Dobersek 2013-02-15 12:14:51 PST
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.
Comment 7 Zan Dobersek 2013-02-15 12:30:26 PST
Created attachment 188618 [details]
Patch
Comment 8 Gustavo Noronha (kov) 2013-03-06 09:14:45 PST
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.
Comment 9 Martin Robinson 2013-03-06 09:18:34 PST
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.
Comment 10 Zan Dobersek 2013-03-06 09:42:05 PST
(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?
Comment 11 Martin Robinson 2013-03-06 09:44:33 PST
(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.
Comment 12 Carlos Garcia Campos 2013-03-06 09:45:22 PST
(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?
Comment 13 Martin Robinson 2013-03-06 09:48:40 PST
(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.
Comment 14 Carlos Garcia Campos 2013-03-06 09:52:35 PST
(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?
Comment 15 Zan Dobersek 2013-03-06 11:43:00 PST
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.
Comment 16 Zan Dobersek 2013-03-06 13:12:28 PST
Waiting for nods from both Carlos and Gustavo on comment #10 before landing this.
Comment 17 Carlos Garcia Campos 2013-03-06 13:32:26 PST
(In reply to comment #16)
> Waiting for nods from both Carlos and Gustavo on comment #10 before landing this.

nod
Comment 18 Zan Dobersek 2013-03-07 08:39:20 PST
(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/
Comment 19 Zan Dobersek 2013-03-07 09:01:36 PST
Added changelogs (dunno why they weren't there in the first place) and landed in r145084.
http://trac.webkit.org/changeset/145084