Bug 20380 – [GTK][AUTOTOOLS] Include autotoolsconfig.h from config.h

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Marco Barisione]

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.

[Marco Barisione]

Created attachment 22789 [details]
Include aconfig.h if available

[Alp Toker]

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 @@
>[phone]  Marco Barisione  <[e-mail]>
>+
>+        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  <[e-mail]>
> 
>         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?

[Marco Barisione]

(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".

[Eric Seidel (no email)]

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.

[Marco Barisione]

(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.

[Jan Alonzo]

(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.

[Marco Barisione]

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

[Eric Seidel (no email)]

Comment on attachment 23141 [details]
Include the autotools configuration header if available

Seems sane enough.

[Jan Alonzo]

landed in r36098