build.ninja generator not re-running when it should

[bradking]

#1753 broke CMake's test suite. I must have neglected to run it before my approval in that PR.

I tracked it down to this example:

rm -f .ninja_deps .ninja_log
cat >build.ninja <<EOF
rule generator
  command = touch build.ninja
  generator = 1
build build.ninja: generator | implicit_generator_input
EOF
sleep 1
touch implicit_generator_input
ninja
ls -tr build.ninja implicit_generator_input

Prior to the change, the output is:

[1/1] touch build.ninja
ninja: no work to do.
implicit_generator_input
build.ninja

After the change, the output is:

ninja: no work to do.
build.ninja
implicit_generator_input

[bradking]

Cc: @jhasse @jdrouhard

[jdrouhard]

I'll take a look. Thanks.

[jdrouhard]

@bradking you may have run my original PR against your test suite. The small refactor I made at the tail end of @jhasse's review is actually what caused it. I'm going to put up another PR to revert back to my original code in RecomputeOutputDirty().

[bradking]

Here is another case not fixed by the current draft of #1933:

rm -f .ninja_deps .ninja_log
touch inout
cat >build.ninja <<EOF
rule generator
  command = touch inout ; sleep 1; touch build.ninja
  generator = 1
build build.ninja: generator | inout inimp
EOF
sleep 1
touch inimp
ninja
ls -tr build.ninja inout inimp

Prior to #1753, this re-ran the generator exactly once. After #1753 it does not re-run the generator at all. With the current draft of #1933, it re-runs the generator repeatedly.

[jdrouhard]

@bradking would it make sense to continue to record the output's mtime in the build log if the rule is marked as a generator rule? Doing this fixes this issue while still being able to detect input file changes for normal rules. It seems like for generator rules, recording the most recent input time in the build log could lead to infinite loops (like this one) if the generator generates inputs as well as outputs.

[bradking]

@jdrouhard let's try that special case for generator rules. Update your PR for that and I'll see if anything else still fails in the CMake test suite.

[jdrouhard]

@bradking done. Let me know.

[bradking]

Here is another case:

rm -f .ninja_deps .ninja_log out out.d

>inimp
echo >build.ninja '
ninja_required_version = 1.5
rule example
  command = echo "$out: inimp" > $depfile ; touch $out
build out: example
  depfile = out.d
default out'

ninja
ninja -d explain

Prior to #1753, the output was:

[1/1] echo "out: inimp" > out.d ; touch out
ninja: no work to do.

Now the output is:

[1/1] echo "out: inimp" > out.d ; touch out
ninja explain: recorded mtime of out older than most recent input inimp (0 vs 1616447551098396350)
[1/1] echo "out: inimp" > out.d ; touch out

Adding deps = gcc to the build out statement fixes it, so I suspect the non-depslog variant of the implementation was not handled by #1753.

[jdrouhard]

Sure. This seems to be of a similar nature to the other cases, where the rule itself generates a file that ends up being an implicit input dependency. In this case, it's not a generator rule, but a non-special depfile.

I have it fixed on my branch, but I'd like to write a test for it. We should port your entire CMake test suite into native ninja tests, they would be very useful, I think ;). Thanks for testing this so thoroughly.

[jdrouhard]

@bradking New commit pushed to #1933 which should get CMake's test suite fully passing. Thanks again for your diligence with testing this.

[bradking]

similar nature to the other cases, where the rule itself generates a file that ends up being an implicit input dependency.

How so? The inimp file is created ahead of time. The only thing the command= does is create the output file and a depfile.

EDIT: Or do you mean that the depfile = itself, when not used with the deps log, becomes an implicit dependency?

[jdrouhard]

EDIT: Or do you mean that the depfile = itself, when not used with the deps log, becomes an implicit dependency?

Yes, that's what I meant.