| Summary: | REGRESSION(r257975): [GTK][WPE] Build failure after a clean build | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Carlos Alberto Lopez Perez <clopez> | ||||||||||
| Component: | WebKitGTK | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | aakash_jain, annulen, aperez, bugs-noreply, cdumez, commit-queue, ews-watchlist, gyuyoung.kim, Hironori.Fujii, pnormand, ryuan.choi, saam, sergio, stephan.szabo, webkit-bot-watchers-bugzilla, ysuzuki | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=205107 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Carlos Alberto Lopez Perez
2020-03-06 07:36:12 PST
(In reply to Carlos Alberto Lopez Perez from comment #0) > A build failure happens on GTK and WPE after a clean build in r257995 (this > is the revision I'm testing; doesn't mean it caused it.. it may have been a > previous issue, the failure doesn't trigger if you build continuously > instead from a clean build) > This has been caused by r257975 I tried to revert r257975 locally and the issue is gone (a clean builds works now). BTW, I don't have time today to fix this. Feel free to pick it if you wish. I'm only reporting the issue. It will be nice to have this fixed ASAP as it causing issues on the GTK and WPE EWS. The EWS are unable to determine when a patch fails to build because of the patch or because of a previous failure because the clean build without patch that they try at the end fails due to this. I triggered a clean build on the GTK and WPE release buildbots at build.webkit.org. As I was expecting this happened: * Clean build at r257995 ordered failed: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Build%29/builds/30766 * Next build that happened at r257996 worked because it was not a clean build, but a continuous build from the previous state https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Build%29/builds/30767 And the issue seems gone there, as the bot continues to do continuous builds. But the problem remains. This issue only triggers the first time you build from clean. Yusuke, could you take a look at this? My guess is that this is caused because dependency from the first generate-bindings-all.pl to InternalSettingsGenerated.idl is not set in CMake. Created attachment 392773 [details]
Patch
Speculative attempt to fix. Comment on attachment 392773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392773&action=review > Source/WebCore/WebCoreMacros.cmake:209 > + add_custom_command(${target} And I think we should use add_custom_target. (In reply to Yusuke Suzuki from comment #7) > Speculative attempt to fix. https://ews-build.webkit.org/#/builders/8/builds/17679/steps/8/logs/stdio CMake Error at Source/WebCore/WebCoreMacros.cmake:209 (add_custom_command): add_custom_command Wrong syntax. Unknown type of argument. Comment on attachment 392773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392773&action=review >> Source/WebCore/WebCoreMacros.cmake:209 >> + add_custom_command(${target} > > And I think we should use add_custom_target. Possibly, but add_custom_target has no output file, so you have to be careful with using the output file as a dependency to other things since it doesn't exist at generation time and there's no rule specifically creating it. We might be able to get both with add_custom_command (specifying no target) and then add_custom_target(${target} DEPENDS [output file from the add_custom_command]). Comment on attachment 392773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392773&action=review >>> Source/WebCore/WebCoreMacros.cmake:209 >>> + add_custom_command(${target} >> >> And I think we should use add_custom_target. > > Possibly, but add_custom_target has no output file, so you have to be careful with using the output file as a dependency to other things since it doesn't exist at generation time and there's no rule specifically creating it. > We might be able to get both with add_custom_command (specifying no target) and then add_custom_target(${target} DEPENDS [output file from the add_custom_command]). What is this change supposed to do? It doesn't look like a correct invocation of add_custom_command(), ${target} is likely just ignored. And no, we shouldn't use add_custom_target here. Comment on attachment 392773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392773&action=review > Source/WebCore/ChangeLog:3 > + REGRESSION(r257975): [GTK][WPE] Build failure after a clean build Seems like it's actually not a regression from r257975, but an issue introduced much earlier I think the problem here is that CMake ports are not properly setting dependencies between InternalSettingGenerated.idl and GENERATE_BINDINGS. So in some way, we need to represent dependencies. Maybe, we should place InternalSettingsGenerated.idl as an output of this add_custom_command. Created attachment 392791 [details]
Patch
(In reply to Yusuke Suzuki from comment #14) > Created attachment 392791 [details] > Patch Some speculative attempts since I don't have any environments using CMake WebCore right now. Comment on attachment 392791 [details]
Patch
Still doesn't work here. I guess .idl file should be main product and not Settings.h, as it's needed earlier (or idl generation should depend on Setting.h, but that would be illogical)
Got it: GENERATE_SETTINGS_MACROS takes 2 arguments, but is given 3, and idl file is just ignored (In reply to Konstantin Tokarev from comment #17) > Got it: GENERATE_SETTINGS_MACROS takes 2 arguments, but is given 3, and idl > file is just ignored Handling the two outfiles in GENERATE_SETTINGS_MACROS didn't seem to be enough locally at least on wincairo, unless I also made GENERATE_BINDINGS set a DEPENDS on the input and pp input files in its add_custom_target. I'm not sure if that's a reasonable set of dependencies to set there. Created attachment 392840 [details]
Patch
Comment on attachment 392840 [details]
Patch
Looks nice.
Comment on attachment 392840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392840&action=review > Source/WebCore/WebCoreMacros.cmake:209 > + add_custom_target(${_target} I think add_custom_target makes it won’t stop recompiling. Created attachment 392853 [details]
Patch
Comment on attachment 392853 [details] Patch Clearing flags on attachment: 392853 Committed r258068: <https://trac.webkit.org/changeset/258068> All reviewed patches have been landed. Closing bug. Comment on attachment 392853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392853&action=review > Source/WebCore/WebCoreMacros.cmake:210 > + OUTPUT ${WebCore_DERIVED_SOURCES_DIR}/${_outfile} ${_extra_output} This change reverts https://bugs.webkit.org/show_bug.cgi?id=163774. I wasn't involved with that patch, so I didn't dare to do this. Fujii, is it intentional? Comment on attachment 392853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392853&action=review >> Source/WebCore/WebCoreMacros.cmake:210 >> + OUTPUT ${WebCore_DERIVED_SOURCES_DIR}/${_outfile} ${_extra_output} > > This change reverts https://bugs.webkit.org/show_bug.cgi?id=163774. I wasn't involved with that patch, so I didn't dare to do this. > Fujii, is it intentional? AFAIU, it introduces race condition in parallel build Comment on attachment 392853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392853&action=review >>> Source/WebCore/WebCoreMacros.cmake:210 >>> + OUTPUT ${WebCore_DERIVED_SOURCES_DIR}/${_outfile} ${_extra_output} >> >> This change reverts https://bugs.webkit.org/show_bug.cgi?id=163774. I wasn't involved with that patch, so I didn't dare to do this. >> Fujii, is it intentional? > > AFAIU, it introduces race condition in parallel build Good point. I forgot it completely. It was a CMake Visual Studio generator specific issue. Ninja generator builds aren't affected. And, the issue was fixed in CMake 3.12. https://gitlab.kitware.com/cmake/cmake/issues/16767 https://gitlab.kitware.com/cmake/cmake/commit/5a6c6292898fe238f3a5105133b8904209fbedaf All Windows ports are using newer CMake for Visual Studio 2019 support. Actually, WebKit CMake scripts relies on the fix to avoid unnecessary recompilation. However, I realize one problem related to it. Will fix it in another ticket. (In reply to Fujii Hironori from comment #27) > However, I realize one problem related to it. Will fix it in another ticket. Filed : Bug 208771 – [CMake][Win] GenerateSettings.rb are invoked twice in WebCoreBindings.vcxproj and WebCoreTestSupportBindings.vcxproj |