Bug 25286 - [Gtk] Various autotools build refactoring and fixes
Summary: [Gtk] Various autotools build refactoring and fixes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Jan Alonzo
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2009-04-19 01:57 PDT by Jan Alonzo
Modified: 2009-06-25 04:53 PDT (History)
3 users (show)

See Also:


Attachments
patch (24.91 KB, patch)
2009-04-19 02:29 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
fixup uses of libadd, cflags, cxxflags and ldflags (7.92 KB, patch)
2009-05-09 04:59 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
cleanup webcore, webkit, and jsc flags. (13.84 KB, patch)
2009-05-09 05:00 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
add glib-2.0 in pkg-config (1.15 KB, patch)
2009-05-09 05:01 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
refactor cflags (5.99 KB, patch)
2009-05-15 07:48 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
refactor use of libadd (2.26 KB, patch)
2009-05-15 07:50 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
remove hardcoded -O2 and add no-install and no-fast-install (6.38 KB, patch)
2009-05-15 07:53 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
style fixes (9.94 KB, patch)
2009-05-15 07:54 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
add 'test' targets to run the unit tests and test report generation (2.16 KB, patch)
2009-05-15 07:57 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Alonzo 2009-04-19 01:57:56 PDT
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.
Comment 1 Jan Alonzo 2009-04-19 02:29:03 PDT
Created attachment 29604 [details]
patch

Patch for the issues I raised above. I'll split this and fix the changelog and stuff later..
Comment 2 Xan Lopez 2009-04-26 10:48:44 PDT
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?
Comment 3 Jan Alonzo 2009-05-02 15:17:53 PDT
(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). 
Comment 4 Jan Alonzo 2009-05-09 04:59:20 PDT
Created attachment 30153 [details]
fixup uses of libadd, cflags, cxxflags and ldflags

This patch refactors our usage of CFLAGS, CXXFLAGS, LDFLAGS and LIBADD.
Comment 5 Jan Alonzo 2009-05-09 05:00:25 PDT
Created attachment 30154 [details]
cleanup webcore, webkit, and jsc flags.

Refactor webcore, webkit and jsc flags.
Comment 6 Jan Alonzo 2009-05-09 05:01:16 PDT
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.
Comment 7 Holger Freyther 2009-05-11 20:17:44 PDT
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...
Comment 8 Jan Alonzo 2009-05-12 05:07:36 PDT
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.
Comment 9 Jan Alonzo 2009-05-12 05:08:05 PDT
Comment on attachment 30155 [details]
add glib-2.0 in pkg-config

Landed in r43564.

Clearing review flag.
Comment 10 Jan Alonzo 2009-05-14 06:14:41 PDT
(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.
Comment 11 Jan Alonzo 2009-05-15 07:48:45 PDT
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.
Comment 12 Jan Alonzo 2009-05-15 07:50:15 PDT
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.
Comment 13 Jan Alonzo 2009-05-15 07:53:48 PDT
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).
Comment 14 Jan Alonzo 2009-05-15 07:54:32 PDT
Created attachment 30386 [details]
style fixes

Mostly style fixes
Comment 15 Jan Alonzo 2009-05-15 07:57:00 PDT
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.)
Comment 16 Xan Lopez 2009-05-17 12:26:38 PDT
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.
Comment 17 Xan Lopez 2009-05-17 12:30:24 PDT
Comment on attachment 30383 [details]
refactor use of libadd

Why is this desirable?
Comment 18 Xan Lopez 2009-05-17 12:31:21 PDT
Comment on attachment 30385 [details]
remove hardcoded -O2 and add no-install and no-fast-install 

Looks good.
Comment 19 Xan Lopez 2009-05-17 12:32:40 PDT
Comment on attachment 30388 [details]
add 'test' targets to run the unit tests and test report generation

Great!
Comment 20 Xan Lopez 2009-05-17 12:36:03 PDT
Comment on attachment 30386 [details]
style fixes

OK.
Comment 21 Jan Alonzo 2009-05-17 13:04:22 PDT
(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.
Comment 22 Xan Lopez 2009-05-17 13:07:34 PDT
(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.
Comment 23 Jan Alonzo 2009-05-18 04:31:14 PDT
(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.
> 

Comment 24 Jan Alonzo 2009-05-18 04:32:34 PDT
(In reply to comment #18)
> (From update of attachment 30385 [details] [review])
> Looks good.
> 

Landed in r43825
Comment 25 Jan Alonzo 2009-05-18 04:33:23 PDT
(In reply to comment #20)
> (From update of attachment 30386 [details] [review])
> OK.
> 

Landed in r43826
Comment 26 Jan Alonzo 2009-05-18 04:33:51 PDT
(In reply to comment #19)
> (From update of attachment 30388 [details] [review])
> Great!
> 

Landed in r43827
Comment 27 Jan Alonzo 2009-05-18 04:35:13 PDT
Comment on attachment 30382 [details]
refactor cflags

Clearing review flag
Comment 28 Jan Alonzo 2009-05-18 04:35:33 PDT
Comment on attachment 30385 [details]
remove hardcoded -O2 and add no-install and no-fast-install 

Clearing review flag
Comment 29 Jan Alonzo 2009-05-18 04:35:55 PDT
Comment on attachment 30386 [details]
style fixes

Clearing review flag
Comment 30 Jan Alonzo 2009-05-18 04:36:16 PDT
Comment on attachment 30388 [details]
add 'test' targets to run the unit tests and test report generation

Clearing review flag
Comment 31 Alex Dedul 2009-05-18 22:34:00 PDT
(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}\"
-
Comment 32 Xan Lopez 2009-05-19 00:44:38 PDT
You need to re-run autogen.sh to regenerate the build system.
Comment 33 Alex Dedul 2009-05-19 01:00:58 PDT
I did. Tried one more time now, still the same result..
Comment 34 Xan Lopez 2009-05-19 02:42:52 PDT
(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.
Comment 35 Jan Alonzo 2009-05-19 03:01:55 PDT
(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.
Comment 36 Jan Alonzo 2009-05-19 03:20:31 PDT
(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).

> 

Comment 37 Alex Dedul 2009-05-19 04:50:38 PDT
(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..
Comment 38 Jan Alonzo 2009-05-19 05:47:29 PDT
(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?

> 

Comment 39 Alex Dedul 2009-05-19 05:54:10 PDT
(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.
Comment 40 Xan Lopez 2009-05-23 06:20:03 PDT
(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.

Comment 41 Jan Alonzo 2009-05-23 11:07:32 PDT
(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.

Comment 42 Jan Alonzo 2009-05-23 15:24:30 PDT
Comment on attachment 30383 [details]
refactor use of libadd

Patch landed in r44103. Clearing review flag.
Comment 43 Jan Alonzo 2009-06-25 04:53:40 PDT
This is done. Closing