Bug 208711 – REGRESSION(r257975): [GTK][WPE] Build failure after a clean build

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

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)

It has been detected in the EWS bots since those bots do a clean build after a patch doesn't build, but the bots at build.webkit.org always build continously

The failure is this one:

Sources/WebCore/JSInternalSettingsGenerated.cpp.o.d -o Source/WebCore/CMakeFiles/WebCoreTestSupport.dir/__/__/DerivedSources/WebCore/JSInternalSettingsGenerated.cpp.o -c DerivedSources/WebCore/JSInternalSettingsGenerated.cpp
DerivedSources/WebCore/JSInternalSettingsGenerated.cpp: In static member function ‘static JSC::IsoSubspace* WebCore::JSInternalSettingsGenerated::subspaceForImpl(JSC::VM&)’:
DerivedSources/WebCore/JSInternalSettingsGenerated.cpp:5927:30: error: ‘class WebCore::DOMIsoSubspaces’ has no member named ‘m_subspaceForInternalSettingsGenerated’; did you mean ‘m_subspaceForInternalSettings’?
     if (auto* space = spaces.m_subspaceForInternalSettingsGenerated.get())
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                              m_subspaceForInternalSettings
DerivedSources/WebCore/JSInternalSettingsGenerated.cpp:5931:16: error: ‘class WebCore::DOMIsoSubspaces’ has no member named ‘m_subspaceForInternalSettingsGenerated’; did you mean ‘m_subspaceForInternalSettings’?
         spaces.m_subspaceForInternalSettingsGenerated = makeUnique<IsoSubspace> ISO_SUBSPACE_INIT(vm.heap, vm.destructibleObjectHeapCellType.get(), JSInternalSettingsGenerated);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                m_subspaceForInternalSettings
DerivedSources/WebCore/JSInternalSettingsGenerated.cpp:5933:16: error: ‘class WebCore::DOMIsoSubspaces’ has no member named ‘m_subspaceForInternalSettingsGenerated’; did you mean ‘m_subspaceForInternalSettings’?
         spaces.m_subspaceForInternalSettingsGenerated = makeUnique<IsoSubspace> ISO_SUBSPACE_INIT(vm.heap, vm.cellHeapCellType.get(), JSInternalSettingsGenerated);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                m_subspaceForInternalSettings
DerivedSources/WebCore/JSInternalSettingsGenerated.cpp:5934:26: error: ‘class WebCore::DOMIsoSubspaces’ has no member named ‘m_subspaceForInternalSettingsGenerated’; did you mean ‘m_subspaceForInternalSettings’?
     auto* space = spaces.m_subspaceForInternalSettingsGenerated.get();
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                          m_subspaceForInternalSettings


And its tricky because it only triggers once, and if you try to re-build after it has triggered then it will build fine. So it only fails once :? That also explain why the EWS its only failing for patches not failing to build (because it triggers clean build after a patch fails to build), but on those building fine it will pass since it tries the patch from a previous rebuild.

So to trigger this you have to test always from a clean build.

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

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.

[Carlos Alberto Lopez Perez]

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.

[Ryan Haddad]

Yusuke, could you take a look at this?

[Yusuke Suzuki]

My guess is that this is caused because dependency from the first generate-bindings-all.pl to InternalSettingsGenerated.idl is not set in CMake.

[Yusuke Suzuki]

Created attachment 392773 [details]
Patch

[Yusuke Suzuki]

Speculative attempt to fix.

[Yusuke Suzuki]

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.

[Aakash Jain]

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

[Stephan Szabo]

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]).

[Konstantin Tokarev]

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.

[Konstantin Tokarev]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 392791 [details]
Patch

[Yusuke Suzuki]

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

[Konstantin Tokarev]

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)

[Konstantin Tokarev]

Got it: GENERATE_SETTINGS_MACROS takes 2 arguments, but is given 3, and idl file is just ignored

[Stephan Szabo]

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

[Konstantin Tokarev]

Created attachment 392840 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 392840 [details]
Patch

Looks nice.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 392853 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 392853 [details]
Patch

Clearing flags on attachment: 392853

Committed r258068: <https://trac.webkit.org/changeset/258068>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Konstantin Tokarev]

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?

[Konstantin Tokarev]

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

[Fujii Hironori]

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.

[Fujii Hironori]

(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