Bug 72685 – [CMake] Move the top-level logic to the top-level directory.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Raphael Kubo da Costa (:rakuco)]

[CMake] Move the top-level logic to the top-level directory.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 115724 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 115726 [details]
Same patch, fix CMAKE_SOURCE_DIR in WebKit2 too

[Patrick R. Gansterer]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

Created attachment 115788 [details]
Follow Patrick's suggestions

[Gyuyoung Kim]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

Created attachment 115828 [details]
Rebase on top of Patrick's recent CMake commits

[Gyuyoung Kim]

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

[Brent Fulgham]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Patrick R. Gansterer]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

Created attachment 116210 [details]
Mark bfulgham as reviewer, rebase and adjust the blackberry files

[Gyuyoung Kim]

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

[Patrick R. Gansterer]

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>

[Patrick R. Gansterer]

All reviewed patches have been landed.  Closing bug.