Summary: | [Win] Bundle Inspector Resources in Release builds | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Christopher Reid <chris.reid> | ||||||||||||||||
Component: | CMake | Assignee: | Christopher Reid <chris.reid> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | annulen, aperez, cgarcia, don.olmstead, dpino, ews-watchlist, gyuyoung.kim, Hironori.Fujii, jmason, mcatanzaro, ryuan.choi, sergio, stephan.szabo | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=211014 https://bugs.webkit.org/show_bug.cgi?id=211060 |
||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 212583 | ||||||||||||||||||
Attachments: |
|
Description
Christopher Reid
2020-04-23 16:36:37 PDT
Created attachment 397399 [details]
patch
Created attachment 397400 [details]
patch
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 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 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 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 () Created attachment 397501 [details]
Patch for landing
Created attachment 397502 [details]
Patch for landing
ChangeLog entry in ChangeLog contains OOPS!. Created attachment 397507 [details]
Patch for landing
Committed r260672: <https://trac.webkit.org/changeset/260672> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397507 [details]. This patch broke the compilation of GTK and WPE EWS build bots. Fixed by https://bugs.webkit.org/show_bug.cgi?id=211014 (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. 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. 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. Reopening. (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? 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) Reverted r260672 for reason: [GTK] WebInspector tests are timing out after r260672 Committed r260743: <https://trac.webkit.org/changeset/260743> 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. 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. Looks good to me. Created attachment 397867 [details]
Patch for landing
Committed r260844: <https://trac.webkit.org/changeset/260844> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397867 [details]. 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. (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 *** Bug 211014 has been marked as a duplicate of this bug. *** |