Bug 20380

Summary: [GTK][AUTOTOOLS] Include autotoolsconfig.h from config.h
Product: WebKit Reporter: Marco Barisione <marco.barisione>
Component: WebKitGTKAssignee: Marco Barisione <marco.barisione>
Status: RESOLVED FIXED    
Severity: Normal CC: alp, jmalonzo, marco.barisione
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Include aconfig.h if available
eric: review-
Include the autotools configuration header if available eric: review+

Description Marco Barisione 2008-08-14 04:07:12 PDT
Projects using autotools usually have an auto-generated config.h file containing various defines on the functions and libraries available. WebKit already contained a config.h file in WebCore, so the config file is called aconfig.h instead of config.h but this file is never included.
Comment 1 Marco Barisione 2008-08-14 05:13:22 PDT
Created attachment 22789 [details]
Include aconfig.h if available
Comment 2 Alp Toker 2008-08-16 14:31:48 PDT
Comment on attachment 22789 [details]
Include aconfig.h if available

>diff --git a/JavaScriptCore/ChangeLog b/JavaScriptCore/ChangeLog
>index b0f0978..99285df 100644
>--- a/JavaScriptCore/ChangeLog
>+++ b/JavaScriptCore/ChangeLog
>@@ -1,3 +1,13 @@
>+2008-08-14  Marco Barisione  <marco.barisione@collabora.co.uk>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        http://bugs.webkit.org/show_bug.cgi?id=20380
>+        [GTK][AUTOTOOLS] Include aconfig.h from config.h
>+
>+        * kjs/config.h: include the aconfig.h generated by autotools if
>+        available
>+
> 2008-08-13  Geoffrey Garen  <ggaren@apple.com>
> 
>         Reviewed by Oliver Hunt.
>diff --git a/JavaScriptCore/kjs/config.h b/JavaScriptCore/kjs/config.h
>index f87b664..e4a6792 100644
>--- a/JavaScriptCore/kjs/config.h
>+++ b/JavaScriptCore/kjs/config.h
>@@ -21,6 +21,10 @@
> 
> #include <wtf/Platform.h>
> 
>+#if PLATFORM(GTK) && defined(HAVE_CONFIG_H)
>+#include "aconfig.h"
>+#endif
>+

Here and in other places, aconfig.h is included after wtf/Platform.h in the patch.

wtf/Platform.h tests for various configurable flags that we currently define using compiler arguments, but that we want to define in aconfig.h in future, eg:

#if !defined(ENABLE_DATABASE)
#define ENABLE_DATABASE 1
#endif

So for that to work, aconfig.h needs to be included at the very top of the config.h files as far as I can tell, rather than mid-way as this patch does. Any thoughts on that?
Comment 3 Marco Barisione 2008-08-17 05:02:25 PDT
(In reply to comment #2)
> So for that to work, aconfig.h needs to be included at the very top of the
> config.h files as far as I can tell, rather than mid-way as this patch does.
> Any thoughts on that?

But the PLATFORM macro is defined in Platform.h, so if we want to ue it we need to include Platform.h. Do you think that other ports could be defining a HAVE_CONFIG_H macro? If not we could just remove the check for the platform, or we could check for "defined(WTF_PLATFORM_GTK) && WTF_PLATFORM_GTK".

Comment 4 Eric Seidel (no email) 2008-08-25 21:07:15 PDT
Comment on attachment 22789 [details]
Include aconfig.h if available

JavaScriptGlue is a mac-only project, I don't see why it would need any changes to config.h

Why is aconfig.h needed?  Could it have a better name too?  Like autoconf_config.h or similar?

The Mac and Windows ports pass various ENABLE_ defines via compiler flags.  I would assume that the Automake system would do the same, but I guess you're suggesting that the automake way is to pass these ENABLE_* defines as part of aconfig.h?

Marking r- and waiting for further clarification.
Comment 5 Marco Barisione 2008-09-03 04:08:04 PDT
(In reply to comment #4)
> JavaScriptGlue is a mac-only project, I don't see why it would need any changes
> to config.h

Ok, I will remove it.

> Why is aconfig.h needed?  Could it have a better name too?  Like
> autoconf_config.h or similar?

THe standard name for it is config.h, probably Jan is the one who chose the aconfig.h name as config.h was already taken. Jan what do you think about the rename?

> The Mac and Windows ports pass various ENABLE_ defines via compiler flags.  I
> would assume that the Automake system would do the same, but I guess you're
> suggesting that the automake way is to pass these ENABLE_* defines as part of
> aconfig.h?

Yes.
Comment 6 Jan Alonzo 2008-09-03 04:24:33 PDT
(In reply to comment #5)
> 
> > Why is aconfig.h needed?  Could it have a better name too?  Like
> > autoconf_config.h or similar?
> 
> THe standard name for it is config.h, probably Jan is the one who chose the
> aconfig.h name as config.h was already taken. Jan what do you think about the
> rename?

Please rename it to something like gtkconfig.h or autotools_config.h.

Comment 7 Marco Barisione 2008-09-03 06:54:14 PDT
Created attachment 23141 [details]
Include the autotools configuration header if available

- Renamed aconfig.h to autotoolsconfig.h
- Include the header only in WebCore and JSCore
- Include the header before Platform.h
- Donn't check if the platform is GTK as PLATFORM is defined in Platform.h and ohter ports don't define HAVE_CONFIG_H
Comment 8 Eric Seidel (no email) 2008-09-03 13:30:00 PDT
Comment on attachment 23141 [details]
Include the autotools configuration header if available

Seems sane enough.
Comment 9 Jan Alonzo 2008-09-04 13:26:47 PDT
landed in r36098