Bug 210942 – [Win] Bundle Inspector Resources in Release builds

• Component: CMake
• Status: RESOLVED FIXED

[Christopher Reid]

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.

[Christopher Reid]

Created attachment 397399 [details]
patch

[Christopher Reid]

Created attachment 397400 [details]
patch

[Fujii Hironori]

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.

[Fujii Hironori]

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)

[Fujii Hironori]

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.

[Don Olmstead]

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

[Christopher Reid]

Created attachment 397501 [details]
Patch for landing

[Christopher Reid]

Created attachment 397502 [details]
Patch for landing

[EWS]

ChangeLog entry in ChangeLog contains OOPS!.

[Christopher Reid]

Created attachment 397507 [details]
Patch for landing

[EWS]

Committed r260672: <https://trac.webkit.org/changeset/260672>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397507 [details].

[Diego Pino]

This patch broke the compilation of GTK and WPE EWS build bots. Fixed by https://bugs.webkit.org/show_bug.cgi?id=211014

[Jim Mason]

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

[Diego Pino]

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.

[Jim Mason]

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.

[Michael Catanzaro]

Reopening.

[Fujii Hironori]

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

[Stephan Szabo]

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)

[Diego Pino]

Reverted r260672 for reason:

[GTK] WebInspector tests are timing out after r260672

Committed r260743: <https://trac.webkit.org/changeset/260743>

[Diego Pino]

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.

[Christopher Reid]

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.

[Diego Pino]

Looks good to me.

[Christopher Reid]

Created attachment 397867 [details]
Patch for landing

[EWS]

Committed r260844: <https://trac.webkit.org/changeset/260844>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397867 [details].

[Carlos Garcia Campos]

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.

[Christopher Reid]

(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

[Diego Pino]

*** Bug 211014 has been marked as a duplicate of this bug. ***