Bug 150245 - Unify handling of JavaScriptCore scripts that are used in WebCore
Summary: Unify handling of JavaScriptCore scripts that are used in WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords:
Depends on:
Blocks: 149929
  Show dependency treegraph
 
Reported: 2015-10-16 11:35 PDT by BJ Burg
Modified: 2015-10-19 13:19 PDT (History)
5 users (show)

See Also:


Attachments
Patch (119.97 KB, patch)
2015-10-16 13:31 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (124.20 KB, patch)
2015-10-16 14:20 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (124.43 KB, patch)
2015-10-16 16:27 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
For Landing (125.23 KB, patch)
2015-10-19 11:02 PDT, BJ Burg
achristensen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2015-10-16 11:35:47 PDT
Splitting this out from 149929 to make sure it will work independently of the other changes.
Comment 1 BJ Burg 2015-10-16 11:38:05 PDT
Should also move non-inspector scripts to Source/JavaScriptCore/Scripts, so we only need to copy over one directory to ForwardingHeaders.
Comment 2 BJ Burg 2015-10-16 13:31:58 PDT
Created attachment 263318 [details]
Patch
Comment 3 WebKit Commit Bot 2015-10-16 13:34:48 PDT
Attachment 263318 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Scripts/generate-combined-inspector-json.py:69:  blank line at end of file  [pep8/W391] [5]
ERROR: Source/JavaScriptCore/Scripts/cssmin.py:28:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Source/JavaScriptCore/Scripts/cssmin.py:34:  whitespace before ')'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/Scripts/cssmin.py:33:  at least two spaces before inline comment  [pep8/E261] [5]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 BJ Burg 2015-10-16 14:20:45 PDT
Created attachment 263330 [details]
Proposed Fix
Comment 5 WebKit Commit Bot 2015-10-16 14:24:16 PDT
Attachment 263330 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Scripts/cssmin.py:28:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Source/JavaScriptCore/Scripts/cssmin.py:34:  whitespace before ')'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/Scripts/cssmin.py:33:  at least two spaces before inline comment  [pep8/E261] [5]
ERROR: Source/JavaScriptCore/Scripts/generate-combined-inspector-json.py:69:  blank line at end of file  [pep8/W391] [5]
Total errors found: 4 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Alex Christensen 2015-10-16 14:45:09 PDT
Comment on attachment 263330 [details]
Proposed Fix

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

> Source/JavaScriptCore/PlatformWin.cmake:35
> +file(APPEND "${JavaScriptCore_POST_BUILD_COMMAND}" "@xcopy /y /d /f \"${DERIVED_SOURCES_DIR}/JavaScriptCore/Scripts/*.*\" \"${DERIVED_SOURCES_DIR}/ForwardingHeaders/JavaScriptCore/Scripts\" >nul 2>nul\n")

I think this should be a file(COPY ...) command in CMake instead of putting this in the post build command.  You might need to use file(GLOB ...) if that doesn't work.

> Source/WebCore/CMakeLists.txt:3370
> +    DEPENDS ${JavaScriptCore_SCRIPTS_DIR}/xxd.pl ${JavaScriptCore_SCRIPTS_DIR}/inline-and-minify-stylesheets-and-scripts.py

I'm not sure you can do this.  When CMake runs, this file won't exist because it is copied in the post build command.  It will probably fail.
Comment 7 BJ Burg 2015-10-16 15:18:38 PDT
Comment on attachment 263330 [details]
Proposed Fix

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

>> Source/WebCore/CMakeLists.txt:3370
>> +    DEPENDS ${JavaScriptCore_SCRIPTS_DIR}/xxd.pl ${JavaScriptCore_SCRIPTS_DIR}/inline-and-minify-stylesheets-and-scripts.py
> 
> I'm not sure you can do this.  When CMake runs, this file won't exist because it is copied in the post build command.  It will probably fail.

Yup, it failed on EWS. If it's not in the post build command and simply a file(COPY command, it should work, right?
Comment 8 Alex Christensen 2015-10-16 15:45:58 PDT
(In reply to comment #7)
> Comment on attachment 263330 [details]
> Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263330&action=review
> 
> >> Source/WebCore/CMakeLists.txt:3370
> >> +    DEPENDS ${JavaScriptCore_SCRIPTS_DIR}/xxd.pl ${JavaScriptCore_SCRIPTS_DIR}/inline-and-minify-stylesheets-and-scripts.py
> > 
> > I'm not sure you can do this.  When CMake runs, this file won't exist because it is copied in the post build command.  It will probably fail.
> 
> Yup, it failed on EWS. If it's not in the post build command and simply a
> file(COPY command, it should work, right?
I think so.  By the time it gets to WebCore/CMakeLists.txt it will have already copied the file, so it should be able to find it.  Keep the xcopy in the post build script, though, because otherwise an incremental build that touches a script but not any CMake files would fail because it would be looking at the old copy in ForwardingHeaders.
Comment 9 BJ Burg 2015-10-16 16:27:56 PDT
Created attachment 263347 [details]
Proposed Fix
Comment 10 WebKit Commit Bot 2015-10-16 16:29:27 PDT
Attachment 263347 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/PlatformWin.cmake:36:  One space between command "endforeach" and its parentheses, should be "endforeach ("  [whitespace/parentheses] [5]
ERROR: Source/JavaScriptCore/Scripts/cssmin.py:28:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Source/JavaScriptCore/Scripts/cssmin.py:34:  whitespace before ')'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/Scripts/cssmin.py:33:  at least two spaces before inline comment  [pep8/E261] [5]
ERROR: Source/JavaScriptCore/Scripts/generate-combined-inspector-json.py:69:  blank line at end of file  [pep8/W391] [5]
Total errors found: 5 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 BJ Burg 2015-10-18 15:32:49 PDT
Comment on attachment 263347 [details]
Proposed Fix

This version seems to work for EWS, so maybe we should give it a try before the tree gets busy again.
Comment 12 Alex Christensen 2015-10-19 10:33:15 PDT
Comment on attachment 263347 [details]
Proposed Fix

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

> Source/WebCore/CMakeLists.txt:823
> +        set(JavaScriptCore_SCRIPTS_DIR "${CMAKE_BINARY_DIR}/../include/private/JavaScriptCore")

I think this should be .../JavaScriptCore/Scripts if the scripts are going to be copied into a subdirectory called Scripts.
Comment 13 BJ Burg 2015-10-19 11:02:37 PDT
Created attachment 263491 [details]
For Landing
Comment 14 BJ Burg 2015-10-19 13:19:22 PDT
Committed r191312: <http://trac.webkit.org/changeset/191312>