How to express generated header dependencies?

[Reid Kleckner]

My real problem is that the CMake+ninja build for clang has missing dependencies and requires two builds.  Headers are generated with tblgen, which reads .td files.  If you edit a .td file, there are two actions:

1. generate temporary output files from input.td

2. if the temporary output differs from the old output or the old output does not exist, copy the temporary file to the real output path

This gets wrapped up in a phony target, which is used as an order only dep on every .o file in the library that uses the tablegen output.

If I edit the .td file, the headers are regenerated, but no source files are rebuilt.  If I run ninja again, it notices that the headers have changed and rebuilds the relevant .o files.

Why do I have to do two builds?  What needs to change in build.ninja to get this right?  The answer to this question should probably go into the manual.

Previous discussion suggests that I need to put use '|| path\to\header.inc', but hacking buid.ninja to use that instead of the phony target doesn't help.

https://groups.google.com/forum/?fromgroups#!topic/ninja-build/2C1PISyR1Yk

The rules used to generate the headers have 'restat = 1' set, if that matters.  Removing restat breaks incremental builds, in that the generated headers are always out of date and the C++ files including them have to be rebuilt.

[Evan Martin]

Let me rephrase what I think you're saying, so that I'm sure I followed you.

1) A .td file is processed by some helper tool to generate .h files.  You have this rule expressed in .ninja syntax.

2) Those .h files are then used like normal C++ code.  These are implicitly used by the compile rule.

There are two cases:

A) On the first build, you need to tell Ninja something that causes step one above to happen before step two.  That could be a direct dependency, or that could be something indirect (like say maybe step one writes out a stamp file or something).  You just need to ensure they happen in the right order.  This dependency is what the "||" syntax is for (it expresses "ensure x happens before y if both need to be built, but otherwise don't order them").

B) On subsequent builds, step one already links .td to .h, and step two should pick up (via the normal Ninja header dependencies) that the .h files are inputs to the compile, so you don't need anything else.

It sounds like case A already works for whatever reason, and case B is busted for you in some way.  Without poking at it directly I would suspect the use of a phony rule -- there was a recent thread here about using them to do something that might just be a bug.

I'm trying to think of how I can remote-diagnose this and not coming up with a lot.  :(

Maybe we can start with running "ninja -t explain" and pasting the output of that.

Or maybe give me instructions on how to replicate your build setup?

--
You received this message because you are subscribed to the Google Groups "ninja-build" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ninja-build...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Reid Kleckner]

Good, I think we're on the same page.  :)  I was having a hard time expressing the problem.

Case A works.  A clean build is correct and doesn't require a second build.

Case B doesn't work.  An incremental build after modifying a .td requires two builds: one to update the .h, and a second to realize that the .h implicit compile input changed.

Unfortunately I don't have a reduced test case yet.  I'll try to make one.

Mostly I wanted to know, what is the preferred, correct way to express this in build.ninja?  From the docs and this email it sounds like the order only dependency should be enough, but something is off.  It could be the intermediate phony targets.

The way I see the problem is that ninja checks timestamps up front to compute the action graph.  At this time, the .h files are older than the .o files, so the compile actions are dropped from the action graph.  What logic is supposed to kick in to notice that the .h files are the output of some other target?

[Bill Hoffman]

On 5/23/2013 11:15 AM, Reid Kleckner wrote:
> Good, I think we're on the same page. :) I was having a hard time
> expressing the problem.
>
> Case A works. A clean build is correct and doesn't require a second build.
>
> Case B doesn't work. An incremental build after modifying a .td
> requires two builds: one to update the .h, and a second to realize that
> the .h implicit compile input changed.
>
> Unfortunately I don't have a reduced test case yet. I'll try to make one.
>
> Mostly I wanted to know, what is the preferred, correct way to express
> this in build.ninja? From the docs and this email it sounds like the
> order only dependency should be enough, but something is off. It could
> be the intermediate phony targets.
>
> The way I see the problem is that ninja checks timestamps up front to
> compute the action graph. At this time, the .h files are older than the
> .o files, so the compile actions are dropped from the action graph.
> What logic is supposed to kick in to notice that the .h files are the
> output of some other target?

Can you create a small CMake example of this. I want to make sure it
works with the other CMake generators. If it works with them it should
work with the ninja one of course.

Thanks.

-Bill

[Reid Kleckner]

I attached a minimal CMake example that simply copies header.txt to header.h.  Let's see if it makes it through Google Groups.

This looks like a Windows-specific ninja bug.  It doesn't repo on Linux with make or ninja.

Repro steps:

