Bug 56705

Created attachment 86256 [details] Patch

Hello Lucas and Leandro, Gyuyoung How do you think about this patch ?

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.

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

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

Created attachment 87095 [details] Patch

(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 on attachment 87095 [details] Patch LGTM, but I like this to be committed via svn to keep history.

Comment on attachment 87095 [details] Patch If Patrick likes it and it builds, it seems fine to me.

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?

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

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

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

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

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

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

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

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

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

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

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

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

Created attachment 114498 [details] Patch

Comment on attachment 114498 [details] Patch Clearing flags on attachment: 114498 Committed r99866: <http://trac.webkit.org/changeset/99866>

All reviewed patches have been landed. Closing bug.