Bug 72685 - [CMake] Move the top-level logic to the top-level directory.
Summary: [CMake] Move the top-level logic to the top-level directory.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raphael Kubo da Costa (:rakuco)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-17 18:35 PST by Raphael Kubo da Costa (:rakuco)
Modified: 2011-11-23 01:08 PST (History)
4 users (show)

See Also:


Attachments
Patch (25.16 KB, patch)
2011-11-17 18:51 PST, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Same patch, fix CMAKE_SOURCE_DIR in WebKit2 too (26.29 KB, patch)
2011-11-17 19:33 PST, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Follow Patrick's suggestions (26.08 KB, patch)
2011-11-18 05:35 PST, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Rebase on top of Patrick's recent CMake commits (26.11 KB, patch)
2011-11-18 10:18 PST, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Mark bfulgham as reviewer, rebase and adjust the blackberry files (28.72 KB, patch)
2011-11-22 05:30 PST, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raphael Kubo da Costa (:rakuco) 2011-11-17 18:35:56 PST
[CMake] Move the top-level logic to the top-level directory.
Comment 1 Raphael Kubo da Costa (:rakuco) 2011-11-17 18:51:01 PST
Created attachment 115724 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2011-11-17 18:57:41 PST
Rationale:

This is related to bug 72684 -- the build started to break on my ArchLinux box after Valgrind was upgraded to 3.7.0, andthis version started shipping /usr/include/valgrind/config.h.

Bug 72684 fixes the actual include order in EFL's DumpRenderTree, however Tools/CMakeListsEfl.txt has all the code to build EWebLauncher (including adding the required includes) and then uses INCLUDE() to add Tools/DumpRenderTree/efl/CMakeLists.txt, so the external include paths from EWebLauncher will always come before the ones added by DRT and the build will still break.

Since I was already at this, I found it would be better to fix it in a more general way.
Comment 3 Raphael Kubo da Costa (:rakuco) 2011-11-17 19:33:31 PST
Created attachment 115726 [details]
Same patch, fix CMAKE_SOURCE_DIR in WebKit2 too
Comment 4 Patrick R. Gansterer 2011-11-18 00:20:38 PST
Comment on attachment 115726 [details]
Same patch, fix CMAKE_SOURCE_DIR in WebKit2 too

View in context: https://bugs.webkit.org/attachment.cgi?id=115726&action=review

> Tools/CMakeLists.txt:1
> +INCLUDE_IF_EXISTS(${TOOLS_DIR}/CMakeLists${PORT}.txt)

What about changing this to a simple 
IF($PORT EQUALS EFL)
   ADD_SUBDIRECTORY(...)
ELSEIF ($PORT EQUALS WinCE)
  ADD_SUBDIRECTORY
ENDIF ()

IMHO that will be simpler, uses only CMakeLists.txt (without port) and does not pollute the Tools dir with a many 1-3 line files.

> Tools/DumpRenderTree/efl/CMakeLists.txt:126
> +SET_TARGET_PROPERTIES(Programs/DumpRenderTree PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}")

i don't see how this change is related to the bug title

> Tools/DumpRenderTree/efl/CMakeLists.txt:131
> +SET_TARGET_PROPERTIES(Programs/ImageDiff PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}")

ditto