$ cmake .. -GNinja && ninja

-- The C compiler identification is MSVC 17.0.51106.1

....

[3/3] Linking CXX executable foo.exe

$ ./foo.exe

HEADER_STRING: asdf;

$ sed -i -e 's/asdf/jkl/' ../header.txt

$ ninja && ./foo.exe

[1/1] Generating header.h

HEADER_STRING: asdf;

$ ninja && ./foo.exe

[1/2] Building CXX object CMakeFiles\foo.dir\foo.cpp.obj

[2/2] Linking CXX executable foo.exe

HEADER_STRING: jkl;

The .d file looks correct:

$ cat CMakeFiles/foo.dir/foo.cpp.obj.d

CMakeFiles\foo.dir\foo.cpp.obj: \

C:/PROGRA~2/MICROS~3.0/VC/include/ConcurrencySal.h \

C:/PROGRA~2/MICROS~3.0/VC/include/crtdefs.h \

C:/PROGRA~2/MICROS~3.0/VC/include/sal.h \

C:/PROGRA~2/MICROS~3.0/VC/include/stdio.h \

C:/PROGRA~2/MICROS~3.0/VC/include/swprintf.inl \

C:/PROGRA~2/MICROS~3.0/VC/include/vadefs.h \

D:/src/ninja/gen_header/build/header.h \

Path separator bug?

--
You received this message because you are subscribed to the Google Groups "ninja-build" group.

To unsubscribe from this group and stop receiving emails from it, send an email to ninja-build+unsubscribe@googlegroups.com.

[Reid Kleckner]

Debugging further, it's because 'cmcldeps.exe' and 'ninja -t msvc' produce different .d file syntax.  If I hack out the 'cmcldeps.exe' call with 'ninja -t msvc' I get this in the .d file:

CMakeFiles\foo.dir\foo.cpp.obj: c:\progra~2\micros~3.0\vc\include\concurrencysal.h

c:\progra~2\micros~3.0\vc\include\crtdefs.h

... snip

header.h

[Evan Martin]

On Thu, May 23, 2013 at 8:15 AM, Reid Kleckner <r...@google.com> wrote:

Good, I think we're on the same page.  :)  I was having a hard time expressing the problem.

Case A works.  A clean build is correct and doesn't require a second build.

Case B doesn't work.  An incremental build after modifying a .td requires two builds: one to update the .h, and a second to realize that the .h implicit compile input changed.

Unfortunately I don't have a reduced test case yet.  I'll try to make one.

Mostly I wanted to know, what is the preferred, correct way to express this in build.ninja?  From the docs and this email it sounds like the order only dependency should be enough, but something is off.  It could be the intermediate phony targets.

The way I see the problem is that ninja checks timestamps up front to compute the action graph.  At this time, the .h files are older than the .o files, so the compile actions are dropped from the action graph.  What logic is supposed to kick in to notice that the .h files are the output of some other target?

The command that generates the headers must tell Ninja which headers it generates.

The command that consumes the headers implicitly tells Ninja which headers it uses via its depfile.

So the graph is complete, and it's just a question of figuring out which parts of the graph need to be updated.

Ninja's graph traversal logic looks like:

- determine if the .o needs rebuilding, it depends on the .h (known via depfile)
- determine if the .h needs rebuilding, it depends on the .td (known via the .h being an output of a command)

- put both of the above on a "to be built" queue

Then when building, due to the restat line, if the .td -> .h command doesn't end up touching its output, the downstream items (the .o recompile) in the queue are removed.

[Nico Weber]

Here's a gyp file for the same project. Running your steps, except with `gyp -f ninja --depth .` instead of cmake, things seem to work.

