Bug 37945 – CMake buildsystem

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

Created attachment 53982 [details]
First step for CMake buildsystem

The patch adds a CMake buildsystem for JSC.

This is only a first step, but it shows how it might look in the future.

* I tried it only with windows in the moment.
* I did a small hack at dfables to use the MS compiler.
* If you want to try it out you need the requirements in the WebKitLibraries/win folder.
* Maybe you need to create a "QuartzCorePresent.h" with "#define QUARTZCORE_PRESENT 0" in one of the include directories.

[Patrick R. Gansterer]

Created attachment 54140 [details]
The patch 0.2

This new version of the patch should work now on linux with Gtk or Qt too.

If you want to try it on windows:
* install CMake
* check for the requirements in the WebKitLibraries/win folder
* apply the patch to your WebKit directory
* create an empty directory for the build (WebKitBuild)
* run "cmake -G "Visual Studio 8 2005" <PATH_TO_WEBKIT_SOURCEDIR"
* CMake will chek your platform
* If nothing went wrong you should have a WebKit.sln
* Open the solution file and try it!

If you use cmake 2.6 you might have some missing CMake modules (FindBISO, FindFLEX, ...). You can download them from http://cmake.org/gitweb?p=cmake.git;a=tree;f=Modules into the cmake directory to get it running.

[Leandro Pereira]

(In reply to comment #1)
> Created an attachment (id=54140) [details]
> The patch 0.2
> 
> This new version of the patch should work now on linux with Gtk or Qt too.
>

I have some considerations:

1) Are you sure you need to list the headers in addition to the source files? On the build system we've made for the EFL port, we didn't include them and it worked like a charm.

2) Wouldn't it be nice to split the platform specifics into CMakeListsPlatform.txt? This way the main CMakeLists.txt wont't get polluted, with the drawback of having more files to touch in the event of certain changes (but it'll be easier than today since it is still only one build system format).

3) Auto-detecting the platform is nice, but we need to come up with a better way to determine which build will be made (e.g. one might have both GTK+ and Qt libraries and won't be able to build the Qt port -- unless the main CMakeLists.txt file is edited). Perhaps a WEBKIT_PORT variable to be passed passed to cmake?

In fact, I've made those changes to your previous patch. I am updating it to take advantage of version 0.2 of your patch and I'll post here as soon as I finish.

[Patrick R. Gansterer]

(In reply to comment #2)
> 1) Are you sure you need to list the headers in addition to the source files?
> On the build system we've made for the EFL port, we didn't include them and it
> worked like a charm.
The reason is to get them into the Visual Studio files. It will work without them too, but it is usual to have the headers in the vcproj files.

> 2) Wouldn't it be nice to split the platform specifics into
> CMakeListsPlatform.txt? This way the main CMakeLists.txt wont't get polluted,
> with the drawback of having more files to touch in the event of certain changes
> (but it'll be easier than today since it is still only one build system
> format).
Since I have not done the WebCore and WebKit part by now, I don't know the exact requirements there to do a _clean_ an universal solution.
It is possible to split the files, but it is very unusal in the CMake world.

> 3) Auto-detecting the platform is nice, but we need to come up with a better
> way to determine which build will be made (e.g. one might have both GTK+ and Qt
> libraries and won't be able to build the Qt port -- unless the main
> CMakeLists.txt file is edited). Perhaps a WEBKIT_PORT variable to be passed
> passed to cmake?
That's the reason why I move all platform decisions into the root CMakeLists.txt. I'd like to implement a possibility to switch to a specific port/backend via a CMake option. (auto dedection can only be the default option).

[Leandro Pereira]

Created attachment 54637 [details]
cmake build system 0.3

This version I'm submitting currently only supports the Gtk port (as I'm pretty familiar with its build system). It is basically Patrick's 0.2 version, albeit somewhat refactored to support more ports easily.

This version does not build a port automatically, -DPORT=Gtk must be passed to CMake. Autodetection can be made if no port is chosen, but I did not add this ability in the attached patch.

