WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
109932
[GTK] Limit the supported compilers to GCC >= 4.7 and Clang >= 3.0
https://bugs.webkit.org/show_bug.cgi?id=109932
Summary
[GTK] Limit the supported compilers to GCC >= 4.7 and Clang >= 3.0
Zan Dobersek
Reported
2013-02-15 06:01:24 PST
[GTK] Limit the supported compilers to GCC >= 4.7 and Clang >= 3.0
Attachments
Provisional patch
(9.64 KB, patch)
2013-02-15 06:26 PST
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(9.75 KB, patch)
2013-02-15 12:30 PST
,
Zan Dobersek
mrobinson
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Zan Dobersek
Comment 1
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
Zan Dobersek
Comment 2
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.
Martin Robinson
Comment 3
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
Zan Dobersek
Comment 4
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.
Martin Robinson
Comment 5
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. :)
Zan Dobersek
Comment 6
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.
Zan Dobersek
Comment 7
2013-02-15 12:30:26 PST
Created
attachment 188618
[details]
Patch
Gustavo Noronha (kov)
Comment 8
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.
Martin Robinson
Comment 9
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.
Zan Dobersek
Comment 10
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?
Martin Robinson
Comment 11
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.
Carlos Garcia Campos
Comment 12
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?
Martin Robinson
Comment 13
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.
Carlos Garcia Campos
Comment 14
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?
Zan Dobersek
Comment 15
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.
Zan Dobersek
Comment 16
2013-03-06 13:12:28 PST
Waiting for nods from both Carlos and Gustavo on
comment #10
before landing this.
Carlos Garcia Campos
Comment 17
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
Zan Dobersek
Comment 18
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/
Zan Dobersek
Comment 19
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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug