Bug 77425 - [CMAKE] Support javascriptcore test for EFL port
Summary: [CMAKE] Support javascriptcore test for EFL port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
: 77426 (view as bug list)
Depends on:
Blocks: 77426
  Show dependency treegraph
 
Reported: 2012-01-31 04:36 PST by Gyuyoung Kim
Modified: 2012-02-05 01:36 PST (History)
4 users (show)

See Also:


Attachments
Patch (4.97 KB, patch)
2012-01-31 04:39 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (4.01 KB, patch)
2012-01-31 08:03 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (4.08 KB, patch)
2012-01-31 20:12 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (4.13 KB, patch)
2012-02-01 04:51 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (3.21 KB, patch)
2012-02-04 22:59 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2012-01-31 04:36:36 PST
Efl and WinCE as well as Blackberry port are now using Cmake as its build system and they are share the make file to create jsc excutable. In order to run "run-javascriptcore-tests", EFL port needs to change jsc installation configuration with executable output directory(e.g. Programs). So, this patch moves jsc installation configuration to Platform*.cmake.
Comment 1 Gyuyoung Kim 2012-01-31 04:39:05 PST
Created attachment 124707 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-31 04:41:28 PST
Attachment 124707 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Gyuyoung Kim 2012-01-31 04:50:49 PST
CC'ing Daniel, Patric and Kubo.

BTW, it looks style bot is strange now.
Comment 4 Raphael Kubo da Costa (:rakuco) 2012-01-31 05:22:19 PST
Comment on attachment 124707 [details]
Patch