The references to header files were removed (I did that before reading comment #3) from the CMakeLists, but it is easy to add them later.

Although there isn't currently any way to choose which features to build and which features to exclude (except by editing Options${PORT} files), I'd like to offer a default feature set and enable/disable each one individually by using CMake's OPTION().

There are other things I'd like to do, but I am submitting this to get some feedback and know if I'm on the right track.

[Leandro Pereira]

Created attachment 54701 [details]
cmake build system 0.3

Gah, I've sent the ChangeLog instead of the patch. Here goes the correct file :)

[Leandro Pereira]

Created attachment 54939 [details]
CMake build system -- port independent files

Here's another patch for the build system: this one contains all the port-independent files.

The EFL-port one follows.

[Leandro Pereira]

Created attachment 54940 [details]
CMake build system -- EFL port dependent files

This patch corresponds to the EFL-part of the build system. It should be a drop-in patch and just work with attachment #54939 [details].

[Leandro Pereira]

No comments were given -- could someone please apply the patches that are marked for review?

[Leandro Pereira]

Created attachment 55750 [details]
CMake build system, with EFL port dependent files

Attached is an updated version for attachment #54939 [details] and attachment #54940 [details].

This updated version is able to build SVN r59151, and replaces the ADD_FEATURE and DEL_FEATURE macros with a WEBKIT_FEATURE macro, which makes it easier to define default features and enable/disable each one from ccmake/cmake-gui/cmake -DENABLE_$FEATURE=ON|OFF.

[WebKit Review Bot]

Attachment 55750 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/CMakeListsEfl.txt:96:  Line contains tab character.  [whitespace/tab] [5]
WebKit/CMakeLists.txt:49:  Line contains tab character.  [whitespace/tab] [5]
WebKit/CMakeLists.txt:50:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Treat]

