Bug 161212

<rdar://problem/28017961>

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.

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.

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

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.

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.

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

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.

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

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.

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

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

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

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

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.

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

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

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

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.

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

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

Created attachment 287726 [details] Proposed Fix

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.

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.

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

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.

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

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.

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

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.

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

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.

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

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.

Created attachment 287835 [details] Proposed Fix

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

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

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

Re-opened since this is blocked by bug 161573

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.

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.

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