[cmake-developers] Severe regression caused by #14972 fixes

[Brad King]

On 10/07/2014 02:21 PM, Adam Strzelecki wrote:
> So please either provide test case, or some info
[snip]
On 10/07/2014 03:20 PM, Adam Strzelecki wrote:
> I had a talk with Amine, and the problem was that due 7243c951
> their build.ninja was getting numerous extra phony entries that
> were leading Ninja to crash! So yes we could point our fingers
> on Ninja and close the case, but I propose we do something
> better:

For reference, that commit was:

 Ninja: Don't limit custom cmd side-effects to build folder (#14972)
 http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=7243c951

So now a large number of phony rules for files in the source tree
appear that did not before.  The case in question uses custom
command dependencies so is not helped by the parent change:

 Ninja: Consider only custom commands deps as side-effects (#14972)
 http://www.cmake.org/gitweb?p=cmake.git;a=commitdiff;h=a33cf6d0

> I kindly ask to make possible side-effects phony rules as
> deprecated by some new policy. Basically long story short all
> these "side-effects" tricks were to make some old projects
> don't explicitly specifying custom command outputs work with
> Ninja that stat files once at start, so if there is a
> side-effect file that isn't explicitly specified as command
> output it will be NOT restat, that will lead to some other
> command having that file as dependency to fail.

That situation is discussed thoroughly here:

 Add explicit specification of custom command side effect outputs
 http://www.cmake.org/Bug/view.php?id=14963

and here:

 https://github.com/martine/ninja/issues/760#issuecomment-46540858

Ideally both CMake and Ninja will fix their pieces of the problem.
Fixing the Ninja side as I propose in the above-linked comment
would solve the problem outright with no further changes.  The
fix on the CMake side will also require projects to be updated
to use the new feature, but will help generate more efficient
Ninja build rules.

> Since Make does restat dependencies on each rule this won't
> happen with Make. So we put "side-effects" in phony ensuring
> Ninja to pickup the rules even the file didn't exist when Ninja
> was started.

No.  For build systems besides Ninja the side effects cannot be
listed as outputs of custom commands.  Their timestamps are not
always updated when the custom command runs, so the rules may be
left in an always-rerun state because the side-effect "outputs"
will never be newer than inputs.  Since CMake was designed for
such build systems before Ninja existed, there is no interface
to specify the side-effects explicitly.

> My original intention was actually to limit these phony rules
> and it was done by:
>
> (1) a33cf6d0 (Ninja: Consider only custom commands deps as
> side-effects (#14972))
>
> However somewhere during discussion with Brad I raised question
> why such side-effect are to be in build folder in fact they can
> be anywhere and it we want to ensure Ninja build won't fail
> when Make build goes well we need introduced:
>
> (2) 7243c951 (Ninja: Don't limit custom cmd side-effects to
> build folder (#14972))
>
> This unfortunately lead Ninja to crash with many extra
> side-effect phony rules in ReactOS case.
>
> So again I propose we make side-effect as policy enabled only
> for projects <3.1.

That is not possible because CMake currently does not have enough
information to produce valid Ninja files without the phony rules.
The proper fix to address issue 14963 and provide projects with
an interface to specify custom command side-effects explicitly.
Then dependencies can be hooked up correctly for Ninja.  I invite
you to propose an interface to achieve this.

This will not be done before the freeze for 3.1 on Thursday.
Reverting 7243c951 will resolve the problem for ReactOS in
out-of-source builds.  So, we either revert that or hope Ninja can
be fixed to deal with the large dependency lists w/out crashing.

-Brad