Bug 150245 – Unify handling of JavaScriptCore scripts that are used in WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Blaze Burg]

Splitting this out from 149929 to make sure it will work independently of the other changes.

[Blaze Burg]

Should also move non-inspector scripts to Source/JavaScriptCore/Scripts, so we only need to copy over one directory to ForwardingHeaders.

[Blaze Burg]

Created attachment 263318 [details]
Patch

[WebKit Commit Bot]

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.

[Blaze Burg]

Created attachment 263330 [details]
Proposed Fix

[WebKit Commit Bot]

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.

[Alex Christensen]

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.

[Blaze Burg]

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?

[Alex Christensen]

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

[Blaze Burg]

Created attachment 263347 [details]
Proposed Fix

[WebKit Commit Bot]

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.

[Blaze Burg]

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.

[Alex Christensen]

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.

[Blaze Burg]

Created attachment 263491 [details]
For Landing

[Blaze Burg]

Committed r191312: <http://trac.webkit.org/changeset/191312>