(In reply to comment #9)
> Created an attachment (id=55750) [details]
> CMake build system, with EFL port dependent files
> 
> Attached is an updated version for attachment #54939 [details] and attachment #54940 [details].
> 
> This updated version is able to build SVN r59151, and replaces the ADD_FEATURE and DEL_FEATURE macros with a WEBKIT_FEATURE macro, which makes it easier to define default features and enable/disable each one from ccmake/cmake-gui/cmake -DENABLE_$FEATURE=ON|OFF.

I am trying to review this and thought as a first step I'd build EFL port :)

However, I'm running into numerous errors with OptionsEfl.cmake and the usage of pkg-config.  Can you tell me for starters what packages are required?

[Gustavo Sverzut Barbieri]

(In reply to comment #11)
> (In reply to comment #9)
> > Created an attachment (id=55750) [details] [details]
> > CMake build system, with EFL port dependent files
> > 
> > Attached is an updated version for attachment #54939 [details] [details] and attachment #54940 [details] [details].
> > 
> > This updated version is able to build SVN r59151, and replaces the ADD_FEATURE and DEL_FEATURE macros with a WEBKIT_FEATURE macro, which makes it easier to define default features and enable/disable each one from ccmake/cmake-gui/cmake -DENABLE_$FEATURE=ON|OFF.
> 
> I am trying to review this and thought as a first step I'd build EFL port :)
> 
> However, I'm running into numerous errors with OptionsEfl.cmake and the usage of pkg-config.  Can you tell me for starters what packages are required?

Well, you'll have to put your compiler to work, you'll need EFL from SVN:

    http://svn.enlightenment.org/

It provides instructions. You need at least: eina, eet, evas, ecore, embryo and edje (In this order, check dependencies in that page). Don't use versions provided by packages as they are likely too old (if you use Gentoo, then you can use my overlay, listed in that page and stored in Enlightenment's SVN as it will build live packages).

For WebKit build you'll need bit more deps, just follow GTK's dependencies (http://trac.webkit.org/wiki/BuildingGtk). Our port is based on theirs and thus share lots of code and dependencies like Cairo and LibSoup, that in turn depend on Glib. Gstreamer should not be required as we don't support video, but GTK/GDK is still required until we split some bits out of Gtk files that we use.

[Patrick R. Gansterer]

Is there a reason for declaring the library names?
Why you use "${JavaScriptCore_LIBRARY_NAME}" instead of "JavaScriptCore" only?

> +SET(WebCore_PORT_FLAGS )
> +
> +FOREACH (_flag ${WebCore_PORT_FLAGS})
> +    ADD_TARGET_FLAGS(${WebCore_LIBRARY_NAME} ${_flag})
> +ENDFOREACH()
and
> +LIST (APPEND WebCore_PORT_FLAGS
> +    CAIRO
> +    ECORE_X
> ...
> +    PANGO
> +    SQLITE3
> +)
You have to dedect your libraries with a FindXXX.cmake first!
For SQLite you need to do:
1) Insert "FIND_PACKAGE(Sqlite REQUIRED)" into OptionsEfl.cmake.
2) Add a "LIST(APPEND WebCore_LIBRARIES ${SQLITE_LIBRARIES})" in your CMakeListsEfl

This must be done for all the other system and 3rdparty libs too!!
See http://www.cmake.org/Wiki/CMake:How_To_Find_Libraries

Then you can remove all your ADD_TARGET_FLAGS calls.

The missing FindXXX.cmake is a r-!

[Leandro Pereira]

(In reply to comment #13)
> Is there a reason for declaring the library names?
> Why you use "${JavaScriptCore_LIBRARY_NAME}" instead of "JavaScriptCore" only?
> 

Each port might want to define different names for these libraries. For instance, the EFL port creates a libewebkit.so under Unix; the GTK+ port might want to create a libwebkitgtk.so and so on.

The *Core libraries are usually statically-linked, but while developing, it is usually a good idea to make them shared objects to speed up linking time on machines with small amounts of memory. This way, being able to define different names for these libraries is important to avoid name clashes.

> You have to dedect your libraries with a FindXXX.cmake first!

I know it is standard practice in CMake to use FindXXX, however the EFL platform is basically Unix-targeted, so we're using PKG_CHECK_MODULES() which uses pkg-config.

If it is absolutely needed, we may either use standard FindXXX and/or create ones that don't exist (such as the ones for the EFL libraries).

However, note that these checks are being performed inside the EFL Options file, so even pkg-config not existing or behaving weirdly in platforms other than Unix, it won't really be a portability issue.

[Patrick R. Gansterer]

(In reply to comment #14)
> (In reply to comment #13)
> > Is there a reason for declaring the library names?
> > Why you use "${JavaScriptCore_LIBRARY_NAME}" instead of "JavaScriptCore" only?
> > 
> 
> Each port might want to define different names for these libraries. For instance, the EFL port creates a libewebkit.so under Unix; the GTK+ port might want to create a libwebkitgtk.so and so on.
Ok, makes sense! ;-)

> 
> > You have to dedect your libraries with a FindXXX.cmake first!
> 
> I know it is standard practice in CMake to use FindXXX, however the EFL platform is basically Unix-targeted, so we're using PKG_CHECK_MODULES() which uses pkg-config.
> 
> If it is absolutely needed, we may either use standard FindXXX and/or create ones that don't exist (such as the ones for the EFL libraries).
> 
> However, note that these checks are being performed inside the EFL Options file, so even pkg-config not existing or behaving weirdly in platforms other than Unix, it won't really be a portability issue.

I think it should be done via FindXXX.
Many of the 3rdparty libs like libxml, sqlite, ... are used on other ports too, so the FindXXX can be reused then.
Most of the FinXXX you'll need exist already.

[Adam Treat]

(In reply to comment #15)
> I think it should be done via FindXXX.
> Many of the 3rdparty libs like libxml, sqlite, ... are used on other ports too, so the FindXXX can be reused then.
> Most of the FinXXX you'll need exist already.

I agree.  The SQLITE one exists for sure.

[Leandro Pereira]

(In reply to comment #15)
> I think it should be done via FindXXX.
> Many of the 3rdparty libs like libxml, sqlite, ... are used on other ports too, so the FindXXX can be reused then.

If it uses standard FindXXX files, I don't see any problem -- maintaining non-standard files inside the WebKit tree might be a problem, though. 

Before posting another patch that uses FindXXX, I'll wait for Adam's review also, as he might have some other concerns.

[Gustavo Sverzut Barbieri]

(In reply to comment #16)
> (In reply to comment #15)
> > I think it should be done via FindXXX.
> > Many of the 3rdparty libs like libxml, sqlite, ... are used on other ports too, so the FindXXX can be reused then.
> > Most of the FinXXX you'll need exist already.
> 
> I agree.  The SQLITE one exists for sure.

Are you sure? At least in Cmake 2.8 it does not. Is there any repository of unofficial/3rd party FindXXX.cmake out there? For instance, a quick research of our needs do not exist in Cmake 2.8:

CAIRO
LIBSOUP
SQLITE3
GLIB (although there are GTK and GTK2)
-- following are EFL-specific --
EINA
EET
EVAS
ECORE (and sub-modules, like Ecore-X, Ecore-Evas, ...)
EMBRYO
EDJE (actually Embryo and Eet are indirect dependencies that come due Edje)

How should we proceed? Create FindXXX.cmake for all of them or just the first part?

[Adam Treat]

(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > I think it should be done via FindXXX.
> > > Many of the 3rdparty libs like libxml, sqlite, ... are used on other ports too, so the FindXXX can be reused then.
> > > Most of the FinXXX you'll need exist already.
> > 
> > I agree.  The SQLITE one exists for sure.
> 
> Are you sure? At least in Cmake 2.8 it does not. Is there any repository of unofficial/3rd party FindXXX.cmake out there? For instance, a quick research of our needs do not exist in Cmake 2.8:
> 
> CAIRO
> LIBSOUP
> SQLITE3
> GLIB (although there are GTK and GTK2)
> -- following are EFL-specific --
> EINA
> EET
> EVAS
> ECORE (and sub-modules, like Ecore-X, Ecore-Evas, ...)
> EMBRYO
> EDJE (actually Embryo and Eet are indirect dependencies that come due Edje)
> 
> How should we proceed? Create FindXXX.cmake for all of them or just the first part?

It exists as 3rdParty.  I'd prefer to have FindXXX.cmake for all of these.

[Adam Treat]

(In reply to comment #19)
>
> It exists as 3rdParty.  I'd prefer to have FindXXX.cmake for all of these.

Here are some... http://www.cmake.org/Wiki/CMake:Module_Maintainers

[Patrick R. Gansterer]

(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > I think it should be done via FindXXX.
> > > Many of the 3rdparty libs like libxml, sqlite, ... are used on other ports too, so the FindXXX can be reused then.
> > > Most of the FinXXX you'll need exist already.
> > 
> > I agree.  The SQLITE one exists for sure.
> 
> Are you sure? At least in Cmake 2.8 it does not. Is there any repository of unofficial/3rd party FindXXX.cmake out there? For instance, a quick research of our needs do not exist in Cmake 2.8:
Sorry, wrong example! ;-)

> GLIB (although there are GTK and GTK2)
GTK2 works with my GTK port. I think it will work for EFL too.

> CAIRO
http://zi.fi/cmake/Modules

> SQLITE3
http://websvn.kde.org/trunk/KDE/kdelibs/cmake/modules/

> How should we proceed? Create FindXXX.cmake for all of them or just the first part?
I think you should do all with the FinXXX.cmake. Maybe you can do only one FindEFL.cmake for EINA, EET, EVAS, ECORE, EMBRYO and EDJE since they are only used by the EFL port.

> CMAKE_MINIMUM_REQUIRED(VERSION 2.8)
If you need 2.8 only because of the FindXXX you should add them into the cmake directory. Not all linux distributions ship with 2.8 already.

> > Each port might want to define different names for these libraries. For instance, the EFL port creates a libewebkit.so under Unix; the GTK+ port might want to create a libwebkitgtk.so and so on.
> Ok, makes sense! ;-)
I think we should rename it to TARGET_NAME (JavaScriptCore_LIBRARY_NAME -> JavaScriptCore_TARGET_NAME). E.g. JSC isn't a library.

[Adam Treat]

Comment on attachment 55750 [details]
CMake build system, with EFL port dependent files

> +IF (NOT WTF_USE_ICU_UNICODE)
> +    ADD_DEFINITIONS(-DWTF_USE_ICU_UNICODE=0)

/home/manyoso/dev/webkit_1/JavaScriptCore/wtf/Platform.h:580:1: warning: "WTF_USE_ICU_UNICODE" redefined

This is in cross-platform wtf/CMakeLists.txt and in Platform.h you can see that you are re-defining it.

This needs to be fixed.

[Adam Treat]

(In reply to comment #22)
> (From update of attachment 55750 [details])
> > +IF (NOT WTF_USE_ICU_UNICODE)
> > +    ADD_DEFINITIONS(-DWTF_USE_ICU_UNICODE=0)
> 
> /home/manyoso/dev/webkit_1/JavaScriptCore/wtf/Platform.h:580:1: warning: "WTF_USE_ICU_UNICODE" redefined
> 
> This is in cross-platform wtf/CMakeLists.txt and in Platform.h you can see that you are re-defining it.
> 
> This needs to be fixed.

Ok, I've gone ahead and tried a full build, but my edje packages were not new enough.  Regardless, here is what I think needs to be fixed for review:

1) FindXXX.cmake packages for all required/optional dependencies.  I'd like to see this for both Efl specific dependencies as well as the generic cross-platform dependencies.

2) Need to be able to build without any of the optional packages. (See ICU error above)

3) Remove all port specific references from the cross-platform CMakeLists.txt... For example, I see that JavaScriptCore/wtf/CMakeLists.txt references the various ports to decide if it should include unicode.  This logic needs to be moved out as it is a recipe for spaghetti build code.  The port specific files should decide which unicode to include, etc.  Suggestions for mechanism for doing this would be helpful.  For instance, instead of the main CMakeLists.txt including Options{$PORT}... I'd rather it the other way around.  I'd rather the port specific build files including the cross-platform ones.

Those are the standout pieces (the last one might be a big change) so I'll leave it there for now.  Comments?

[Patrick R. Gansterer]

(In reply to comment #23)
> The port specific files should decide which unicode to include, etc.  Suggestions for mechanism for doing this would be helpful.  For instance, instead of the main CMakeLists.txt including Options{$PORT}... I'd rather it the other way around.  I'd rather the port specific build files including the cross-platform ones.
I think the corss-platform one should include the platform-specific, because the ADD_SUBDIRECTORY() won't work otherwise.
I don't like the INCLUDE("${PORT}"), but it might be the best solution in the moment. :-/

[Adam Treat]

(In reply to comment #24)
> (In reply to comment #23)
> > The port specific files should decide which unicode to include, etc.  Suggestions for mechanism for doing this would be helpful.  For instance, instead of the main CMakeLists.txt including Options{$PORT}... I'd rather it the other way around.  I'd rather the port specific build files including the cross-platform ones.
> I think the corss-platform one should include the platform-specific, because the ADD_SUBDIRECTORY() won't work otherwise.
> I don't like the INCLUDE("${PORT}"), but it might be the best solution in the moment. :-/

Why would we lose 'ADD_SUBDIRECTORY'... rather we'd have a top-level CMakeListsPORT.cmake file that includes the top-level cross-platform one and this could still do ADD_SUBDIRECTORY, no?

[Patrick R. Gansterer]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > The port specific files should decide which unicode to include, etc.  Suggestions for mechanism for doing this would be helpful.  For instance, instead of the main CMakeLists.txt including Options{$PORT}... I'd rather it the other way around.  I'd rather the port specific build files including the cross-platform ones.
> > I think the corss-platform one should include the platform-specific, because the ADD_SUBDIRECTORY() won't work otherwise.
> > I don't like the INCLUDE("${PORT}"), but it might be the best solution in the moment. :-/
> 
> Why would we lose 'ADD_SUBDIRECTORY'... rather we'd have a top-level CMakeListsPORT.cmake file that includes the top-level cross-platform one and this could still do ADD_SUBDIRECTORY, no?
Usualy ADD_SUBDIRECTORY uses CMakeLists.txt in the directory. Then you already have an cross-platfrom file. Why create a second one?
I think you should have only a CMakeLists.txt, sometimes a DerivedSources.cmake and a PortXXX.cmake for each port.

[Adam Treat]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > (In reply to comment #23)
> > > > The port specific files should decide which unicode to include, etc.  Suggestions for mechanism for doing this would be helpful.  For instance, instead of the main CMakeLists.txt including Options{$PORT}... I'd rather it the other way around.  I'd rather the port specific build files including the cross-platform ones.
> > > I think the corss-platform one should include the platform-specific, because the ADD_SUBDIRECTORY() won't work otherwise.
> > > I don't like the INCLUDE("${PORT}"), but it might be the best solution in the moment. :-/
> > 
> > Why would we lose 'ADD_SUBDIRECTORY'... rather we'd have a top-level CMakeListsPORT.cmake file that includes the top-level cross-platform one and this could still do ADD_SUBDIRECTORY, no?
> Usualy ADD_SUBDIRECTORY uses CMakeLists.txt in the directory. Then you already have an cross-platfrom file. Why create a second one?
> I think you should have only a CMakeLists.txt, sometimes a DerivedSources.cmake and a PortXXX.cmake for each port.

Alright, but the cross-platform ones should be limited to including the port specific files... all other port specific logic should be removed from the cross-platform ones IMO.

[Patrick R. Gansterer]

(In reply to comment #27)
> Alright, but the cross-platform ones should be limited to including the port specific files... all other port specific logic should be removed from the cross-platform ones IMO.
The plaform independent files go into the CMakeLists.txt?
- CMakeLists.txt: Declares all cross-platform source files, include DerivedSources.cmake and include Port${PORT}.cmake
- DerivedSources.cmake: Do the code generator stuff.
- PortGtk.cmake, PortQt.cmake, PortEfl.cmake: Add additional source files and required libs for the target.

In the JavaScriptCore we need a CPU dedection for the JIT: I think this can go into the CMakeLists.txt because it is not _port_ specific?

[Adam Treat]

(In reply to comment #28)
> (In reply to comment #27)
> > Alright, but the cross-platform ones should be limited to including the port specific files... all other port specific logic should be removed from the cross-platform ones IMO.
> The plaform independent files go into the CMakeLists.txt?
> - CMakeLists.txt: Declares all cross-platform source files, include DerivedSources.cmake and include Port${PORT}.cmake
> - DerivedSources.cmake: Do the code generator stuff.
> - PortGtk.cmake, PortQt.cmake, PortEfl.cmake: Add additional source files and required libs for the target.
> 
> In the JavaScriptCore we need a CPU dedection for the JIT: I think this can go into the CMakeLists.txt because it is not _port_ specific?

Close, but we'll also need FeatureFOO.cmake for features that are shared by more than one port, but not by *all* ports and then the PortFoo.cmake can determine which FeatureFOO.cmake(s) to include.

[Gustavo Sverzut Barbieri]

Guys, we partially agree with you, but it's hard to say someone is 100% right in this case. Some remarks on our side:

  - CMakeLists.txt including the ${PORT} version is easier and for this case clearer. It is not usual in programming, particular in inheritance, but in this case it does make sense as you can just add the sub directory and CMake will always look for CMakeLists.txt there, not CMakeList${PORT}.txt, then to solve this we add include and it will add directory... which is not that usual. So we're with Patrick here.

  - JIT's CPU detection in general CMakeLists.txt does make sense, at least so far. So with Patrick here as well.

  - DerivedSources does not exist in source, just in build tree, so no option to ADD_DIRECTORY(). If you mean to have it in root or inside cmake/, then I disagree as it will be confusing. So we'd rather stay with these port-independent CMakeList.txt

  - FeatureFOO.cmake is doubtful, as usually the shared bit is a single CPP definition. The dependencies will likely differ a lot, some will not even have dependencies and it is just a feature to reduce code size.  Leandro's WEBKIT_FEATURE() handles that nicely, ports can define it. If you want we could add more logic there to allow ports to define the ${DEFAULT_${FEATURE}} and maybe add a keyword to block it, in this case WEBKIT_FEATURE() could be moved to common files, with these values being defined in the port -- but we rather keep with the simpler yet efficient solution by Leandro.

[Patrick R. Gansterer]

(In reply to comment #30)
> Guys, we partially agree with you, but it's hard to say someone is 100% right in this case. Some remarks on our side:
>   - JIT's CPU detection in general CMakeLists.txt does make sense, at least so far. So with Patrick here as well.
> 
>   - DerivedSources does not exist in source, just in build tree, so no option to ADD_DIRECTORY(). If you mean to have it in root or inside cmake/, then I disagree as it will be confusing. So we'd rather stay with these port-independent CMakeList.txt
I startet to port the WebCore CMakeLists.txt to Windows. I moved most of you stuff from WebKitGenerators into the CMakeLists.txt. My idea behind a additional DerivedSources.cmake file was to remove the generators into a own file (one per library). This should be included per INCLUDE() in the CMakeLists.txt.

>   - FeatureFOO.cmake is doubtful, as usually the shared bit is a single CPP definition. The dependencies will likely differ a lot, some will not even have dependencies and it is just a feature to reduce code size.  Leandro's WEBKIT_FEATURE() handles that nicely, ports can define it. If you want we could add more logic there to allow ports to define the ${DEFAULT_${FEATURE}} and maybe add a keyword to block it, in this case WEBKIT_FEATURE() could be moved to common files, with these values being defined in the port -- but we rather keep with the simpler yet efficient solution by Leandro.
I think that this FeatureXXX files mostly define only one cpp too. So I'm not sure if this will be the best solution IMHO. I would prefere the feature dedection/definition in the PortXXX.cmake.

[Adam Treat]

(In reply to comment #30)
> Guys, we partially agree with you, but it's hard to say someone is 100% right in this case. Some remarks on our side:
> 
>   - CMakeLists.txt including the ${PORT} version is easier and for this case clearer. It is not usual in programming, particular in inheritance, but in this case it does make sense as you can just add the sub directory and CMake will always look for CMakeLists.txt there, not CMakeList${PORT}.txt, then to solve this we add include and it will add directory... which is not that usual. So we're with Patrick here.
> 
>   - JIT's CPU detection in general CMakeLists.txt does make sense, at least so far. So with Patrick here as well.
> 
>   - DerivedSources does not exist in source, just in build tree, so no option to ADD_DIRECTORY(). If you mean to have it in root or inside cmake/, then I disagree as it will be confusing. So we'd rather stay with these port-independent CMakeList.txt
> 
>   - FeatureFOO.cmake is doubtful, as usually the shared bit is a single CPP definition. The dependencies will likely differ a lot, some will not even have dependencies and it is just a feature to reduce code size.  Leandro's WEBKIT_FEATURE() handles that nicely, ports can define it. If you want we could add more logic there to allow ports to define the ${DEFAULT_${FEATURE}} and maybe add a keyword to block it, in this case WEBKIT_FEATURE() could be moved to common files, with these values being defined in the port -- but we rather keep with the simpler yet efficient solution by Leandro.

I don't want to see cross-platform CMakeLists.txt doing this:

(pseudo)
if (port)
  include unicodeicu.cpp
else
  include unicodefoo.cpp

Adam

[Patrick R. Gansterer]

(In reply to comment #32)
> I don't want to see cross-platform CMakeLists.txt doing this:
> 
> (pseudo)
> if (port)
>   include unicodeicu.cpp
> else
>   include unicodefoo.cpp

I think that this should go into the platform specific file.
I'm not sure if we realy create a own file for this 4 lines.
I don't see many places where to use this.

An other question:
For example we can dedect if the system has sqlite. This will be done with a FindSQLite in the root CMakeLists.txt. If SQLITE_FOUND is set the ENABLE_DATABES will be set. In the WebCore/CMakeLists.txt is then a IF(ENABLE_DATABSE) which add the required source files and libraries for the linker. Do you agree?

[Gustavo Sverzut Barbieri]

(In reply to comment #33)
> (In reply to comment #32)
> > I don't want to see cross-platform CMakeLists.txt doing this:
> > 
> > (pseudo)
> > if (port)
> >   include unicodeicu.cpp
> > else
> >   include unicodefoo.cpp
> 
> I think that this should go into the platform specific file.
> I'm not sure if we realy create a own file for this 4 lines.
> I don't see many places where to use this.
> 
> An other question:
> For example we can dedect if the system has sqlite. This will be done with a FindSQLite in the root CMakeLists.txt. If SQLITE_FOUND is set the ENABLE_DATABES will be set. In the WebCore/CMakeLists.txt is then a IF(ENABLE_DATABSE) which add the required source files and libraries for the linker. Do you agree?

Patrick, Please let's not go this route. It is called "automagic" to automatically enable or disable features based on the installed libraries. If we do this, we still need to have explicit/forceful disable/enable that will check the library and emit error messages. If we don't do this, systems that build packages without a clean tree (many cases, like Gentoo users) the packaging system will not know features were enabled and thus the package have dependencies!  To add both automatic and manual, it starts to be a real PITA as many features have multiple dependencies and code start to be messy (I'm saying because I had to care about it for EFL and Gentoo packages, it was troublesome).

[Patrick R. Gansterer]

(In reply to comment #34)
> Patrick, Please let's not go this route. It is called "automagic" to automatically enable or disable features based on the installed libraries. If we do this, we still need to have explicit/forceful disable/enable that will check the library and emit error messages. If we don't do this, systems that build packages without a clean tree (many cases, like Gentoo users) the packaging system will not know features were enabled and thus the package have dependencies!  To add both automatic and manual, it starts to be a real PITA as many features have multiple dependencies and code start to be messy (I'm saying because I had to care about it for EFL and Gentoo packages, it was troublesome).

No, no! I don't like the idea of "automagic" too! I think I wrote it badly. What I meant was sth like this:

root/CMakeLists.txt:

FIND_PACKAGE(SQLite) # No REQUIRED

IF (USER_REQUEST_ENABLE_DATABASE)
    IF (NOT ${SQLITE_FOUND})
        MESSAGE(FATAL_ERROR "You need SQLite")
    ENDIF ()
    SET(ENABLE_DATABASE 1)
ELSE ()
    SET(ENABLE_DATABASE 0)
ENDIF()

root/WebCore/CMakeLists.txt:

IF (ENABLE_DATABASE)
    LIST(APPEND WebCore_SOURCES ...)
    LIST(APPEND WebCore_LIBRARIES ${SQLITE_LIBRARY})
ENDIF ()


Maybe it's also possible to change the root CMakeLists.txt to:
SET(ENABLE_DATABASE ${USER_REQUEST_ENABLE_DATABASE}) # You macro does better
IF (ENABLE_DATABASE)
    FIND_PACKAGE(SQLite REQUIRED)
ENDIF()

I think the 3rdpary library dection has to be done in the root CMakeLists.txt (or a FindStuff.cmake which is included in root CMakeLists.txt)

[Gustavo Sverzut Barbieri]

(In reply to comment #35)
> (In reply to comment #34)
> > Patrick, Please let's not go this route. It is called "automagic" to automatically enable or disable features based on the installed libraries. If we do this, we still need to have explicit/forceful disable/enable that will check the library and emit error messages. If we don't do this, systems that build packages without a clean tree (many cases, like Gentoo users) the packaging system will not know features were enabled and thus the package have dependencies!  To add both automatic and manual, it starts to be a real PITA as many features have multiple dependencies and code start to be messy (I'm saying because I had to care about it for EFL and Gentoo packages, it was troublesome).
> 
> No, no! I don't like the idea of "automagic" too! I think I wrote it badly. What I meant was sth like this:
> 
> root/CMakeLists.txt:
> 
> FIND_PACKAGE(SQLite) # No REQUIRED
> 
> IF (USER_REQUEST_ENABLE_DATABASE)
>     IF (NOT ${SQLITE_FOUND})
>         MESSAGE(FATAL_ERROR "You need SQLite")
>     ENDIF ()
>     SET(ENABLE_DATABASE 1)
> ELSE ()
>     SET(ENABLE_DATABASE 0)
> ENDIF()
> 
> root/WebCore/CMakeLists.txt:
> 
> IF (ENABLE_DATABASE)
>     LIST(APPEND WebCore_SOURCES ...)
>     LIST(APPEND WebCore_LIBRARIES ${SQLITE_LIBRARY})
> ENDIF ()
> 
> 
> Maybe it's also possible to change the root CMakeLists.txt to:
> SET(ENABLE_DATABASE ${USER_REQUEST_ENABLE_DATABASE}) # You macro does better
> IF (ENABLE_DATABASE)
>     FIND_PACKAGE(SQLite REQUIRED)
> ENDIF()
> 
> I think the 3rdpary library dection has to be done in the root CMakeLists.txt (or a FindStuff.cmake which is included in root CMakeLists.txt)

No, no. Then you go back to what Adam and we complained. SQLite is a bad example as it is used by all ports, but consider a feature that depends on LibA on EFL, LibB for GTK, LibC for Windows... you'd have to have IF() to check the platform -- which is really bad.

We'll fix the agreed points so far and send a new version. Let's hope it is accepted.

[Leandro Pereira]

Created attachment 56093 [details]
CMake build system, with EFL port dependent files

This updated version:

- Uses various FindXXX files instead of pkgconfig.
- Lets each port choose which files to include in their port-specific CMakeLists${PORT}.txt files.
- Prints a summary of each enabled feature.
- Changes WebCore's and JavaScriptCore's config.h so they look for a cmakeconfig.h file, replacing -DXXX in compiler's command line.

[Patrick R. Gansterer]

> +CMAKE_MINIMUM_REQUIRED(VERSION 2.8)
Do we realy need 2.8?


> +FIND_PACKAGE(ICU)
> +FIND_PACKAGE(Threads)
If we move them into the plaform files, they have to be removed in the root CMakeLists.txt


> +INSTALL(TARGETS ${WebCore_LIBRARY_NAME} DESTINATION lib)
I would remove the INSTALL calls in the moment.


> +ADD_TARGET_PROPERTIES(${WebKit_LIBRARY_NAME} LINK_FLAGS ${WebKit_LINK_FLAGS})
Do you realy need the linker flags here? I don't like the idea of per target linker flags. If EFL needs special linker flags they should be declared in the OptionsEfl.cmake with CMAKE_SHARED_LINKER_FLAGS/CMAKE_EXE_LINKER_FLAGS.


> +IF (NOT ICU_FOUND)
> +    MESSAGE(FATAL_ERROR "ICU is required for EFL port")
> +ENDIF (NOT ICU_FOUND)
Should become FIND_PACKAGE(ICU REQUIRED) here.


> +SET(JSC_EXECUTABLE_NAME jsc)
> +SET(WTF_LIBRARY_NAME wtf)
> +SET(JavaScriptCore_LIBRARY_NAME javascriptcore)
> +SET(WebCore_LIBRARY_NAME webcore)
> +SET(WebKit_LIBRARY_NAME ewebkit)
> +
> +SET(WTF_LIBRARY_TYPE STATIC)
> +SET(JavaScriptCore_LIBRARY_TYPE STATIC)
> +SET(WebCore_LIBRARY_TYPE STATIC)
> +SET(WebKit_LIBRARY_TYPE SHARED)
I think we should declare them in the root CMakeLists.txt and overrided them only in the port files.


Can you please remove the unnecessary FindXXX files. Many of them will be installed by CMake already. If we want do depend on CMake 2.8 we can remove even more.

What about coding style?
I would prefere space only between IF, ENDIF, FOREACH and ENDFOREACH. The END should have empty braces. All other keyworks should have no space between (SET, LIST, ...). Additionaly we should use a 4 space intention like the other WebKit code does.

[Patrick R. Gansterer]

> +ADD_SUBDIRECTORY(wtf)
> +ADD_SUBDIRECTORY(jsc)
I would prefere ADD_SUBDIRECTORY(JavaScriptCore/wtf) ADD_SUBDIRECTORY(JavaScriptCore/jsc) in the root CMakeLists.txt instead.

> cmake/FindEFL.cmake
Isn't it posisble to find the libraries with the usual FIND_LIBRARY?

[Leandro Pereira]

Created attachment 56111 [details]
CMake build system, with EFL port dependent files

New patch.

- Style issues fixed.
- s/JSC_DIR/JAVASCRIPTCORE_DIR/ for clarity (as jsc is a directory and a target)
- Move ICU/Thread checks to OptionsEfl

[Adam Treat]

Comment on attachment 56111 [details]
CMake build system, with EFL port dependent files

Overall, I like this version much much better and think it is a good first step for a cmake buildsystem that can grow to include more ports.  My other big style nitpick is the choice of UPPERCASE vs lowercase, but I'm not willing to r- on this account alone as lots of cmake buildsystems have this.

[Adam Treat]

(In reply to comment #41)
> (From update of attachment 56111 [details])
> Overall, I like this version much much better and think it is a good first step for a cmake buildsystem that can grow to include more ports.  My other big style nitpick is the choice of UPPERCASE vs lowercase, but I'm not willing to r- on this account alone as lots of cmake buildsystems have this.

When landing you need to be sure to check the buildbots to make sure of no unintended problems with other ports.  Thank you!

[WebKit Commit Bot]

Comment on attachment 56111 [details]
CMake build system, with EFL port dependent files

Clearing flags on attachment: 56111

Committed r59537: <http://trac.webkit.org/changeset/59537>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/59537 might have broken Qt Linux ARMv7 Release