> Tools/EWebLauncher/CMakeLists.txt:35
> +  LIST(APPEND EWebLauncher_LIBRARIES

please use 4 space ident

> Tools/EWebLauncher/CMakeLists.txt:44
> +  LIST(APPEND EWebLauncher_LIBRARIES ${LIBSOUP24_LIBRARIES})
> +  LIST(APPEND EWebLauncher_LINK_FLAGS ${LIBSOUP24_LDFLAGS})

4 space ident

> Tools/EWebLauncher/CMakeLists.txt:49
> +  LIST(APPEND EWebLauncher_LIBRARIES ${CURL_LIBRARIES})
> +  LIST(APPEND EWebLauncher_LINK_FLAGS ${CURL_LDFLAGS})

4 space ident
Comment 5 Raphael Kubo da Costa (:rakuco) 2011-11-18 05:00:46 PST
(In reply to comment #4)
> > Tools/DumpRenderTree/efl/CMakeLists.txt:126
> > +SET_TARGET_PROPERTIES(Programs/DumpRenderTree PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}")
> 
> i don't see how this change is related to the bug title
> 
> > Tools/DumpRenderTree/efl/CMakeLists.txt:131
> > +SET_TARGET_PROPERTIES(Programs/ImageDiff PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}")
> 
> ditto

Now that these directories are being added via ADD_SUBDIRECTORY() instead of being indirectly INCLUDE()d all the way from Source/, by default the compiled binaries were being put in ${CMAKE_BINARY_DIR}/Tools/DumpRenderTree/efl/Programs/{DumpRenderTree,ImageDiff} (the same happened to EWebLauncher and WinCELauncher with their respective paths). Since the current DumpRenderTree code expects these programs to be in ${CMAKE_BINARY_DIR}/Programs (and it is much easier to find them there while working on layout stuff), I had to set RUNTIME_OUTPUT_DIRECTORY.
Comment 6 Raphael Kubo da Costa (:rakuco) 2011-11-18 05:35:04 PST
Created attachment 115788 [details]
Follow Patrick's suggestions
Comment 7 Gyuyoung Kim 2011-11-18 05:38:26 PST
Comment on attachment 115788 [details]
Follow Patrick's suggestions

Attachment 115788 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10517089
Comment 8 Raphael Kubo da Costa (:rakuco) 2011-11-18 05:43:50 PST
(In reply to comment #7)
> (From update of attachment 115788 [details])
> Attachment 115788 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/10517089

This part of the patch has not changed, it looks like the buildbot is not cleaning the build directory and is thus trying to reuse Source/CMakeLists.txt as the top-level build file.
Comment 9 Raphael Kubo da Costa (:rakuco) 2011-11-18 10:18:56 PST
Created attachment 115828 [details]
Rebase on top of Patrick's recent CMake commits
Comment 10 Gyuyoung Kim 2011-11-18 10:21:41 PST
Comment on attachment 115828 [details]
Rebase on top of Patrick's recent CMake commits

Attachment 115828 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10495160
Comment 11 Brent Fulgham 2011-11-18 10:46:31 PST
Comment on attachment 115828 [details]
Rebase on top of Patrick's recent CMake commits

View in context: https://bugs.webkit.org/attachment.cgi?id=115828&action=review

Looks good.  Holding cq+ until buildbot maintainer can update build configuration.

> Source/JavaScriptCore/CMakeLists.txt:20
> +    "${CMAKE_SOURCE_DIR}/Source"

It seems funny that CMAKE_SOURCE_DIR doesn't have Source as part of its path.  I guess it's really more a "CMAKE_ROOT_BUILD_DIR"?
Comment 12 Raphael Kubo da Costa (:rakuco) 2011-11-21 04:41:40 PST
(In reply to comment #11)
> (From update of attachment 115828 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115828&action=review
> 
> Looks good.  Holding cq+ until buildbot maintainer can update build configuration.

Patrick, do you know how the issue was solved when Source/ was created? Did someone just clean the bot's build dir after the commit?

> > Source/JavaScriptCore/CMakeLists.txt:20
> > +    "${CMAKE_SOURCE_DIR}/Source"
> 
> It seems funny that CMAKE_SOURCE_DIR doesn't have Source as part of its path.  I guess it's really more a "CMAKE_ROOT_BUILD_DIR"?

That's a variable defined by CMake itself and points to the root of the source directories (in our case, the top-level directory where the main CMakeLists.txt is).
Comment 13 Patrick R. Gansterer 2011-11-21 07:23:02 PST
(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 115828 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=115828&action=review
> > 
> > Looks good.  Holding cq+ until buildbot maintainer can update build configuration.
> 
> Patrick, do you know how the issue was solved when Source/ was created? Did someone just clean the bot's build dir after the commit?

Yes, both WinCE and EFL build dir have been clean up before.
Comment 14 Raphael Kubo da Costa (:rakuco) 2011-11-21 08:51:00 PST
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (From update of attachment 115828 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=115828&action=review
> > > 
> > > Looks good.  Holding cq+ until buildbot maintainer can update build configuration.
> > 
> > Patrick, do you know how the issue was solved when Source/ was created? Did someone just clean the bot's build dir after the commit?
> 
> Yes, both WinCE and EFL build dir have been clean up before.

Alright, so is it OK to rebase the patch, have it committed and have the bot clean up afterwards?
Comment 15 Raphael Kubo da Costa (:rakuco) 2011-11-22 05:30:37 PST
Created attachment 116210 [details]
Mark bfulgham as reviewer, rebase and adjust the blackberry files
Comment 16 Gyuyoung Kim 2011-11-22 05:58:28 PST
Comment on attachment 116210 [details]
Mark bfulgham as reviewer, rebase and adjust the blackberry files

Attachment 116210 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10548174
Comment 17 Patrick R. Gansterer 2011-11-23 01:08:39 PST
Comment on attachment 116210 [details]
Mark bfulgham as reviewer, rebase and adjust the blackberry files

Clearing flags on attachment: 116210

Committed r101052: <http://trac.webkit.org/changeset/101052>
Comment 18 Patrick R. Gansterer 2011-11-23 01:08:49 PST
All reviewed patches have been landed.  Closing bug.