Bug 56705 – [CMAKE] Refactoring CMakeLists${PORT}.txt to Platform${PORT}.cmake

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ryuan Choi]

As Patric mentioned in Bug 56624,
Platform${PORT}.cmake looks better than CMakeLists${PORT}.txt

[Ryuan Choi]

Created attachment 86256 [details]
Patch

[Ryuan Choi]

Hello Lucas and Leandro, Gyuyoung

How do you think about this patch ?

[Gyuyoung Kim]

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.

[Patrick R. Gansterer]

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

[Ryuan Choi]

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

[Ryuan Choi]

Created attachment 87095 [details]
Patch

[Ryuan Choi]

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

[Patrick R. Gansterer]

Comment on attachment 87095 [details]
Patch

LGTM, but I like this to be committed via svn to keep history.

[Adam Roben (:aroben)]

Comment on attachment 87095 [details]
Patch

If Patrick likes it and it builds, it seems fine to me.

[Lucas De Marchi]

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?

[Patrick R. Gansterer]

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

[Lucas De Marchi]

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

[Ryuan Choi]

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

[Patrick R. Gansterer]

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

[Lucas De Marchi]

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

[Eric Seidel (no email)]

Ryuan isn't a committer, but this was cq-'d.  Does it need a r-, or is someone going to land this?

[Lucas De Marchi]

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

[Patrick R. Gansterer]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Patrick R. Gansterer]

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

[Lucas De Marchi]

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

[Ryuan Choi]

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

[Ryuan Choi]

Created attachment 114498 [details]
Patch

[WebKit Review Bot]

Comment on attachment 114498 [details]
Patch

Clearing flags on attachment: 114498

Committed r99866: <http://trac.webkit.org/changeset/99866>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.