Most of the code you have moved to the Platform files is still common to all ports, so I see little value in duplicating it all around. You could change the value of JSC_EXECUTABLE_NAME in Source/cmake/OptionsEfl.cmake to "Programs/jsc_efl" and conditionally call SET_TARGET_PROPERTIES(... RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}") if you're building the EFL port. Alternatively, you could have PlatformEfl.cmake (or OptionsEfl.cmake) set an option which defines whether programs should have their runtime output dir set to the base binary directory.
Comment 5 Gyuyoung Kim 2012-01-31 07:57:33 PST
*** Bug 77426 has been marked as a duplicate of this bug. ***
Comment 6 Gyuyoung Kim 2012-01-31 08:03:47 PST
Created attachment 124733 [details]
Patch
Comment 7 WebKit Review Bot 2012-01-31 08:06:11 PST
Attachment 124733 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Gyuyoung Kim 2012-01-31 08:08:36 PST
(In reply to comment #4)
> (From update of attachment 124707 [details])
> Most of the code you have moved to the Platform files is still common to all ports, so I see little value in duplicating it all around. You could change the value of JSC_EXECUTABLE_NAME in Source/cmake/OptionsEfl.cmake to "Programs/jsc_efl" and conditionally call SET_TARGET_PROPERTIES(... RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}") if you're building the EFL port. Alternatively, you could have PlatformEfl.cmake (or OptionsEfl.cmake) set an option which defines whether programs should have their runtime output dir set to the base binary directory.

Thank you for your comment. Previous patch moved common code to Platform*.cmake unnecessarily. Patch is updated.
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-01-31 08:22:21 PST
Comment on attachment 124733 [details]
Patch

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

> Source/JavaScriptCore/shell/CMakeLists.txt:23
> +IF (WTF_PLATFORM_EFL)

Why WTF_PLATFORM_EFL instead of "${PORT}" STREQUAL "Efl"?

> Source/JavaScriptCore/shell/CMakeLists.txt:25
> +ELSEIF (SHARED_CORE)

These conditions don't exclude each other.

> Tools/CMakeLists.txt:1
> +IF ("${PORT}" STREQUAL "Efl" AND NOT ONLY_BUILD_JAVASCRIPTCORE)

This change is unrelated to the patch, and I'd rather do the same thing that's done with Source: in the top-level CMakeLists.txt, set ENABLE_TOOLS to Off and only add Tools/ to the build if it's On.

> ChangeLog:9
> +        Change *jsc_efl* executable name with *jsc* in order to run
> +        run-javascriptcore-tests.

This belongs to the section below where you describe the changes to each file.
Comment 10 Gyuyoung Kim 2012-01-31 19:40:02 PST
(In reply to comment #9)
> (From update of attachment 124733 [details])

> > Source/JavaScriptCore/shell/CMakeLists.txt:25
> > +ELSEIF (SHARED_CORE)
> 
> These conditions don't exclude each other.

Do you know why target properties is set when SHARED_CORE is on ? It seems to me we can set target properties regardless of SHARED_CORE option.
Comment 11 Gyuyoung Kim 2012-01-31 20:12:15 PST
Created attachment 124873 [details]
Patch
Comment 12 Gyuyoung Kim 2012-01-31 20:16:14 PST
Kubo, thank you for your comment. I update patch again.

If there is no clear reason, I would like to set target properties regardless of SHARED_CORE as below,  

IF ("${PORT}" STREQUAL "Efl")
    SET_TARGET_PROPERTIES(${JSC_EXECUTABLE_NAME} PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}")
ELSE ()
    SET_TARGET_PROPERTIES(${JSC_EXECUTABLE_NAME} PROPERTIES VERSION ${PROJECT_VERSION})
ENDIF ()
Comment 13 Patrick R. Gansterer 2012-02-01 03:44:35 PST
Comment on attachment 124873 [details]
Patch

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

All comments are "nice to have", so no blocker.

> Source/JavaScriptCore/shell/CMakeLists.txt:26
>      SET_TARGET_PROPERTIES(${JSC_EXECUTABLE_NAME} PROPERTIES VERSION ${PROJECT_VERSION})

i don't think that any other port than EFL uses the VERSION property, so you can remove it

> Source/cmake/OptionsEfl.cmake:51
> +SET(JSC_EXECUTABLE_NAME Programs/jsc)

I don't linke encoding the output directory into _every_ executable name.
The "correct" CMake way to do this is to define sth. like:
SET(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib)
SET(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
SET(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY})

When done this way the Makefile target and generated folder names make more sense too. :-)

> CMakeLists.txt:20
> +SET(ENABLE_TOOLS On)

Usually we write all CMake variables and function in uppercase only. So this should be "ON" instead of "On". Some applies to all other usages. IMHO it's ok to change this an all other ones in a later patch.

> CMakeLists.txt:160
> -ADD_SUBDIRECTORY(Tools)
> +IF (ENABLE_TOOLS)
> +    ADD_SUBDIRECTORY(Tools)
> +ENDIF ()

is this really needed in this change? At least the ChangeLog does not mention it?
Comment 14 Gyuyoung Kim 2012-02-01 04:25:10 PST
Comment on attachment 124873 [details]
Patch

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

>> Source/JavaScriptCore/shell/CMakeLists.txt:26
>>      SET_TARGET_PROPERTIES(${JSC_EXECUTABLE_NAME} PROPERTIES VERSION ${PROJECT_VERSION})
> 
> i don't think that any other port than EFL uses the VERSION property, so you can remove it

Why don't we set "Programs" to TARGET_PROPERTIES as below ? Then, use jsc instead of Programs/jsc in 51 line of OptionsEfl.cmake

SET_TARGET_PROPERTIES(${JSC_EXECUTABLE_NAME} PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/Programs")
Comment 15 Gyuyoung Kim 2012-02-01 04:51:28 PST
Created attachment 124930 [details]
Patch
Comment 16 Daniel Bates 2012-02-04 22:01:44 PST
Comment on attachment 124930 [details]
Patch

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

The patch seems to be fixing two bugs. While both the EFL JavaScript shell properties/name changes and ENABLE_TOOLS changes look reasonable these changes seem mutually exclusive and hence should be separated into individual bugs.

> ChangeLog:11
> +        Do not build Tools when jsc is only built. Build breaks occur because
> +        Tools is built together when building jsc. In addition, change *jsc_efl*
> +        executable name with *jsc* in order to run run-javascriptcore-tests.
> +

The EFL JavaScript shell build is currently broken? Regardless, the first and second sentence in this ChangeLog and the ENABLE_TOOLS changes made to CMakeLists.txt seem unrelated to the issue mentioned in the description of this bug (comment #0). I suggest extracting the Tools changes into a separate bug.
Comment 17 Gyuyoung Kim 2012-02-04 22:59:24 PST
Created attachment 125515 [details]
Patch
Comment 18 Gyuyoung Kim 2012-02-04 23:01:24 PST
(In reply to comment #16)
> (From update of attachment 124930 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124930&action=review
> 
> The patch seems to be fixing two bugs. While both the EFL JavaScript shell properties/name changes and ENABLE_TOOLS changes look reasonable these changes seem mutually exclusive and hence should be separated into individual bugs.
> 
> > ChangeLog:11
> > +        Do not build Tools when jsc is only built. Build breaks occur because
> > +        Tools is built together when building jsc. In addition, change *jsc_efl*
> > +        executable name with *jsc* in order to run run-javascriptcore-tests.
> > +
> 
> The EFL JavaScript shell build is currently broken? Regardless, the first and second sentence in this ChangeLog and the ENABLE_TOOLS changes made to CMakeLists.txt seem unrelated to the issue mentioned in the description of this bug (comment #0). I suggest extracting the Tools changes into a separate bug.

I also think this patch can be separated. So, I file a new Bug for build break.

[CMAKE] Do not build Tools when building jsc only.
https://bugs.webkit.org/show_bug.cgi?id=77826

Could you review the bug ?
Comment 19 WebKit Review Bot 2012-02-05 01:36:39 PST
Comment on attachment 125515 [details]
Patch

Clearing flags on attachment: 125515

Committed r106759: <http://trac.webkit.org/changeset/106759>
Comment 20 WebKit Review Bot 2012-02-05 01:36:45 PST
All reviewed patches have been landed.  Closing bug.