Bug 210942 - [Win] Bundle Inspector Resources in Release builds
Summary: [Win] Bundle Inspector Resources in Release builds
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CMake (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Christopher Reid
URL:
Keywords:
: 211014 (view as bug list)
Depends on:
Blocks: 212583
  Show dependency treegraph
 
Reported: 2020-04-23 16:36 PDT by Christopher Reid
Modified: 2020-05-31 10:17 PDT (History)
13 users (show)

See Also:


Attachments
patch (18.69 KB, patch)
2020-04-23 16:42 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
patch (18.66 KB, patch)
2020-04-23 16:45 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch for landing (19.66 KB, patch)
2020-04-24 13:38 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch for landing (19.65 KB, patch)
2020-04-24 13:39 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch for landing (19.65 KB, patch)
2020-04-24 14:20 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Fixing GTK timeouts (19.87 KB, patch)
2020-04-27 18:15 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch for landing (20.92 KB, patch)
2020-04-28 12:23 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Reid 2020-04-23 16:36:37 PDT
GTK uses the copy-user-interface-resources.pl to combine inspector resources. Windows should do something similar to speed up inspector load times in Release builds.
Comment 1 Christopher Reid 2020-04-23 16:42:14 PDT
Created attachment 397399 [details]
patch
Comment 2 Christopher Reid 2020-04-23 16:45:20 PDT
Created attachment 397400 [details]
patch
Comment 3 Fujii Hironori 2020-04-23 16:59:49 PDT
Comment on attachment 397400 [details]
patch

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

> Source/WebInspectorUI/CMakeLists.txt:3
> +    ${CMAKE_SOURCE_DIR}/Source/WebInspectorUI/UserInterface/*.html

I prefer ${CMAKE_CURRENT_SOURCE_DIR} to ${CMAKE_SOURCE_DIR}/Source/WebInspectorUI,
becuase WebKit might be built as submodule of other project.
Comment 4 Fujii Hironori 2020-04-23 17:04:26 PDT
Comment on attachment 397400 [details]
patch

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

> CMakeLists.txt:36
> +endif ()

If I understand CMake properly, 'option' command is the CMake way to do that.
option(ENABLE_WEBINSPECTOR_UI "help string describing option" ON)
Comment 5 Fujii Hironori 2020-04-23 22:20:31 PDT
Comment on attachment 397400 [details]
patch

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

>> CMakeLists.txt:36
>> +endif ()
> 
> If I understand CMake properly, 'option' command is the CMake way to do that.
> option(ENABLE_WEBINSPECTOR_UI "help string describing option" ON)

Ah, the default values are different for ports. Ignore this comment.
Comment 6 Don Olmstead 2020-04-24 09:54:58 PDT
Comment on attachment 397400 [details]
patch

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

Other than the below LGTM

>> Source/WebInspectorUI/CMakeLists.txt:3
>> +    ${CMAKE_SOURCE_DIR}/Source/WebInspectorUI/UserInterface/*.html
> 
> I prefer ${CMAKE_CURRENT_SOURCE_DIR} to ${CMAKE_SOURCE_DIR}/Source/WebInspectorUI,
> becuase WebKit might be built as submodule of other project.

Seems more like you should follow along with what happens in WebKitFS.cmake

if (NOT WEBINSPECTORUI_DIR)
    set(WEBINSPECTORUI_DIR "${CMAKE_SOURCE_DIR}/Source/WebInpsectorUI")
endif ()
Comment 7 Christopher Reid 2020-04-24 13:38:04 PDT
Created attachment 397501 [details]
Patch for landing
Comment 8 Christopher Reid 2020-04-24 13:39:53 PDT
Created attachment 397502 [details]
Patch for landing
Comment 9 EWS 2020-04-24 13:40:28 PDT
ChangeLog entry in ChangeLog contains OOPS!.
Comment 10 Christopher Reid 2020-04-24 14:20:04 PDT
Created attachment 397507 [details]
Patch for landing
Comment 11 EWS 2020-04-24 14:45:47 PDT
Committed r260672: <https://trac.webkit.org/changeset/260672>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397507 [details].
Comment 12 Diego Pino 2020-04-25 06:33:08 PDT
This patch broke the compilation of GTK and WPE EWS build bots. Fixed by https://bugs.webkit.org/show_bug.cgi?id=211014
Comment 13 Jim Mason 2020-04-26 02:53:54 PDT
(In reply to Diego Pino from comment #12)
> This patch broke the compilation of GTK and WPE EWS build bots. Fixed by
> https://bugs.webkit.org/show_bug.cgi?id=211014
I am getting GTK build errors since r260672 that are still present as of this moment (r260723).  The build error is this:

   make[3]: *** No rule to make target 'DerivedSources/JavaScriptCore/inspector/InspectorBackendCommands.js', needed by 'DerivedSources/WebInspectorUI/UserInterface/Protocol/InspectorBackendCommands.js'.  Stop.

r260671 was the last I could build the GTK port without any errors.
Comment 14 Diego Pino 2020-04-26 06:45:21 PDT
Jim Manson:

The GTK build bot is green on r260723, and the GTK EWS build bots are able to build too.

https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20(Build)?numbuilds=50

https://ews-build.webkit.org/#/builders/36

I've also tried to build r260723 locally and it worked OK. I would suggest to try a clean build.

What command are you using to build WebKitGTK? If I have time I can give it a try.
Comment 15 Jim Mason 2020-04-26 10:47:53 PDT
Hello Diego, thank you for the prompt reply.

I believe I have found the problem: It seems the new WebInspectorUI target has an implicit dependency on JavaScriptCore for generation of 'DerivedSources/JavaScriptCore/inspector/InspectorBackendCommands.js' referenced above and maybe more.

When I add JavaScriptCore as a dependency to the WebInspectorUI target in Source/WebInspectorUI/CMakeLists.txt, it builds with no problem.

I am building with -j 8, which may have helped expose the dependency.
Comment 16 Michael Catanzaro 2020-04-26 10:56:12 PDT
Reopening.
Comment 17 Fujii Hironori 2020-04-26 13:09:32 PDT
(In reply to Jim Mason from comment #15)
> When I add JavaScriptCore as a dependency to the WebInspectorUI target in
> Source/WebInspectorUI/CMakeLists.txt, it builds with no problem.

Did r260728 fix it?
Comment 18 Stephan Szabo 2020-04-26 23:09:40 PDT
This also appears to be broken for not ENABLE_WEBINSPECTOR_UI ports that build WebCore (i.e. not jsc only) as it unconditionally adds the dependency to WebCore whether the enable is on or not.

From https://build.webkit.org/builders/PlayStation-Release-Build/builds/2043/steps/compile-webkit/logs/stdio 

CMake Error at Source/cmake/WebKitMacros.cmake:179 (add_dependencies):
  The dependency target "WebInspectorUI" of target "WebCore" does not exist.
Call Stack (most recent call first):
  Source/cmake/WebKitMacros.cmake:184 (_WEBKIT_TARGET)
  Source/WebCore/CMakeLists.txt:2033 (WEBKIT_FRAMEWORK)
Comment 19 Diego Pino 2020-04-27 00:40:51 PDT
Reverted r260672 for reason:

[GTK] WebInspector tests are timing out after r260672

Committed r260743: <https://trac.webkit.org/changeset/260743>
Comment 20 Diego Pino 2020-04-27 00:53:52 PDT
I reverted the patch since WebInspector tests are timing out after this change. I also reverted related build fixes changesets (r260728 and r260696).

Ticket of the issue: https://bugs.webkit.org/show_bug.cgi?id=211060
Latest GTK test builds: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20(Tests)?numbuilds=50

I was able to detect the test regression after a clean build.
Comment 21 Christopher Reid 2020-04-27 18:15:44 PDT
Created attachment 397778 [details]
Fixing GTK timeouts

This patch should fix the timeouts. The problem was WebInspectorUI_RESOURCES_DIR wasn't pointing to the InspectorResources folder on GTK/WPE, sorry about that.

I tried a clean build with these changes on my local GTK environment and the only tests timing out now are:
> 17:05:49.058 3 Tests that timed out or crashed:
> 17:05:49.058 3   inspector/debugger/nested-inspectors.html took 17.0 seconds
> 17:05:49.058 3   inspector/debugger/breakpoints/resolved-dump-each-line.html took 16.9 seconds
> 17:05:49.058 3   inspector/dom-debugger/dom-breakpoints.html took 15.2 seconds
> 17:05:49.058 3   inspector/console/heapSnapshot.html took 15.1 seconds

These timeouts look expected.
Comment 22 Diego Pino 2020-04-28 07:15:49 PDT
Looks good to me.
Comment 23 Christopher Reid 2020-04-28 12:23:07 PDT
Created attachment 397867 [details]
Patch for landing
Comment 24 EWS 2020-04-28 13:59:04 PDT
Committed r260844: <https://trac.webkit.org/changeset/260844>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397867 [details].
Comment 25 Carlos Garcia Campos 2020-04-29 08:29:42 PDT
I think this patch made incremental builds super slow because inspector files are always copied now. I guess it's because inspector-resources.stamp is no longer used.
Comment 26 Christopher Reid 2020-04-29 17:55:55 PDT
(In reply to Carlos Garcia Campos from comment #25)
> I think this patch made incremental builds super slow because inspector
> files are always copied now. I guess it's because inspector-resources.stamp
> is no longer used.

I'm looking in to fixing this now and created https://bugs.webkit.org/show_bug.cgi?id=211212 for it
Comment 27 Diego Pino 2020-05-02 03:54:27 PDT
*** Bug 211014 has been marked as a duplicate of this bug. ***