Bug 56705 - [CMAKE] Refactoring CMakeLists${PORT}.txt to Platform${PORT}.cmake
: [CMAKE] Refactoring CMakeLists${PORT}.txt to Platform${PORT}.cmake
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-03-18 23:10 PST by
Modified: 2011-11-10 09:05 PST (History)


Attachments
Patch (27.17 KB, patch)
2011-03-18 23:17 PST, Ryuan Choi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (36.98 KB, patch)
2011-03-27 18:45 PST, Ryuan Choi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (42.70 KB, patch)
2011-11-10 07:22 PST, Ryuan Choi
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


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

How do you think about this patch ?
------- Comment #3 From 2011-03-23 18:46:31 PST -------
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 From 2011-03-24 01:22:09 PST -------
(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 From 2011-03-24 02:08:29 PST -------
(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 From 2011-03-27 18:45:41 PST -------
Created an attachment (id=87095) [details]
Patch
------- Comment #7 From 2011-03-27 18:47:30 PST -------
(In reply to comment #6)
> Created an attachment (id=87095) [details] [details]
> Patch

In addition to CMakeLists${PORT}.txt in WebCore/, 
I refactored them in JavaScriptCore and JavaScriptCore/wtf.
------- Comment #8 From 2011-03-27 18:53:51 PST -------
(From update of attachment 87095 [details])
LGTM, but I like this to be committed via svn to keep history.
------- Comment #9 From 2011-04-26 16:26:58 PST -------
(From update of attachment 87095 [details])
If Patrick likes it and it builds, it seems fine to me.
------- Comment #10 From 2011-04-26 16:59:28 PST -------
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 From 2011-04-26 23:01:25 PST -------
(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 From 2011-04-28 15:31:55 PST -------
(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 From 2011-04-28 18:10:22 PST -------
(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 From 2011-04-28 18:52:19 PST -------
(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 From 2011-04-29 07:10:52 PST -------
(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 From 2011-06-18 19:58:05 PST -------
Ryuan isn't a committer, but this was cq-'d.  Does it need a r-, or is someone going to land this?
------- Comment #17 From 2011-06-19 12:51:14 PST -------
(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 From 2011-06-20 01:35:08 PST -------
(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 From 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 From 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 From 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 From 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 From 2011-11-10 07:22:53 PST -------
Created an attachment (id=114498) [details]
Patch
------- Comment #24 From 2011-11-10 09:05:35 PST -------
(From update of attachment 114498 [details])
Clearing flags on attachment: 114498

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