Bug 161212 – Web Inspector: unify Main.html and Test.html sources and generate different copies with the preprocessor

• Status: REOPENED
• Component: Web Inspector

[Blaze Burg]

Let's end the annoying syncing thing.

[Radar WebKit Bug Importer]

<rdar://problem/28017961>

[Blaze Burg]

This is kind of blocked on moving at least some code generation to CMake + DerivedSources.make. I am not going to attempt to edit the Windows-only scripts.

[Alex Christensen]

Windows uses CMake.  The internal build just blindly copies everything, which works after https://bugs.webkit.org/show_bug.cgi?id=161221 and we could probably consolidate the WebInspectorUI.vcxproj, but verifying it is annoying.  Don't worry about that.

[Blaze Burg]

Created attachment 287282 [details]
Proposed Fix - Make-only

[Blaze Burg]

Comment on attachment 287282 [details]
Proposed Fix - Make-only

This patch was tested on macOS Sierra engineering and production builds. Next up is getting CMake to work, I'll be prototyping on GTK port.

[Build Bot]

Comment on attachment 287282 [details]
Proposed Fix - Make-only

Attachment 287282 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1966861

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 287295 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 287282 [details]
Proposed Fix - Make-only

Attachment 287282 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1966866

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 287299 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 287282 [details]
Proposed Fix - Make-only

Attachment 287282 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1966857

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 287302 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Blaze Burg]

Created attachment 287353 [details]
Proposed Fix (Make-only)

[Blaze Burg]

Created attachment 287445 [details]
Proposed Fix (CMake+Make)

[Blaze Burg]

Created attachment 287446 [details]
Proposed Fix (CMake+Make)

[Blaze Burg]

Comment on attachment 287446 [details]
Proposed Fix (CMake+Make)

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

> Source/PlatformWin.cmake:12
> +    DEPENDS JavaScriptCore WebCore ${WebInspectorUI_RESOURCES}

Blah, I think we need a fake target in WebInspectorUI to force Main.html to be generated earlier. It can't be generated here because WebInspectorUI_RESOURCES is scoped to WebInspectorUI. Windows will have the same problem.

[Blaze Burg]

Created attachment 287472 [details]
Proposed Fix (CMake+Make)

[Blaze Burg]

Created attachment 287473 [details]
Proposed Fix (CMake+Make)

[Blaze Burg]

Created attachment 287555 [details]
Proposed Fix (CMake+Make)

[Joseph Pecoraro]

Comment on attachment 287555 [details]
Proposed Fix (CMake+Make)

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

r=me. Patch looks good to me.

It does seem as though there is a Legit Windows build error on the bot that needs to be addressed:

  Error copying file "C:/cygwin/home/buildbot/WebKit/WebKitBuild/Release/DerivedSources/WebInspectorUI/inspector/InspectorBackendCommands.js" to "C:/cygwin/home/buildbot/WebKit/WebKitBuild/Release/bin32/WebKit.resources/WebInspectorUI/Protocol".

I got a little lost on the GTK resource bundles, but it doesn't appear to have changed behavior.

