Bug 163774 - [CMake][Win] Visual Studio invokes make_settings.pl twice
Summary: [CMake][Win] Visual Studio invokes make_settings.pl twice
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords:
: 154938 155872 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-10-20 22:57 PDT by Fujii Hironori
Modified: 2017-09-20 19:00 PDT (History)
7 users (show)

See Also:


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 Details | Formatted Diff | Diff
Patch (3.60 KB, patch)
2016-10-24 02:03 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 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)
Comment 1 Fujii Hironori 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.
Comment 2 Fujii Hironori 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.
Comment 3 Fujii Hironori 2016-10-21 04:23:12 PDT
Created attachment 292342 [details]
Patch
Comment 4 Alex Christensen 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.
Comment 5 Fujii Hironori 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.
Comment 6 Fujii Hironori 2016-10-24 02:03:12 PDT
Created attachment 292596 [details]
Patch
Comment 7 Michael Catanzaro 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.
Comment 8 WebKit Commit Bot 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>
Comment 9 WebKit Commit Bot 2016-10-29 09:06:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Fujii Hironori 2016-10-30 19:10:19 PDT
*** Bug 155872 has been marked as a duplicate of this bug. ***
Comment 11 Fujii Hironori 2016-10-30 19:10:35 PDT
*** Bug 154938 has been marked as a duplicate of this bug. ***