The attached patch modifies the autotools build system to support the EFL port. By default, the GTK+ port will be built, so no changes to the GTK+ build bots are necessary; to build the EFL port, the '--with-port=efl' argument should be passed to configure. There shouldn't be any performance issues (except for negligible AM_CONDITIONAL tests while generating the makefiles). Everything EFL-related are in 'if EFL_PORT' / 'endif' blocks inside GNUmakefile.am files; GTK port have its own GTK_PORT conditional. The patch is somewhat big, but I decided not to split it (and apply it in one go) to make the GTK+ buildbot happy.
Created attachment 51924 [details] Changes to the autotools build system to build the EFL port
Attachment 51924 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 File not a recognized type to check. Skipping: "GNUmakefile.am" WebCore/ChangeLog:3: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:5: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] ChangeLog:3: Line contains tab character. [whitespace/tab] [5] ChangeLog:5: Line contains tab character. [whitespace/tab] [5] ChangeLog:6: Line contains tab character. [whitespace/tab] [5] ChangeLog:8: Line contains tab character. [whitespace/tab] [5] ChangeLog:9: Line contains tab character. [whitespace/tab] [5] File not a recognized type to check. Skipping: "WebKit/efl/EWebLauncher/Makefile.am" JavaScriptCore/ChangeLog:3: Line contains tab character. [whitespace/tab] [5] JavaScriptCore/ChangeLog:5: Line contains tab character. [whitespace/tab] [5] JavaScriptCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] JavaScriptCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] File not a recognized type to check. Skipping: "JavaScriptCore/GNUmakefile.am" File not a recognized type to check. Skipping: "WebKit/efl/DefaultTheme/Makefile.am" File not a recognized type to check. Skipping: "WebKitTools/GNUmakefile.am" File not a recognized type to check. Skipping: "configure.ac" WebKitTools/ChangeLog:3: Line contains tab character. [whitespace/tab] [5] WebKitTools/ChangeLog:5: Line contains tab character. [whitespace/tab] [5] WebKitTools/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] WebKitTools/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] File not a recognized type to check. Skipping: "WebCore/GNUmakefile.am" WebKit/ChangeLog:3: Line contains tab character. [whitespace/tab] [5] WebKit/ChangeLog:5: Line contains tab character. [whitespace/tab] [5] WebKit/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] WebKit/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] WebKit/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Total errors found: 22 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 51924 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/1580048
Created attachment 51939 [details] Changes to the autotools build system to build the EFL port Changes to make the style queue bot happy.
(In reply to comment #3) > Attachment 51924 [details] did not build on gtk: > Build output: http://webkit-commit-queue.appspot.com/results/1580048 Bug #36766 adds the missing ewebkit.pc.in file.
Attachment 51939 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/1568062
You could have added the pc file on this one, I think. Can you resubmit when the other lands, so that we get fresh output from the EWS? I'll look at the actual patch later today.
Created attachment 52298 [details] Changes to the autotools build system to build the EFL port Resubmitting to obtain fresh output from EWS.
Created attachment 52340 [details] Changes to the autotools build system to build the EFL port Submitting updated patch. (Previous patch was being rejected.)
Created attachment 52343 [details] Changes to the autotools build system to build the EFL port Attachment 52340 [details] still had a rejection, submitting a fixed version.
Comment on attachment 52343 [details] Changes to the autotools build system to build the EFL port 228 libJavaScriptCore_la_LIBADD += \ 229 $(ECORE_X_LIBS) \ 230 $(EFLDEPS_LIBS) This sounds like a bad idea. It's your decision to make, but I think you should try to keep X-related deps out of JSC. 20022032 if TARGET_X11 2033 if GTK_PORT 20032034 webcoregtk_sources += \ 20042035 WebCore/plugins/gtk/gtk2xtbin.c \ 20052036 WebCore/plugins/gtk/gtk2xtbin.h \ 20062037 WebCore/plugins/gtk/xembed.h 20072038 endif 2039 endif Perhaps GTK_PORT should be the outer check, instead? Are you also going to use TARGET_*? 28 # Benchmark tools ###################################################### 29 Programs_EWebBenchStress_CPPFLAGS = \ 30 -I$(top_srcdir)/WebKit/efl \ 31 -I$(top_srcdir)/WebKit/efl/ewk \ 32 $(EFLDEPS_CFLAGS) Looks like these are no longer going to go in? This doesn't look too bad, actually. I was expecting more surgery. I would like to get xan at least to look at this before it goes in, given the importance of the build system. It does get more complicated, but that may just be the kick we need to switch to something better =P. r- for now because of the comments above that need addressing.
My comment after spending 3 minutes looking at the patch is that it makes thing even more messy (and I'd say our build system jumped the shark complexity-wise some time ago), and I don't really like that. Maybe it would be the time to refactor things a bit and try to keep GTK+ and EFL support in different Makefiles that we include dependently, sharing only the common bits.
Xan, yes this is one option. But do you want separated configure.ac as well or no?
(In reply to comment #11) > (From update of attachment 52343 [details]) > 228 libJavaScriptCore_la_LIBADD += \ > 229 $(ECORE_X_LIBS) \ > 230 $(EFLDEPS_LIBS) > > This sounds like a bad idea. It's your decision to make, but I think you > should try to keep X-related deps out of JSC. > I'll see if I can compile it without these deps. > > 20022032 if TARGET_X11 > 2033 if GTK_PORT > 20032034 webcoregtk_sources += \ > 20042035 WebCore/plugins/gtk/gtk2xtbin.c \ > 20052036 WebCore/plugins/gtk/gtk2xtbin.h \ > 20062037 WebCore/plugins/gtk/xembed.h > 20072038 endif > 2039 endif > > Perhaps GTK_PORT should be the outer check, instead? Are you also going to > use TARGET_*? > Yes, X11 isn't the only target supported by EFL (although our port only supports it ATM), so we'll use TARGET_* in the future. > 28 # Benchmark tools ###################################################### > 29 Programs_EWebBenchStress_CPPFLAGS = \ > 30 -I$(top_srcdir)/WebKit/efl \ > 31 -I$(top_srcdir)/WebKit/efl/ewk \ > 32 $(EFLDEPS_CFLAGS) > > Looks like these are no longer going to go in? > Yes, I'll remove references to the benchmark programs.
(In reply to comment #13) > Xan, yes this is one option. But do you want separated configure.ac as well or > no? Not sure how you could make that work transparently, but in any case I don't think that's necessary at this point.
(In reply to comment #15) > (In reply to comment #13) > > Xan, yes this is one option. But do you want separated configure.ac as well or > > no? > > Not sure how you could make that work transparently, but in any case I don't > think that's necessary at this point. To make things clear so we provide a patch that works right away and not go through endless review cycles: Do you want something like: GNUMakefile.am, that: if GTK_PORT include GNUMakefileGtk.am endif if EFL_PORT include GNUMakefileEfl.am endif with all required GTK or EFL references in these specific files. There should be one patch refactoring Gtk, splitting it into GNUMakefile.am and GNUMakefileGtk.am, and then another adding GNUMakefileEfl.am and required includes in central makefile? Moreover, how do you like common parts? Should they be preferably initialized to the correct _sources _LIBADD like case 1 below, or defined as variables to be used like in case 2: Case 1: noinst_LTLIBRARIES += libJavaScriptCore.la Case 2: COMMON_NOINST_LIBS += libJavaScriptCore.la #and from inside port: noinst_LTLIBRARIES += $(COMMON_NOINST_LIBS) I like case1 better.
(In reply to comment #16) > To make things clear so we provide a patch that works right away and not go > through endless review cycles: Do you want something like: > > GNUMakefile.am, that: > > if GTK_PORT > include GNUMakefileGtk.am > endif > > if EFL_PORT > include GNUMakefileEfl.am > endif Exactly. > > with all required GTK or EFL references in these specific files. There should > be one patch refactoring Gtk, splitting it into GNUMakefile.am and > GNUMakefileGtk.am, and then another adding GNUMakefileEfl.am and required > includes in central makefile? First refactoring the GTK port and then adding the EFL support would be the ideal way to do it, yep. > > Moreover, how do you like common parts? Should they be preferably initialized > to the correct _sources _LIBADD like case 1 below, or defined as variables to > be used like in case 2: > > Case 1: > noinst_LTLIBRARIES += libJavaScriptCore.la > > Case 2: > COMMON_NOINST_LIBS += libJavaScriptCore.la > #and from inside port: > noinst_LTLIBRARIES += $(COMMON_NOINST_LIBS) > > I like case1 better. Unless you somehow need to refer to the variables from the port-specific files I don't see any reason to not do it like 1), yeah.
Created attachment 53704 [details] Cleanup the GTK+ build system We've decided to use CMake instead of autotools for the EFL port. However, here's an updated patch that might help if someone else wants to use autotools. Most GTK+-related stuff is inside GNUmakefileGtk.am files or (if they're small enough) surrounded by if GTK_PORT..endif blocks.
This won't be necessary anymore. Closing the report.