> Source/WebInspectorUI/CMakeLists.txt:5
> +set(WebInspectorUI_RESOURCE_PATTERNS
> +    ${WEBINSPECTORUI_DIR}/UserInterface/*.html
> +    ${WEBINSPECTORUI_DIR}/UserInterface/Base/*.js

Any chance a double glob would work here?  (I realize this is not new, but just concerning)

    ${WEBINSPECTORUI_DIR}/UserInterface/*.html
    ${WEBINSPECTORUI_DIR}/UserInterface/**/*.js
    ${WEBINSPECTORUI_DIR}/UserInterface/**/*.css

This seems like it will be a real pain to maintain. I also have no idea what it is used for...

> Source/WebInspectorUI/CMakeLists.txt:44
> +# Copy InspectorBackendCommands.js from JavaScriptCore to WebInspectorUI.
> +add_custom_command(
> +    OUTPUT ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/UserInterface/Protocol/InspectorBackendCommands.js
> +    DEPENDS ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/inspector/InspectorBackendCommands.js
> +    COMMAND ${CMAKE_COMMAND} -E copy ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/inspector/InspectorBackendCommands.js ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/UserInterface/Protocol/InspectorBackendCommands.js
> +)

Any idea why the CMake ports do this? Why not just use the JavaScriptCore one? This just ends up getting copied again in Source/PlatformEfl.cmake.

> Source/WebInspectorUI/PlatformGTK.cmake:18
> +    COMMAND ${PYTHON_EXECUTABLE} ${TOOLS_DIR}/gtk/generate-inspector-gresource-manifest.py --output=${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/InspectorGResourceBundle.xml ${WebInspectorUI_RESOURCES}

Wow, this is a lot of arguments on the command line. I hope this doesn't run into issues. (I realize this is not new, but just concerning)

> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:-170
> -    # Remove Debug JavaScript and CSS files in Production builds.

This was the only user of "--strip". That could be removed from Scripts/combine-resources.pl.

> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:254
> +    my $jsMinScript = File::Spec->catfile($sharedScriptsRoot, 'jsmin.py');
> +    my $cssMinScript = File::Spec->catfile($sharedScriptsRoot, 'cssmin.py');

If this uses the jsmin/cssmin from JavaScriptCore we should remove the ones in WebInspectorUI/Scripts. They don't appear to be used anymore.

> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:299
> +    copy(File::Spec->catfile($derivedSourcesDir, 'Main.html'), File::Spec->catfile($targetResourcePath, 'Main.html'));
> +    copy(File::Spec->catfile($derivedSourcesDir, 'Test.html'), File::Spec->catfile($targetResourcePath, 'Test.html'));

Do these fail if the file doesn't exist? That may be useful to catch a port having problems with generating the derived sources.

> Source/WebInspectorUI/Scripts/generate-webinspectorui-derived-sources:1
> +#!/bin/sh

No copyright? Not sure what is common for Build Phase scripts, but all the other scripts have one...

> Source/WebInspectorUI/Scripts/generate-webinspectorui-derived-sources:16
> +    make --no-builtin-rules -f "${WebInspectorUI}/DerivedSources.make" -j `/usr/sbin/sysctl -n hw.activecpu` SDKROOT=${SDKROOT}

This SDKROOT seems to come out of nowhere. Was it needed for something in particular, or just good to have / common?

> Source/WebInspectorUI/Scripts/preprocess-main-resource.pl:25
> +
> +use Getopt::Long;

We should `use strict`.

> Source/WebInspectorUI/Scripts/preprocess-main-resource.pl:31
> +$input = shift or die "Usage: $0 --defines=\"...\" FILENAME.html.in\n";

If using strict, this will need `my $input = ...`.

> Source/WebInspectorUI/UserInterface/Inspector.html.in:694
> +#endif // INCLUDE_UI_RESOURCES
> +
> +#if INCLUDE_UI_RESOURCES

These are the same. I guess it is worth keeping them separate blocks because they are separate blocks.

It would probably be nicer to move these INCLUDE_UI_RESOURCES Controllers down past the Managers. They don't seem to need before the non UI Controllers.

> Source/WebInspectorUI/WebInspectorUI.xcodeproj/project.pbxproj:40
>  		1C435CAC14E7B287004E10EA /* cssmin.py */ = {isa = PBXFileReference; lastKnownFileType = text.script.python; path = cssmin.py; sourceTree = "<group>"; };
>  		1C435CAD14E7B287004E10EA /* jsmin.py */ = {isa = PBXFileReference; lastKnownFileType = text.script.python; path = jsmin.py; sourceTree = "<group>"; };

These are the ones that can/should be removed.

[Blaze Burg]

Comment on attachment 287555 [details]
Proposed Fix (CMake+Make)

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

Thanks for looking!

>> Source/WebInspectorUI/CMakeLists.txt:5
>> +    ${WEBINSPECTORUI_DIR}/UserInterface/Base/*.js
> 
> Any chance a double glob would work here?  (I realize this is not new, but just concerning)
> 
>     ${WEBINSPECTORUI_DIR}/UserInterface/*.html
>     ${WEBINSPECTORUI_DIR}/UserInterface/**/*.js
>     ${WEBINSPECTORUI_DIR}/UserInterface/**/*.css
> 
> This seems like it will be a real pain to maintain. I also have no idea what it is used for...

Right now, its used to generate a manifest for GResource. It's basically GTK's version of xxd.pl, but for multiple resources.

In the future, WebInspectorUI_RESOURCES will reflect any port-independent minification of files, so it could have just the combined files as input to the port-specific packaging.

>> Source/WebInspectorUI/CMakeLists.txt:44
>> +)
> 
> Any idea why the CMake ports do this? Why not just use the JavaScriptCore one? This just ends up getting copied again in Source/PlatformEfl.cmake.

A few loosely related reasons:

- projects outside JavaScriptCore can't see the custom command that generates inspector files, they can only see what's in derived sources.  So we would have to force the thing to be generated in JavaScriptCore anyway. A dependency in WebInspectorUI cannot be fulfilled as we don't know how to build it from outside JavaScriptCore.
- Internal Windows builds have trouble sharing files between projects (though this may be orthogonal)
- some port-specific packaging (specifically, GTK) expects all input files to have a few common root directories. If we don't copy it over, the relative path inspector/ will not match the expected path in the main resource UserInterface/Protocol/.

>> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:-170
>> -    # Remove Debug JavaScript and CSS files in Production builds.
> 
> This was the only user of "--strip". That could be removed from Scripts/combine-resources.pl.

ok

>> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:254
>> +    my $cssMinScript = File::Spec->catfile($sharedScriptsRoot, 'cssmin.py');
> 
> If this uses the jsmin/cssmin from JavaScriptCore we should remove the ones in WebInspectorUI/Scripts. They don't appear to be used anymore.

ok.

>> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:299
>> +    copy(File::Spec->catfile($derivedSourcesDir, 'Test.html'), File::Spec->catfile($targetResourcePath, 'Test.html'));
> 
> Do these fail if the file doesn't exist? That may be useful to catch a port having problems with generating the derived sources.

no but they probably should. It would only catch Mac failures at the moment.

>> Source/WebInspectorUI/Scripts/generate-webinspectorui-derived-sources:1
>> +#!/bin/sh
> 
> No copyright? Not sure what is common for Build Phase scripts, but all the other scripts have one...

Oops, I'll add the block.

>> Source/WebInspectorUI/Scripts/generate-webinspectorui-derived-sources:16
>> +    make --no-builtin-rules -f "${WebInspectorUI}/DerivedSources.make" -j `/usr/sbin/sysctl -n hw.activecpu` SDKROOT=${SDKROOT}
> 
> This SDKROOT seems to come out of nowhere. Was it needed for something in particular, or just good to have / common?

I don't think it's needed, copypasta.

>> Source/WebInspectorUI/Scripts/preprocess-main-resource.pl:25
>> +use Getopt::Long;
> 
> We should `use strict`.

OK

>> Source/WebInspectorUI/UserInterface/Inspector.html.in:694
>> +#if INCLUDE_UI_RESOURCES
> 
> These are the same. I guess it is worth keeping them separate blocks because they are separate blocks.
> 
> It would probably be nicer to move these INCLUDE_UI_RESOURCES Controllers down past the Managers. They don't seem to need before the non UI Controllers.

I'll try it.

>> Source/WebInspectorUI/WebInspectorUI.xcodeproj/project.pbxproj:40
>>  		1C435CAD14E7B287004E10EA /* jsmin.py */ = {isa = PBXFileReference; lastKnownFileType = text.script.python; path = jsmin.py; sourceTree = "<group>"; };
> 
> These are the ones that can/should be removed.

ok

[Blaze Burg]

Created attachment 287679 [details]
Proposed Fix (except Windows)

[Blaze Burg]

Created attachment 287726 [details]
Proposed Fix

[Joseph Pecoraro]

Comment on attachment 287726 [details]
Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Inspector.html.in:696
> +#if INCLUDE_UI_RESOURCES
> +
> +#endif // INCLUDE_UI_RESOURCES

This is empty and can be removed.

[Blaze Burg]

Still somehow fails on windows. It works locally. I'll try another clean build just to be sure. It looks like this part fails:

 Generating ../../DerivedSources/WebInspectorUI/UserInterface/Main.html
  Usage: C:/cygwin/home/buildbot/WebKit/Source/WebInspectorUI/Scripts/preprocess-main-resource.pl --defines="..." FILENAME.html.in
C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.CppCommon.targets(171,5): error MSB6006: "cmd.exe" exited with code 255. 


It would be way more helpful to see the actual invocation of the script to see what led it to have no leftover arguments.

[Blaze Burg]

Created attachment 287746 [details]
Proposed Fix - Debugging Win EWS

[WebKit Commit Bot]

Attachment 287746 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/CMakeLists.txt:54:  One space between command "endforeach" and its parentheses, should be "endforeach ("  [whitespace/parentheses] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Created attachment 287766 [details]
Proposed Fix - Debugging Win EWS

[WebKit Commit Bot]

Attachment 287766 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/CMakeLists.txt:54:  One space between command "endforeach" and its parentheses, should be "endforeach ("  [whitespace/parentheses] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Created attachment 287778 [details]
Proposed Fix - More EWS debugging

[WebKit Commit Bot]

Attachment 287778 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/CMakeLists.txt:55:  One space between command "endforeach" and its parentheses, should be "endforeach ("  [whitespace/parentheses] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Created attachment 287800 [details]
Proposed Fix - More EWS debugging

[WebKit Commit Bot]

Attachment 287800 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/CMakeLists.txt:55:  One space between command "endforeach" and its parentheses, should be "endforeach ("  [whitespace/parentheses] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Created attachment 287806 [details]
Proposed Fix - More EWS debugging

[WebKit Commit Bot]

Attachment 287806 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/CMakeLists.txt:55:  One space between command "endforeach" and its parentheses, should be "endforeach ("  [whitespace/parentheses] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Created attachment 287835 [details]
Proposed Fix

[Blaze Burg]

Created attachment 287870 [details]
Proposed Fix, now with less VERBATIM

[WebKit Commit Bot]

Comment on attachment 287870 [details]
Proposed Fix, now with less VERBATIM

Rejecting attachment 287870 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 287870, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
    -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 205412 = 1f7f8a2610dc5544a3ccd8596ceb5211a5a9ffac
r205413 = f80bb23180ba205719a98ede391f9b973733ded2
r205414 = 4f9556cfeebe3f738abbdcf28ea1f17a55018237
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/2003178

[Blaze Burg]

Committed r205415: <http://trac.webkit.org/changeset/205415>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 161573

[Blaze Burg]

The failure on bots was often:

WebInspectorUI.framework/Resources/TestCombined.js:11280: CONSOLE ERROR SyntaxError: Unexpected end of script
WebInspectorUI.framework/Resources/Test.html:39:21: CONSOLE ERROR ReferenceError: Can't find variable: WebInspector
Wait on notifyDone timed out, process com.apple.WebKit.WebContent.Development pid = 87730

So I think there was a stale resource or something was not built correctly. I cannot reproduce locally on trunk.

[Blaze Burg]

And for some reason EWS was not affected either, so my best guess right now is that a clean build is needed. I tried to force a clean build on a bot, but it just pulled down and archive and re-ran tests without rebuilding, so that's not very helpful.

[Carlos Alberto Lopez Perez]

(In reply to Brian Burg from comment #41)
> And for some reason EWS was not affected either, so my best guess right now
> is that a clean build is needed. I tried to force a clean build on a bot,
> but it just pulled down and archive and re-ran tests without rebuilding, so
> that's not very helpful.

You have to force a clean build on the "(Build)" bot so it generates a new build product. The "(Test)" one will download it later.

What I usually do in this cases is just to trigger clean builds on all the bots. I open a new tab per bot and then click-click on the clean-build form. Just in case. It ends working.

Just my 2 cents.