Bug 56705

Summary: [CMAKE] Refactoring CMakeLists${PORT}.txt to Platform${PORT}.cmake
Product: WebKit Reporter: Ryuan Choi <ryuan.choi>
Component: WebCore Misc.Assignee: Ryuan Choi <ryuan.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: antognolli+webkit, eric, gwright, gyuyoung.kim, kenneth, leandro, levin+threading, lucas.de.marchi, paroga, rakuco, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Ryuan Choi 2011-03-18 23:10:13 PDT
As Patric mentioned in Bug 56624,
Platform${PORT}.cmake looks better than CMakeLists${PORT}.txt
Comment 1 Ryuan Choi 2011-03-18 23:17:08 PDT
Created attachment 86256 [details]
Patch
Comment 2 Ryuan Choi 2011-03-23 18:31:17 PDT
Hello Lucas and Leandro, Gyuyoung

How do you think about this patch ?
Comment 3 Gyuyoung Kim 2011-03-23 18:46:31 PDT
CMakeLists.txt is still .txt extension. I think CMakeLists.txt needs to be changed as well. 

IMHO, CMakeLists name needs to be changed in order to consist with PlatformEfl.cmake.
Comment 4 Patrick R. Gansterer 2011-03-24 01:22:09 PDT
(In reply to comment #3)
> CMakeLists.txt is still .txt extension. I think CMakeLists.txt needs to be changed as well.
NO!!! You usually have _one_ CMakeLists.txt file per directory, for the ADD_SUBDIRECTORY calls. Because I agree that CMakesLists.txt will be very big if we put all files (including platform specific) into it, it's ok for me to put the platform specific files into separate files (with the cmake extension). Having a CMakeLists{PORT}.txt looks weird to people familiar with CMake.

One additional point is that some files (e.g. network layer) are shared between different ports. So if we e.g. switch the GTK port to CMake we have many files in PlatformEfl and PlatformGtk. But that's an other problem.

Can you make the same change for the other targets (WTF, JavaSCriptCore, ...) too?
Comment 5 Ryuan Choi 2011-03-24 02:08:29 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > CMakeLists.txt is still .txt extension. I think CMakeLists.txt needs to be changed as well.
> NO!!! You usually have _one_ CMakeLists.txt file per directory, for the ADD_SUBDIRECTORY calls. Because I agree that CMakesLists.txt will be very big if we put all files (including platform specific) into it, it's ok for me to put the platform specific files into separate files (with the cmake extension). Having a CMakeLists{PORT}.txt looks weird to people familiar with CMake.
> 
> One additional point is that some files (e.g. network layer) are shared between different ports. So if we e.g. switch the GTK port to CMake we have many files in PlatformEfl and PlatformGtk. But that's an other problem.
> 
> Can you make the same change for the other targets (WTF, JavaSCriptCore, ...) too?

Sure, I'll investigate it for the other targets.
Comment 6 Ryuan Choi 2011-03-27 18:45:41 PDT
Created attachment 87095 [details]
Patch
Comment 7 Ryuan Choi 2011-03-27 18:47:30 PDT
(In reply to comment #6)
> Created an attachment (id=87095) [details]
> Patch

In addition to CMakeLists${PORT}.txt in WebCore/, 
I refactored them in JavaScriptCore and JavaScriptCore/wtf.
Comment 8 Patrick R. Gansterer 2011-03-27 18:53:51 PDT
Comment on attachment 87095 [details]
Patch

LGTM, but I like this to be committed via svn to keep history.
Comment 9 Adam Roben (:aroben) 2011-04-26 16:26:58 PDT
Comment on attachment 87095 [details]
Patch

If Patrick likes it and it builds, it seems fine to me.
Comment 10 Lucas De Marchi 2011-04-26 16:59:28 PDT
Patrick: why do we want this? 

As of now if I want to see all the ports using cmake, I just do:

$ ls -l CMakeLists*
-rw-r--r-- 1 lucas users  8034 Apr 26 16:01 CMakeListsEfl.txt
-rw-r--r-- 1 lucas users 73771 Apr 26 16:01 CMakeLists.txt
-rw-r--r-- 1 lucas users  4491 Mar 31 08:11 CMakeListsWinCE.txt


Now I'd need to grep the .cmake files too. What's the point with renaming them?
Comment 11 Patrick R. Gansterer 2011-04-26 23:01:25 PDT
(In reply to comment #10)
> Patrick: why do we want this? 

To make the build system more CMake style.
You usually have _one_ CMakeLists.txt in you build directory. Additional stuff is usually in "XXX.cmake" files.
See the source of CMake itself: http://cmake.org/gitweb?p=cmake.git;a=tree
Many editors will provide correct syntax highlighting, because most of them match "CMakeLists.txt|*.cmake".

> Now I'd need to grep the .cmake files too. What's the point with renaming them?

When do you grep the files? Why?
Comment 12 Lucas De Marchi 2011-04-28 15:31:55 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > Patrick: why do we want this? 
> 
> To make the build system more CMake style.
> You usually have _one_ CMakeLists.txt in you build directory. Additional stuff is usually in "XXX.cmake" files.
> See the source of CMake itself: http://cmake.org/gitweb?p=cmake.git;a=tree

But there they are additional "logical" stuff to the build system. Here they are additional _file lists_ to be added.


> Many editors will provide correct syntax highlighting, because most of them match "CMakeLists.txt|*.cmake".

Here Vim does provides the highlighting in CMakeListsEfl.txt. It's just a matter of configuring another pattern in you editor's options.

> 
> > Now I'd need to grep the .cmake files too. What's the point with renaming them?
> 
> When do you grep the files? Why?

Usually when I'm checking what files are in each list, i.e. when I'm figuring out why it works in GTK (then I grep Makefile.am files) and not in EFL (then I grep CMakeLists*.txt file)... or vice-versa.

But my point here is that they are file lists. For me it makes sense to name them the way they are now.
Comment 13 Ryuan Choi 2011-04-28 18:10:22 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Patrick: why do we want this? 
> > 
> > To make the build system more CMake style.
> > You usually have _one_ CMakeLists.txt in you build directory. Additional stuff is usually in "XXX.cmake" files.
> > See the source of CMake itself: http://cmake.org/gitweb?p=cmake.git;a=tree
> 
> But there they are additional "logical" stuff to the build system. Here they are additional _file lists_ to be added.
> 
This bug have little history from Bug 56624.
I already add UseJSC.cmake for JSC and prepare UseV8.cmake.
those files are also additional file lists to be added as an option(now UseJSC only supported).

IMO, we can give more information using Platform${PORT}.cmake and Use${OPTION}.cmake instead of CMakeLists${PORT}.txt and CMakeLists${OPTION}.txt

> 
> > Many editors will provide correct syntax highlighting, because most of them match "CMakeLists.txt|*.cmake".
> 
> Here Vim does provides the highlighting in CMakeListsEfl.txt. It's just a matter of configuring another pattern in you editor's options.
> 
Strange, Vim on My PC can't

> > 
> > > Now I'd need to grep the .cmake files too. What's the point with renaming them?
> > 
> > When do you grep the files? Why?
> 
> Usually when I'm checking what files are in each list, i.e. when I'm figuring out why it works in GTK (then I grep Makefile.am files) and not in EFL (then I grep CMakeLists*.txt file)... or vice-versa.
> 
> But my point here is that they are file lists. For me it makes sense to name them the way they are now.

Now, We should search CMakeLists.txt, CMakeLists${PORT}.txt and UseJSC.cmake(and UseV8.cmake if needed) for checking all files to build.
Comment 14 Patrick R. Gansterer 2011-04-28 18:52:19 PDT
(In reply to comment #12)
> > To make the build system more CMake style.
> > You usually have _one_ CMakeLists.txt in you build directory. Additional stuff is usually in "XXX.cmake" files.
> > See the source of CMake itself: http://cmake.org/gitweb?p=cmake.git;a=tree
> 
> But there they are additional "logical" stuff to the build system. Here they are additional _file lists_ to be added.

I don't agree on this. http://trac.webkit.org/browser/trunk/Source/WebCore/CMakeListsEfl.txt isn't a simple file list. There are _conditional_ lists (e.g. IF(WTF_USE_CAIRO)) and it also contains a ADD_DEFINITIONS. http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/CMakeListsWinCE.txt defines the generation of the asm files.

> > Many editors will provide correct syntax highlighting, because most of them match "CMakeLists.txt|*.cmake".
> 
> Here Vim does provides the highlighting in CMakeListsEfl.txt. It's just a matter of configuring another pattern in you editor's options.
You're right, but I wanted to give additional examples for using a more common CMake style. I never seen a CMakeListsXY.txt before, but *.cmake files are very common.
Comment 15 Lucas De Marchi 2011-04-29 07:10:52 PDT
(In reply to comment #14)
> http://trac.webkit.org/browser/trunk/Source/WebCore/CMakeListsEfl.txt isn't a simple file list. There are _conditional_ lists (e.g. IF(WTF_USE_CAIRO)) and it also contains a ADD_DEFINITIONS. 

IMO conditional lists are fine because they are still lists. Where to turn on and off the options (the logic) is still in another file, which seems right.

Maybe the ADD_DEFINITIONS would be a thing to put in another file.

> http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/CMakeListsWinCE.txt defines the generation of the asm files.

Same here.
Comment 16 Eric Seidel (no email) 2011-06-18 19:58:05 PDT
Ryuan isn't a committer, but this was cq-'d.  Does it need a r-, or is someone going to land this?
Comment 17 Lucas De Marchi 2011-06-19 12:51:14 PDT
(In reply to comment #16)
> Ryuan isn't a committer, but this was cq-'d.  Does it need a r-, or is someone going to land this?

It has my informal r-, but it's up to Patrick to decide.
Comment 18 Patrick R. Gansterer 2011-06-20 01:35:08 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > Ryuan isn't a committer, but this was cq-'d.  Does it need a r-, or is someone going to land this?
> 
> It has my informal r-, but it's up to Patrick to decide.

I still like this change, but don't want to overrule Lucas. Maybe there is anyone else with deeper CMake experience to decide over the more CMake-like solution?

In my personal CMake history I only had _one_ CMakeLists.txt per folder. The rest was done in *.cmake files.

Not a very strong argument, but IMHO this will scale better (better readable, wihout the "CMakeLists"-prefix): The different Windows ports have many files which are shared in very different ways, so the PlatformXXX won't be sufficient if we don't want to put the same file lists in different PlatformXXX files.


> > Many editors will provide correct syntax highlighting, because most of them match "CMakeLists.txt|*.cmake".
> Here Vim does provides the highlighting in CMakeListsEfl.txt. It's just a matter of configuring another pattern in you editor's options.
It's possible to rename all your *.cpp to Cpp*.txt too and it will work. "It's just a matter of configuring another pattern in you editor's options.", but why you don't do it? The editors are shipped with most common patters, so why we won't use them? IMO the editor patterns are a good source for what people usually do. :-)
Comment 19 Raphael Kubo da Costa (:rakuco) 2011-11-10 04:51:09 PST
Hey there.

Are we going to do something with this bug or can we close it once and for all? Looking at the previous comments, in the end it all boils down to a matter of style (FWIW, I am also used to seeing only one CMakeLists.txt per directory).
Comment 20 Patrick R. Gansterer 2011-11-10 05:22:31 PST
(In reply to comment #19)
> Are we going to do something with this bug or can we close it once and for all? Looking at the previous comments, in the end it all boils down to a matter of style (FWIW, I am also used to seeing only one CMakeLists.txt per directory).

Yes, it's only a matter of style. I'd still like to commit this, if Lucas is ok with it too.
Comment 21 Lucas De Marchi 2011-11-10 06:02:22 PST
(In reply to comment #20)
> (In reply to comment #19)
> > Are we going to do something with this bug or can we close it once and for all? Looking at the previous comments, in the end it all boils down to a matter of style (FWIW, I am also used to seeing only one CMakeLists.txt per directory).
> 
> Yes, it's only a matter of style. I'd still like to commit this, if Lucas is ok with it too.

I don't like it that much, but I'm fine with it.

Let's commit it.  Ryuan Choi, can you rebase it? Otherwise I can do.
Comment 22 Ryuan Choi 2011-11-10 06:07:33 PST
(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > Are we going to do something with this bug or can we close it once and for all? Looking at the previous comments, in the end it all boils down to a matter of style (FWIW, I am also used to seeing only one CMakeLists.txt per directory).
> > 
> > Yes, it's only a matter of style. I'd still like to commit this, if Lucas is ok with it too.
> 
> I don't like it that much, but I'm fine with it.
> 
> Let's commit it.  Ryuan Choi, can you rebase it? Otherwise I can do.

OK. I'll rebase and push it.
Comment 23 Ryuan Choi 2011-11-10 07:22:53 PST
Created attachment 114498 [details]
Patch
Comment 24 WebKit Review Bot 2011-11-10 09:05:35 PST
Comment on attachment 114498 [details]
Patch

Clearing flags on attachment: 114498

Committed r99866: <http://trac.webkit.org/changeset/99866>
Comment 25 WebKit Review Bot 2011-11-10 09:05:41 PST
All reviewed patches have been landed.  Closing bug.