Bug 163774

Summary: [CMake][Win] Visual Studio invokes make_settings.pl twice
Product: WebKit Reporter: Fujii Hironori <fujii.hironori>
Component: Tools / TestsAssignee: Fujii Hironori <fujii.hironori>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, cdumez, commit-queue, don.olmstead, lforschler, pvollan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=177286
Attachments:
Description Flags
screenshot of Visual Studio
none
Patch
none
Patch none

Fujii Hironori
Reported 2016-10-20 22:57:40 PDT
Created attachment 292322 [details] screenshot of Visual Studio WebCoreDerivedSources.vcxproj and WebCoreTestSupport.vcxproj triggers make_settings.pl. This causes unnecessary recompilation in CMake VisualStudio build. make_settings.pl generates following files: - SettingsMacros.h - InternalSettingsGenerated.h - InternalSettingsGenerated.cpp - InternalSettingsGenerated.idl WebCoreDerivedSources depends on SettingsMacros.h. WebCoreTestSupport depends on InternalSettingsGenerated.cpp. This problem is described in CMake document: https://cmake.org/cmake/help/v3.0/command/add_custom_command.html > Do not list the output in more than one independent target that may > build in parallel or the two instances of the rule may conflict > (instead use add_custom_target to drive the command and make the other > targets depend on that one). There are some solutions: 1) Split make_settings.pl For example, add some command arguments like following: 'make_settings.pl --macros-header ...' generates SettingsMacros.h. 'make_settings.pl --internal-idl ...' generates InternalSettingsGenerated.idl. 'make_settings.pl --internal-source ...' generates InternalSettingsGenerated.cpp and InternalSettingsGenerated.h. 2) Write output files only if changed Use WriteFileIfChanged of preprocess-idls.pl WriteFileIfChanged has a bad effect (infinite regeneration, see Bug 131756) 3) Use add_custom_target But, add_custom_target invokes the command every time, I need to implement timestamp check in CMake or Perl 4) Implement timestamp check in make_settings.pl 5) Implement timestamp check in a new script (CMake, Python or Perl)
Attachments
screenshot of Visual Studio (68.11 KB, image/png)
2016-10-20 22:57 PDT, Fujii Hironori 		no flags
Details
Patch (3.45 KB, patch)
2016-10-21 04:23 PDT, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
Patch (3.60 KB, patch)
2016-10-24 02:03 PDT, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Fujii Hironori
Comment 1 2016-10-21 02:35:39 PDT
Here is my three incremental build logs of WebKit WinCairo port CMake VisualStudio build. https://gist.github.com/fujii/7ae2c8928768647b0f26633e7951189a In first build, make_settings.pl was invoked twice: > Generating ../../DerivedSources/WebCore/SettingsMacros.h, ../../DerivedSources/WebCore/InternalSettingsGenerated.h, ../../DerivedSources/WebCore/InternalSettingsGenerated.cpp, ../../DerivedSources/WebCore/InternalSettingsGenerated.idl (...) > Generating ../../DerivedSources/WebCore/SettingsMacros.h, ../../DerivedSources/WebCore/InternalSettingsGenerated.h, ../../DerivedSources/WebCore/InternalSettingsGenerated.cpp, ../../DerivedSources/WebCore/InternalSettingsGenerated.idl In second build, recompiled some files unnecessarily In third build, did nothing.
Fujii Hironori
Comment 2 2016-10-21 03:54:50 PDT
Another idea is coming to my mind: 7) Specify only SettingsMacros.h as OUTPUT of add_custom_command Mark other generated files (InternalSettingsGenerated.{cpp,h,idl}) GENERATED. This is so simple. I'll create a patch of this approach.
Fujii Hironori
Comment 3 2016-10-21 04:23:12 PDT
Created attachment 292342 [details] Patch
Alex Christensen
Comment 4 2016-10-21 09:53:20 PDT
Comment on attachment 292342 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292342&action=review > Source/cmake/WebKitMacros.cmake:190 > + if (${CMAKE_GENERATOR} MATCHES "Visual Studio") I don't like this. The solution should be the same for all generators.
Fujii Hironori
Comment 5 2016-10-24 01:09:50 PDT
Thank you for reviewing my patch. (In reply to comment #4) > I don't like this. The solution should be the same for all generators. Unfortunately, I need a trick to do so for Ninja. BYPRODUCTS option is needed to work Ninja incremental build fine. But, BYPRODUCTS option is available since CMake 3.2. And, we need to support less version of CMake 3.2 at the moment. I'll create a patch.
Fujii Hironori
Comment 6 2016-10-24 02:03:12 PDT
Created attachment 292596 [details] Patch
Michael Catanzaro
Comment 7 2016-10-29 08:42:38 PDT
Comment on attachment 292596 [details] Patch I've been bitten by basically the same problem twice recently in Automake projects. Glad this didn't require any stamp files or magic to solve.
WebKit Commit Bot
Comment 8 2016-10-29 09:06:18 PDT
Comment on attachment 292596 [details] Patch Clearing flags on attachment: 292596 Committed r208104: <http://trac.webkit.org/changeset/208104>
WebKit Commit Bot
Comment 9 2016-10-29 09:06:23 PDT
All reviewed patches have been landed. Closing bug.
Fujii Hironori
Comment 10 2016-10-30 19:10:19 PDT
*** Bug 155872 has been marked as a duplicate of this bug. ***
Fujii Hironori
Comment 11 2016-10-30 19:10:35 PDT
*** Bug 154938 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.