It sounds like cmake needs to change cmcldeps to produce nicer paths, or use ninja -t msvc (or maybe use the new deps=msvs stuff if they're adventurous).

--

[Bill Hoffman]

On 5/23/2013 1:15 PM, Nico Weber wrote:
> Here's a gyp file for the same project. Running your steps, except with
> `gyp -f ninja --depth .` instead of cmake, things seem to work.
>
> It sounds like cmake needs to change cmcldeps to produce nicer paths, or
> use ninja -t msvc (or maybe use the new deps=msvs stuff if they're
> adventurous).
>

So, what is wrong with the paths? I guess ninja can not handle / and
expects a \?

$ cat CMakeFiles/foo.dir/foo.cpp.obj.d
CMakeFiles\foo.dir\foo.cpp.obj: \
C:/PROGRA~2/MICROS~3.0/VC/include/ConcurrencySal.h \

...
C:/PROGRA~2/MICROS~3.0/VC/include/vadefs.h \
D:/src/ninja/gen_header/build/header.h \


vs

c:\progra~2\micros~3.0\vc\include\crtdefs.h
... snip
header.h

I am pretty sure the CMake stuff worked at some point. Was there a
change in ninja? Longer term we do want to get to -t msvc, but I want
to figure out what changed and how to fix it.

Thanks.

-Bill

[Scott Graham]

I don't think ninja has ever normalized path slashes internally. Is it possible that inconsistent slashes are just undetected by CMake's test suite?

I think we have the same problem in gyp right now unfortunately. Maybe we could add a -d on Windows that forces you to pick either \ or / and errors out when it sees the other one to help generators.

--
You received this message because you are subscribed to the Google Groups "ninja-build" group.

To unsubscribe from this group and stop receiving emails from it, send an email to ninja-build+unsubscribe@googlegroups.com.

[Nico Weber]

On Thu, May 23, 2013 at 10:25 AM, Bill Hoffman <bill.h...@kitware.com> wrote:

On 5/23/2013 1:15 PM, Nico Weber wrote:

Here's a gyp file for the same project. Running your steps, except with
`gyp -f ninja --depth .` instead of cmake, things seem to work.

It sounds like cmake needs to change cmcldeps to produce nicer paths, or
use ninja -t msvc (or maybe use the new deps=msvs stuff if they're
adventurous).

So, what is wrong with the paths?  I guess ninja can not handle / and expects a \?

I think the main problem is that they are absolute. As far as I understand, ninja roughly* maps every path it finds to one graph node, and two paths are only considered equal if they're lexically equal. You can use both '/' and '\', as long as you make sure that you're consistent about this. But since build.ninja refers to header.h as just header.h, the .d files need to do that too. Ninja's -t msvc relativizes paths for that reason, see src/includes_normalize-win32.cc (which apparently normalizes '/' to '\\', so that kind of requires that '\' is used in ninja files hat use -t msvc).

*: Ninja does some simple relative path normalization in the sense that it converts 'a/../b' to just 'b', see CanonicalizePath() in src/util.cc – but that function _always_ expects '/'s, which contradicts the behavior of IncludesNormalize somewhat. This sounds like a bug somewhere.

 

$ cat CMakeFiles/foo.dir/foo.cpp.obj.d
CMakeFiles\foo.dir\foo.cpp.obj: \
C:/PROGRA~2/MICROS~3.0/VC/include/ConcurrencySal.h \

...
C:/PROGRA~2/MICROS~3.0/VC/include/vadefs.h \
D:/src/ninja/gen_header/build/header.h \


vs

c:\progra~2\micros~3.0\vc\include\crtdefs.h
... snip
header.h

I am pretty sure the CMake stuff worked at some point.   Was there a change in ninja?  Longer term we do want to get to -t msvc, but I want to figure out what changed and how to fix it.

As far as I can tell, nothing in this area has changed since windows support was added. I could be wrong, of course.

If you have a test that tests this case, you could `git bisect` ninja to check if it ever passed.

 

Thanks.

-Bill

--
You received this message because you are subscribed to the Google Groups "ninja-build" group.

To unsubscribe from this group and stop receiving emails from it, send an email to ninja-build+unsubscribe@googlegroups.com.

[Reid Kleckner]

I filed a CMake bug with a patch attached:

http://cmake.org/Bug/view.php?id=14167

I didn't need to change ninja's CanonicalizePath() to handle backslash paths.

--
You received this message because you are subscribed to the Google Groups "ninja-build" group.

To unsubscribe from this group and stop receiving emails from it, send an email to ninja-build+unsubscribe@googlegroups.com.

[Bill Hoffman]

On 5/23/2013 5:05 PM, Reid Kleckner wrote:
> I filed a CMake bug with a patch attached:
> http://cmake.org/Bug/view.php?id=14167
>
> I didn't need to change ninja's CanonicalizePath() to handle backslash
> paths.

Does that fix the problem? It is not because they are full paths? It
would be nice if ninja fixed this. Older versions of CMake will work if
ninja fixes... :)


-Bill

[Reid Kleckner]

The problem is that the deps file parsing is on the critical path for ninja startup, unless you're using the new 'deps = gcc/msvc' feature.  Therefore, I don't want to touch ninja's CanonicalizePath.  The work done by cmcldeps and 'ninja -t msvc' happens in the compile wrapper, so it's much better to do it then even if the string manipulation is inefficient.

Furthermore it's consistent with ninja's usual approach of pushing this kind of complexity off to the generator.