| Summary: | [CMAKE] Support javascriptcore test for EFL port | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||
| Component: | Tools / Tests | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | dbates, paroga, rakuco, webkit.review.bot | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 77426 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Gyuyoung Kim
2012-01-31 04:36:36 PST
Created attachment 124707 [details]
Patch
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.
CC'ing Daniel, Patric and Kubo. BTW, it looks style bot is strange now. 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.
*** Bug 77426 has been marked as a duplicate of this bug. *** Created attachment 124733 [details]
Patch
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.
(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 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. (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. Created attachment 124873 [details]
Patch
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 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 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") Created attachment 124930 [details]
Patch
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. Created attachment 125515 [details]
Patch
(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 on attachment 125515 [details] Patch Clearing flags on attachment: 125515 Committed r106759: <http://trac.webkit.org/changeset/106759> All reviewed patches have been landed. Closing bug. |