Bug 25286 – [Gtk] Various autotools build refactoring and fixes

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Jan Alonzo]

I found a few issues with the current build scripts, particularly:

1. It adds -g even on a release build
2. Convenience libraries should be just convenience libraries. Remove all the _LIBADD stuff and add it libwebkit_LDFLAGS instead. 
3. Add glib to the list of "Requires" in the pkg-config file since we're directly linking to glib. Without this, programs who link to libwebkit but not using glib directly will be forced to link to glib since libwebkit requires it.
4. Not defining WTF_USE_JSC. Since we're using JSC, we need to define it (d'oh)
5. Refactor the scripts to add the only required libs, cppflags and cflags to the respective libraries (convenience and libwebkit). 
6. Add -no-fast-install and -no-install to uninstallable programs and unit tests
7. Missing yarr sources and ExecutableAllocatorFixedVMPool.cpp.

Most of the changes are moving stuff around. Also, I've noticed that libtool 2.2.6 is faster than DOLT. It might be a good idea to conditionalize DOLT in this case.

[Jan Alonzo]

Created attachment 29604 [details]
patch

Patch for the issues I raised above. I'll split this and fix the changelog and stuff later..

[Xan Lopez]

FWIW, I left the actual yarr code because it was experimental and we didn't want to use it yet. Had to add the headers so that we'd compile, but just that. Has the situation changed now?

[Jan Alonzo]

