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.
Created attachment 29604 [details] patch Patch for the issues I raised above. I'll split this and fix the changelog and stuff later..
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?
(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).
Created attachment 30153 [details] fixup uses of libadd, cflags, cxxflags and ldflags This patch refactors our usage of CFLAGS, CXXFLAGS, LDFLAGS and LIBADD.
Created attachment 30154 [details] cleanup webcore, webkit, and jsc flags. Refactor webcore, webkit and jsc flags.
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 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 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 on attachment 30155 [details] add glib-2.0 in pkg-config Landed in r43564. Clearing review flag.
(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.
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.
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.
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).
Created attachment 30386 [details] style fixes Mostly style fixes
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 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 on attachment 30383 [details] refactor use of libadd Why is this desirable?
Comment on attachment 30385 [details] remove hardcoded -O2 and add no-install and no-fast-install Looks good.
Comment on attachment 30388 [details] add 'test' targets to run the unit tests and test report generation Great!
Comment on attachment 30386 [details] style fixes OK.
(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.
(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.
(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. >
(In reply to comment #18) > (From update of attachment 30385 [details] [review]) > Looks good. > Landed in r43825
(In reply to comment #20) > (From update of attachment 30386 [details] [review]) > OK. > Landed in r43826
(In reply to comment #19) > (From update of attachment 30388 [details] [review]) > Great! > Landed in r43827
Comment on attachment 30382 [details] refactor cflags Clearing review flag
Comment on attachment 30385 [details] remove hardcoded -O2 and add no-install and no-fast-install Clearing review flag
Comment on attachment 30386 [details] style fixes Clearing review flag
Comment on attachment 30388 [details] add 'test' targets to run the unit tests and test report generation Clearing review flag
(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}\" -
You need to re-run autogen.sh to regenerate the build system.
I did. Tried one more time now, still the same result..
(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.
(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.
(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). >
(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..
(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? >
(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.
(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.
(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 on attachment 30383 [details] refactor use of libadd Patch landed in r44103. Clearing review flag.
This is done. Closing