Bug 36761 – [EFL] Modify the autotools build system to add support to build the EFL port

• Status: RESOLVED
• Resolution: INVALID
• Component: Platform

[Leandro Pereira]

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.

[Leandro Pereira]

Created attachment 51924 [details]
Changes to the autotools build system to build the EFL port

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 51924 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1580048

[Leandro Pereira]

Created attachment 51939 [details]
Changes to the autotools build system to build the EFL port

Changes to make the style queue bot happy.

[Leandro Pereira]

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

[WebKit Review Bot]

Attachment 51939 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1568062

[Gustavo Noronha (kov)]

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.

[Leandro Pereira]

Created attachment 52298 [details]
Changes to the autotools build system to build the EFL port

Resubmitting to obtain fresh output from EWS.

[Leandro Pereira]

Created attachment 52340 [details]
Changes to the autotools build system to build the EFL port

Submitting updated patch. (Previous patch was being rejected.)

[Leandro Pereira]

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.

[Gustavo Noronha (kov)]

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.

[Xan Lopez]

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.

[Gustavo Sverzut Barbieri]

Xan, yes this is one option. But do you want separated configure.ac as well or no?

[Leandro Pereira]

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

[Xan Lopez]

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

[Gustavo Sverzut Barbieri]

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

[Xan Lopez]

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

[Leandro Pereira]

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.

[Leandro Pereira]

This won't be necessary anymore. Closing the report.