(In reply to comment #2)
> FWIW, I left the actual yarr code because it was experimental and we didn't
> want to use it yet. Had to add the headers so that we'd compile, but just that.
> Has the situation changed now?
> 

We should probably start using since I think it's already being used by mac (and it already compiles for us anyway). 

[Jan Alonzo]

Created attachment 30153 [details]
fixup uses of libadd, cflags, cxxflags and ldflags

This patch refactors our usage of CFLAGS, CXXFLAGS, LDFLAGS and LIBADD.

[Jan Alonzo]

Created attachment 30154 [details]
cleanup webcore, webkit, and jsc flags.

Refactor webcore, webkit and jsc flags.

[Jan Alonzo]

Created attachment 30155 [details]
add glib-2.0 in pkg-config

Add glib-2.0 in webkit-1.0.pc since we use it directly.

[Holger Freyther]

Comment on attachment 30153 [details]
fixup uses of libadd, cflags, cxxflags and ldflags


> Index: WebKit/JavaScriptCore/wtf/Platform.h
> ===================================================================
> --- WebKit.orig/JavaScriptCore/wtf/Platform.h	2009-05-09 19:21:11.000000000 +1000
> +++ WebKit/JavaScriptCore/wtf/Platform.h	2009-05-09 19:21:25.000000000 +1000
> @@ -337,6 +337,7 @@
>  #endif
>  
>  #if PLATFORM(GTK)
> +#define WTF_USE_JSC 1
>  #if HAVE(PTHREAD_H)
>  #define WTF_USE_PTHREADS 1
>  #endif
> Index: WebKit/ChangeLog

separate patch for this change please...

[Jan Alonzo]

Comment on attachment 30153 [details]
fixup uses of libadd, cflags, cxxflags and ldflags

Landed in r43562 (Platform.h change) and the rest in r43563.

Clearing review flag.

[Jan Alonzo]

Comment on attachment 30155 [details]
add glib-2.0 in pkg-config

Landed in r43564.

Clearing review flag.

[Jan Alonzo]

(In reply to comment #8)
> (From update of attachment 30153 [details] [review])
> Landed in r43562 (Platform.h change) and the rest in r43563.
> 
> Clearing review flag.
> 

r43562 was reverted in r43626 due to it being already defined in WebCore/config.h.

r43563 was reverted in r43623 as it caused a segfault when compiled using gcc 4.4.

[Jan Alonzo]

Created attachment 30382 [details]
refactor cflags

This patch refactor our use of cflags, cxxflags and cppflags. I also tested this in gcc 4.4 in x86-64 and I didn't get any crash.

[Jan Alonzo]

Created attachment 30383 [details]
refactor use of libadd

This patch refactors our use of libadd. Tested and doesn't crash in gcc 4.4, x86-64.

[Jan Alonzo]

Created attachment 30385 [details]
remove hardcoded -O2 and add no-install and no-fast-install 

This patch removes hardcoded the -O2 since we already do this in configure time. Also add -no-install and -no-fast-install to uninstalled programs - this noticeably speeds up the loading time of this programs (e.g, GtkLauncher, DRT, unit tests).

[Jan Alonzo]

Created attachment 30386 [details]
style fixes

Mostly style fixes

[Jan Alonzo]

Created attachment 30388 [details]
add 'test' targets to run the unit tests and test report generation

This patch adds "test" targets to the build. This allows us to run the tests during distcheck as well as generate test reports (e.g, make perf-report, make full-report, etc.)

[Xan Lopez]

Comment on attachment 30382 [details]
refactor cflags

> +# JavaScriptCore
>  javascriptcore_cppflags += \
> -	-I$(srcdir)/JavaScriptCore \
>  	-I$(srcdir)/JavaScriptCore/ForwardingHeaders \
>  	-I$(srcdir)/JavaScriptCore/parser \
>  	-I$(srcdir)/JavaScriptCore/wtf \
> @@ -169,34 +218,14 @@ libWebCore_la_SOURCES = \
>  

This is because that's not needed to build WebCore, right? I would mention it in the JSC ChangeLog, instead of just describing the change.

In any case, it looks good to me, so r=me.

[Xan Lopez]

Comment on attachment 30383 [details]
refactor use of libadd

Why is this desirable?

[Xan Lopez]

Comment on attachment 30385 [details]
remove hardcoded -O2 and add no-install and no-fast-install 

Looks good.

[Xan Lopez]

Comment on attachment 30388 [details]
add 'test' targets to run the unit tests and test report generation

Great!

[Xan Lopez]

Comment on attachment 30386 [details]
style fixes

OK.

[Jan Alonzo]

(In reply to comment #17)
> (From update of attachment 30383 [details] [review])
> Why is this desirable?
> 

It doesn't make sense that we're adding those libraries in a convenience library. It should be done in libwebkit.

[Xan Lopez]

(In reply to comment #21)
> (In reply to comment #17)
> > (From update of attachment 30383 [details] [review] [review])
> > Why is this desirable?
> > 
> 
> It doesn't make sense that we're adding those libraries in a convenience
> library. It should be done in libwebkit.
> 

Well, my naive assumption was that "add them where they are first needed" or something like that makes sense. I guess that what I'm asking is why adding them in the final library is better, what benefits does it have.

[Jan Alonzo]

(In reply to comment #16)
> (From update of attachment 30382 [details] [review])
> > +# JavaScriptCore
> >  javascriptcore_cppflags += \
> > -	-I$(srcdir)/JavaScriptCore \
> >  	-I$(srcdir)/JavaScriptCore/ForwardingHeaders \
> >  	-I$(srcdir)/JavaScriptCore/parser \
> >  	-I$(srcdir)/JavaScriptCore/wtf \
> > @@ -169,34 +218,14 @@ libWebCore_la_SOURCES = \
> >  

Landed in r43824

> This is because that's not needed to build WebCore, right? I would mention it
> in the JSC ChangeLog, instead of just describing the change.
> 
> In any case, it looks good to me, so r=me.
> 

[Jan Alonzo]

(In reply to comment #18)
> (From update of attachment 30385 [details] [review])
> Looks good.
> 

Landed in r43825

[Jan Alonzo]

(In reply to comment #20)
> (From update of attachment 30386 [details] [review])
> OK.
> 

Landed in r43826

[Jan Alonzo]

(In reply to comment #19)
> (From update of attachment 30388 [details] [review])
> Great!
> 

Landed in r43827

[Jan Alonzo]

Comment on attachment 30382 [details]
refactor cflags

Clearing review flag

[Jan Alonzo]

Comment on attachment 30385 [details]
remove hardcoded -O2 and add no-install and no-fast-install 

Clearing review flag

[Jan Alonzo]

Comment on attachment 30386 [details]
style fixes

Clearing review flag

[Jan Alonzo]

Comment on attachment 30388 [details]
add 'test' targets to run the unit tests and test report generation

Clearing review flag

[Alex Dedul]

(In reply to comment #25)
> (In reply to comment #20)
> > (From update of attachment 30386 [details] [review] [review])
> > OK.
> > 
> 
> Landed in r43826
> 

Seems like this changeset breaks build. Here is what i get

WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp: In member function 'virtual void WebKit::FrameLoaderClient::dispatchDidFailLoad(const WebCore::ResourceError&)':
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:889: error: 'DATA_DIR' was not declared in this scope
make[1]: *** [WebKit/gtk/WebCoreSupport/libwebkit_1_0_la-FrameLoaderClientGtk.lo] Error 1
make[1]: *** Waiting for unfinished jobs....
WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp: In member function 'virtual WebCore::Page* WebKit::InspectorClient::createPage()':
WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp:95: error: 'DATA_DIR' was not declared in this scope
WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp: In member function 'virtual WebCore::String WebKit::InspectorClient::localizedStringsURL()':
WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp:107: error: 'DATA_DIR' was not declared in this scope
make[1]: *** [WebKit/gtk/WebCoreSupport/libwebkit_1_0_la-InspectorClientGtk.lo] Error 1


I guess its because of this change in WebCore/GNUmakefile.am

-webcore_cppflags += -DDATA_DIR=\"${datadir}\"
-

[Xan Lopez]

You need to re-run autogen.sh to regenerate the build system.

[Alex Dedul]

I did. Tried one more time now, still the same result..

[Xan Lopez]

(In reply to comment #33)
> I did. Tried one more time now, still the same result..
> 

Well, it works OK here and in the build bot, so I think the problem must be on your side somehow, not sure what can be wrong if you have already tried autogen.sh.

[Jan Alonzo]

(In reply to comment #33)
> I did. Tried one more time now, still the same result..
> 

Try make clean then autogen. If that doesn't work, do a clean build using make maintainer-clean and run autogen.sh again.

Let's us know if that works. If not, post the error here.

[Jan Alonzo]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #17)
> > > (From update of attachment 30383 [details] [review] [review] [review])
> > > Why is this desirable?
> > > 
> > 
> > It doesn't make sense that we're adding those libraries in a convenience
> > library. It should be done in libwebkit.
> > 
> 
> Well, my naive assumption was that "add them where they are first needed" or
> something like that makes sense. I guess that what I'm asking is why adding
> them in the final library is better, what benefits does it have.

Hi Xan. Your assumption's fine. This change is basically just moving the linking out of libWebCore and into libwebkit as well as removing _LIBADD altogether out of libWebCore (and just have it in libwebkit).

> 

[Alex Dedul]

(In reply to comment #35)
> (In reply to comment #33)
> > I did. Tried one more time now, still the same result..
> > 
> 
> Try make clean then autogen. If that doesn't work, do a clean build using make
> maintainer-clean and run autogen.sh again.
> 
> Let's us know if that works. If not, post the error here.
> 

So, i just copied everything from svn, so it's clean. Now i run as in build bot here

http://build.webkit.org/builders/GTK%20Linux%20Release/builds/551/steps/compile-webkit/logs/stdio

perl ./WebKitTools/Scripts/build-webkit --release --gtk

And it gives this

----------

./doltlibtool --tag=CXX   --mode=compile g++ -DHAVE_CONFIG_H -I. -I../..  -DWTF_USE_ICU_UNICODE=1 -DBUILDING_CAIRO__=1 -DBUILDING_GTK__=1 -DWTF_CHANGES -DPACKAGE_LOCALE_DIR=\"/usr/local/share/locale\"  -DXP_UNIX -DNDEBUG    -I../../WebCore -I../../WebCore/accessibility -I../../WebCore/bindings/js -I../../WebCore/bridge -I../../WebCore/bridge/c -I../../WebCore/css -I../../WebCore/dom -I../../WebCore/editing -I../../WebCore/history -I../../WebCore/html -I../../WebCore/inspector -I../../WebCore/loader -I../../WebCore/loader/appcache -I../../WebCore/loader/archive -I../../WebCore/loader/icon -I../../WebCore/page -I../../WebCore/page/animation -I../../WebCore/platform -I../../WebCore/platform/animation -I../../WebCore/platform/graphics -I../../WebCore/platform/graphics/filters -I../../WebCore/platform/graphics/transforms -I../../WebCore/platform/image-decoders -I../../WebCore/platform/image-decoders/bmp -I../../WebCore/platform/image-decoders/gif -I../../WebCore/platform/image-decoders/ico -I../../WebCore/platform/image-decoders/jpeg -I../../WebCore/platform/image-decoders/png -I../../WebCore/platform/image-decoders/xbm -I../../WebCore/platform/network -I../../WebCore/platform/text -I../../WebCore/plugins -I../../WebCore/rendering -I../../WebCore/rendering/style -I../../WebCore/workers -I../../WebCore/xml -I./WebCore/bindings/js -DENABLE_JAVASCRIPT_DEBUGGER=1 -DENABLE_OFFLINE_WEB_APPLICATIONS=1 -DENABLE_DASHBOARD_SUPPORT=1 -DENABLE_DATABASE=1 -I../../WebCore/platform/sql -I../../WebCore/storage  -DENABLE_DOM_STORAGE=1 -I../../WebCore/storage -DENABLE_ICONDATABASE=1  -DENABLE_VIDEO=1  -DENABLE_XPATH=1 -DENABLE_XSLT=1 -DENABLE_WORKERS=1   -DENABLE_SVG=1 -I../../WebCore/svg -I../../WebCore/svg/animation -I../../WebCore/svg/graphics -I../../WebCore/svg/graphics/filters -DENABLE_SVG_USE=1 -DENABLE_SVG_FOREIGN_OBJECT=1 -DENABLE_SVG_FONTS=1  -DENABLE_SVG_AS_IMAGE=1 -DENABLE_SVG_ANIMATION=1 -I../../JavaScriptCore -I../../JavaScriptCore/ForwardingHeaders -I../../JavaScriptCore/parser -I../../JavaScriptCore/wtf -I./DerivedSources -I../../JavaScriptCore -I../../JavaScriptCore/API -I../../JavaScriptCore/ForwardingHeaders -I../../JavaScriptCore/interpreter -I../../JavaScriptCore/bytecode -I../../JavaScriptCore/bytecompiler -I../../JavaScriptCore/debugger -I../../JavaScriptCore/jit -I../../JavaScriptCore/pcre -I../../JavaScriptCore/profiler -I../../JavaScriptCore/runtime -I../../JavaScriptCore/wrec -I../../JavaScriptCore/jit -I../../JavaScriptCore/assembler -I../../JavaScriptCore/wtf/unicode -I../../JavaScriptCore/yarr -I./JavaScriptCore/pcre -I./JavaScriptCore/parser -I./JavaScriptCore/runtime  -DWTF_USE_SOUP=1 -I../../WebCore/accessibility/gtk -I../../WebCore/loader/gtk -I../../WebCore/page/gtk -I../../WebCore/platform/graphics/cairo -I../../WebCore/platform/graphics/gtk -I../../WebCore/platform/gtk -I../../WebCore/platform/network/soup -DUSE_FREETYPE=1  -I../../WebCore/svg/graphics/cairo  -DBUILDING_WEBKIT -I../../WebKit/gtk -I../../WebKit/gtk/WebCoreSupport -I../../WebKit/gtk/webkit -I./WebKit/gtk/webkit  -fvisibility-inlines-hidden -fno-rtti -fno-strict-aliasing -Wall -W -Wcast-align -Wchar-subscripts -Wreturn-type -Wformat -Wformat-security -Wno-format-y2k -Wundef -Wmissing-format-attribute -Wpointer-arith -Wwrite-strings -Wno-unused-parameter -Wno-parentheses -fno-exceptions -fvisibility=hidden -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include   -I/usr/include  -I/usr/include/libxml2   -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12   -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/freetype2   -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12    -I/usr/include/libsoup-2.4 -I/usr/include/libxml2 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include   -I/usr/include/cairo -I/usr/include/freetype2 -I/usr/include/pixman-1 -I/usr/include/libpng12    -pthread -I/usr/include/gstreamer-0.10 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/libxml2   -I/usr/include/libxml2     -I/usr/include/enchant -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include    -O2 -MT WebKit/gtk/WebCoreSupport/libwebkit_1_0_la-FrameLoaderClientGtk.lo -MD -MP -MF WebKit/gtk/WebCoreSupport/.deps/libwebkit_1_0_la-FrameLoaderClientGtk.Tpo -c -o WebKit/gtk/WebCoreSupport/libwebkit_1_0_la-FrameLoaderClientGtk.lo `test -f 'WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp' || echo '../../'`WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp
../../WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp: In member function 'virtual void WebKit::FrameLoaderClient::dispatchDidFailLoad(const WebCore::ResourceError&)':
../../WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:889: error: 'DATA_DIR' was not declared in this scope
make[1]: *** [WebKit/gtk/WebCoreSupport/libwebkit_1_0_la-FrameLoaderClientGtk.lo] Error 1
make[1]: Leaving directory `/home/plisk/tmp/trunk/WebKitBuild/Release'
make: *** [all] Error 2

Failed to build WebKit using 'make'!

----------

Comparing this to the commands run by build bot here is no -DDATA_DIR, while build bot have it..

PS Tried to add that webcore_cppflags += -DDATA_DIR=\"${datadir}\" back to WebCore/GNUmakefile.am. This way it has -DDATA_DIR in compile commands and builds OK all files in /WebKit/gtk/WebCoreSupport/ that use DATA_DIR define/var..

[Jan Alonzo]

(In reply to comment #37)
> ----------
> 
> Comparing this to the commands run by build bot here is no -DDATA_DIR, while
> build bot have it..
> 
> PS Tried to add that webcore_cppflags += -DDATA_DIR=\"${datadir}\" back to
> WebCore/GNUmakefile.am. This way it has -DDATA_DIR in compile commands and
> builds OK all files in /WebKit/gtk/WebCoreSupport/ that use DATA_DIR
> define/var..

DATA_DIR wasn't really removed. It was just moved in top-level GNUmakefile. Which distro and what automake version are you using?

> 

[Alex Dedul]

(In reply to comment #38)
> (In reply to comment #37)
> > ----------
> > 
> > Comparing this to the commands run by build bot here is no -DDATA_DIR, while
> > build bot have it..
> > 
> > PS Tried to add that webcore_cppflags += -DDATA_DIR=\"${datadir}\" back to
> > WebCore/GNUmakefile.am. This way it has -DDATA_DIR in compile commands and
> > builds OK all files in /WebKit/gtk/WebCoreSupport/ that use DATA_DIR
> > define/var..
> 
> DATA_DIR wasn't really removed. It was just moved in top-level GNUmakefile.
> Which distro and what automake version are you using?
> 
> > 
> 

Ah, i see, having issues with svn here.. Now it's OK, thanks a lot for the response.

[Xan Lopez]

(In reply to comment #36)
> > > It doesn't make sense that we're adding those libraries in a convenience
> > > library. It should be done in libwebkit.
> > > 
> > 
> > Well, my naive assumption was that "add them where they are first needed" or
> > something like that makes sense. I guess that what I'm asking is why adding
> > them in the final library is better, what benefits does it have.
> 
> Hi Xan. Your assumption's fine. This change is basically just moving the
> linking out of libWebCore and into libwebkit as well as removing _LIBADD
> altogether out of libWebCore (and just have it in libwebkit).
> 

Right. So if you think this makes the setup cleaner or something although it does not really have any visible effect I'm happy to r+ it, I wast just wondering what was the point of it.

[Jan Alonzo]

(In reply to comment #40)
> (In reply to comment #36)
> > > > It doesn't make sense that we're adding those libraries in a convenience
> > > > library. It should be done in libwebkit.
> > > > 
> > > 
> > > Well, my naive assumption was that "add them where they are first needed" or
> > > something like that makes sense. I guess that what I'm asking is why adding
> > > them in the final library is better, what benefits does it have.
> > 
> > Hi Xan. Your assumption's fine. This change is basically just moving the
> > linking out of libWebCore and into libwebkit as well as removing _LIBADD
> > altogether out of libWebCore (and just have it in libwebkit).
> > 
> 
> Right. So if you think this makes the setup cleaner or something although it
> does not really have any visible effect I'm happy to r+ it, I wast just
> wondering what was the point of it.

Please r+ it. It would be nice to have to have this.

[Jan Alonzo]

Comment on attachment 30383 [details]
refactor use of libadd

Patch landed in r44103. Clearing review flag.

[Jan Alonzo]

This is done. Closing