mixing depfile absolute paths with manifest relative paths breaks rebuilds

[bradking]

The ninja manual warns against such mixture, but it has become a serious limitation.

The problem is that people want their build systems to invoke the compiler with an absolute path to the source file. This produces better output for IDEs to match error messages and refer back to the original files. It also may affect the paths recorded in debug information. However, this conflicts with ninja's preference for relative paths in the build manifest.

Here is a script demonstrating the issue: ninja-bug.bash.txt

[bradking]

Cc: @nico @chaoren

Over in CMake, there is relevant discussion in issue 16675, issue 13894, and MR 520.

[zlolik]

As workaround you can set object output directory explicitly, even "." or "..", but absolute path works also. ninjatst.bash.txt

[bradking]

@zlolik IIUC you suggest placing absolute paths in the build manifest (build.ninja) graph nodes. Certainly that works (and is what CMake does in out-of-source builds), but my point here is that Ninja discourages use of absolute paths and yet IDEs require them in output.

It is also hypothetically possible that a compiler given -c foo.c could generate an absolute path in the depfile anyway. If that happens then Ninja will not recognize the dependency due to the same problem reported here.

Fixing this will require Ninja to be able to recognize when a relative path and absolute path are the same file and have only one graph node internally instead of two. Doing this only for depfile processing may be sufficient, as it is reasonable to ask the build.ninja generators to name paths consistently within the build manifest.

[zlolik]

@bradking First of all, I am agree with you that we have an issue with ninja and I only suggest workaround. I think there are 3 linked issues:

1. Logic with input absolute/relative path, more details in #1152 .
2. Logic to force output absolute/relative path. The problem with compilers - the only way to said to compiler that you want to have absolute path in output is use absolute path in command line. Not ninja issue but ours.
3. Logic to parse depfile absolute/relative path. This issue.

I use 2 workarounds:

1. Use prefixes $p and $o in path of build.ninja generator to solve first 2 problems. In my case $p and $o are not absolute but relative from current dir where build.ninja generator was run.
2. Use some script to convert depfile to ninja compatible path. Some compilers even need separate call to generate depfile. But there is no absolute path problem while.

[bradking]

For reference, I've started a thread on the ninja-build mailing list with a proposed solution.

[chfast]

Another related issues about debug and coverage information: using commands with relative paths creates also debug information files with relative paths. The problem is the debug files are usually placed next to the object files, not the source files, so the relative paths there reference non-existing files.

[thughes]

Any update on this? There haven't been any responses on the mailing list thread...

[sebknzl]

Hello everyone,

It is also hypothetically possible that a compiler given -c foo.c could generate an absolute path in the depfile anyway.

IAR v8's dependency files contain absolute paths only.

We've hit this problem when doing out-of-source builds with CMake/Ninja/IAR: Generated files which end up in CMake's binary dir are relative in the Ninja manifest, the deps output by IAR are absolute.

To me, this is clearly a Ninja-issue: It shouldn't have two nodes for the same file, no matter what.

I've read @bradking 's suggestion, but I don't really understand the part about symbolic links and why getcwd() can't be used. Would you mind elaborating?

[bradking]

I don't really understand the part about symbolic links and why getcwd() can't be used

@sebknzl the problem is that the absolute paths written to build.ninja by the generator may contain symbolic links that are intentionally not resolved for one reason or another. getcwd() typically returns a path with symlinks resolved. If Ninja were to interpret a relative path in the build manifest or a depfile relative to getcwd() then the absolute path it constructs may not match what the build manifest generator used and we'd still end up with two nodes.

The ninja_workdir I propose in the thread you linked is basically telling Ninja what the generator used as the base for relative paths. In practice realpath(ninja_workdir) == realpath(getcwd()) should always be true, but that does not mean ninja_workdir == getcwd(). Another alternative would be for Ninja to realpath everything before creating nodes, but that would mean $in would not expand to what the generator intended.

[bradking]

@jhasse please see this issue and my proposal to address it.

[ClausKlein]

Why not generate a build.ninja with absolute paths controlled by an CMAKE_NINJA_GENERATOROPTION ?

i.e. like this, which works:

Claus-MBP:build clausklein$ cat build.ninja 

# project dir
p = /Users/clausklein/Downloads
# object dir
o = /Users/clausklein/Downloads/.ninjatst.build

rule cp
  command = cp $in $out

rule cc
  depfile = $out.d
  deps = gcc
  command = cc -I$o -c $in -o $out -MD -MT $out -MF $out.d

# All absolute paths works
build all: phony $o/foo.o

# All absolute paths works
build $o/foo.h: cp $p/foo.h.in
  IN = $p/foo.h.in

# All absolute paths works
build $o/foo.o: cc $p/foo.c || $o/foo.h
  IN = "$p/foo.c"

default all

Claus-MBP:build clausklein$ 

generated with ninjatst.bash.txt

[bradking]

@ClausKlein we did try teaching CMake to use absolute paths. See the discussions I linked in https://github.com/ninja-build/ninja/issues/1251#issuecomment-282322776. I'd rather not add an option that says "do things in a different way that fixes some cases and breaks others".

We really need the Ninja feature in the proposal I linked in https://github.com/ninja-build/ninja/issues/1251#issuecomment-487618865 to fix this properly.

[ClausKlein]

OK, fine

Then would this fix for mesonbuild system available too ;-)

Am 12.03.2020 um 12:56 schrieb Brad King notifications@github.com:

@ClausKlein https://github.com/ClausKlein we did try teaching CMake to use absolute paths. See the discussions I linked in #1251 (comment) https://github.com/ninja-build/ninja/issues/1251#issuecomment-282322776. I'd rather not add an option that says "do things in a different way that fixes some cases and breaks others".

We really need the Ninja feature in the proposal I linked in #1251 (comment) https://github.com/ninja-build/ninja/issues/1251#issuecomment-487618865 to fix this properly.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ninja-build/ninja/issues/1251#issuecomment-598146822, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN7QWWEDHMDA45DQVPLOUDRHDEVDANCNFSM4DBNL6GA.

[jhasse]

Let's say Ninja would use ninja_workdir for matching depfile information and we have the situation where ninja_workdir != getcwd() because of a symlink. What if the compiler now prints absolute paths by using getcwd()?

[bradking]

The compiler should not use getcwd() for anything unless those things were passed in as relative paths, in which case the buildsystem generator likely needs to take responsibility.

Or we can consider trying to normalize w.r.t. both ninja_workdir and getcwd().

[jhasse]

Well, the compiler should also not return an absolute path in the depfile when given relative paths ;)

@ClausKlein we did try teaching CMake to use absolute paths. See the discussions I linked in #1251 (comment). I'd rather not add an option that says "do things in a different way that fixes some cases and breaks others".

What exactly broke when you used absolute paths as suggested by @ClausKlein?

[bradking]

If the compiler is given absolute paths then the depfile it generates has absolute paths, and those are not reconciled with relative paths to headers in the build manifest, e.g. for generated headers. See the ninja-bug.bash.txt link in my first post at the top of this issue. It shows a step-by-step example of what happens and is expressed purely in terms of Ninja and no CMake.

[jhasse]

I've read that, but why not use https://github.com/ninja-build/ninja/issues/1251#issuecomment-284533599? Simply because Ninja "discourages" absolute paths? If so, this is about a documentation change?

[bradking]

build.ninja files are much smaller without repeating absolute paths everywhere, and that makes them faster. They also look nicer, are easier to debug, and are more appropriate for tab-completion of build target names in shells. IMO it is a good recommendation for Ninja to make. Ninja just needs a bit more information to reconcile this case, and that is what I propose.

[ClausKlein]

@bradking relative paths may not always short:

build jsoncpp@sha/src_lib_json_json_reader.cpp.o: cpp_COMPILER ../../../../../Users/clausklein/Workspace/cpp/jsoncpp/src/lib_json/json_reader.cpp
 DEPFILE = jsoncpp@sha/src_lib_json_json_reader.cpp.o.d
 ARGS = -Ijsoncpp@sha -I. -I../../../../../Users/clausklein/Workspace/cpp/jsoncpp -I../../../../../Users/clausklein/Workspace/cpp/jsoncpp/include -I../../../../../Users/clausklein/Workspace/cpp/jsoncpp/dir1 -I../../../../../Users/clausklein/Workspace/cpp/jsoncpp/dir2 -fdiagnostics-color=always -pipe -Wall -Winvalid-pch -Wnon-virtual-dtor -std=c++17 -O3

my example https://github.com/ninja-build/ninja/issues/1251#issuecomment-597877775 looks nicer!

[bradking]

CMake's generators have a rule about not using ../ sequences that leave the build tree. In fully out-of-source builds we do use absolute paths to the source tree and relative paths within the build tree. The latter never start in ../ because they are under the build tree.

[ClausKlein]

I know, it was generated with mesonbuild. ;-)

But than, why not deny the generation into source tree?

[bradking]

We can debate the merits of relative v. absolute paths forever. Ninja should not be opinionated about that. The problem here is reconciling cases where both are used within a single build. The buildsystem generator cannot always control what the tools will do with paths. Ninja should support mixed absolute/relative paths to the same file and can do so with a simple approach I've outlined in my proposal. For the motivating use case the changes are isolated to depfile parsing.

[ClausKlein]

Ninja should support mixed absolute/relative paths to the same file

I agree!

[jhasse]

Thanks for all the explanations, I'm starting to understand.

What if Ninja would (upon encountering an absolute path in the depfile) realpath both cwd and the path it found? Then it would remove the realpathed cwd from the realpathed depfile-path to get the relative path to match against the generated header. Would that work?

[bradking]

getcwd() is always a realpath already so that step isn't necessary. If Ninja were to realpath() the depfile-provided absolute path and check for a cwd prefix that could work. However, realpath() is a possibly-expensive syscall while my proposed ninja_workdir can reconcile paths purely in memory via string processing.

[bradking]

Mixing in the other direction should also be allowed: an absolute path in the build manifest but a relative path in the depfile. I don't think Ninja should have to realpath() every absolute path while parsing build.ninja. That would probably be slow.

[jhasse]

We could use the result to determine the workdir. E.g.:

getcwd(): /var/xyz/build depfile entry: /home/foo/build/bar.h realpath(depfile entry): /var/xyz/build/bar.h relative realpath: bar.h remove that from the depfile entry: /home/foo/build -> ninja_workdir

This way we only need to call realpath once.

[bradking]

I do not think deriving ninja_workdir automatically is possible in general. How do we know how many logical components of the original symlink-containing logical path correspond to the workdir? We might be able to do it by taking one component at a time until its realpath matches the cwd, but that feels hacky and error prone. It feels risky to trust the first symlink that happens to look like this. Also we'd have to run realpath() on every absolute path we encounter until one resolves this way.

The buildsystem generator knows exactly what logical path to the workdir it used when generating absolute paths in build statements and command lines in build.ninja. It is therefore the most reliable source of information for the workdir path. We just need a way to communicate this information to Ninja, hence my proposed ninja_workdir binding.

[jhasse]

I'm not sure I understand, sorry. Can you give an example where my detection logic wouldn't work?

[bradking]

It could break if the buildsystem generator puts a symlink inside the build tree (pointing elsewhere in the build tree or outside altogether):

getcwd(): /var/xyz/build depfile entry: /home/foo/build/subdir-link/bar.h realpath(depfile entry): /var/xyz/build/subdir-real/bar.h relative realpath: subdir-real/bar.h remove that from the depfile entry: ???

CMake does not do this but in general a buildsystem generator could.

Even if we are willing to say that is not supported, the detection could still face:

getcwd(): /var/xyz/build 1000 depfile entries: /home/foo/ext-lib/bar{1,2,...}.h 1000 realpath(depfile entries): /var/xyz/ext-lib/bar{1,2,...}.h 1000 relative realpaths: none under cwd

Now imagine that appears in 1000 sources' depfiles and there is never a hit. That is 1 million realpath() calls with no match for this heuristic. And that repeats every time a source is rebuilt and its depfile re-parsed.

[jhasse]

symlink inside the build tree -> yes, we can't really support that, it will result in all kinds of other issues anyway (without realpath-ing all over the place).

The second example sounds rather artificial. Normally depfiles start with the local dependencies, not with system headers.

If it really turns out to be a performance problem, we could cache the workdir in .ninja_deps?

[bradking]

In an out-of-source build very few depfile entries will even point at the build tree. This will be very common:

getcwd(): /var/xyz/build 1000 depfile entries: /home/foo/project-source/bar{1,2,...}.h 1000 realpath(depfile entries): /var/xyz/project-source/bar{1,2,...}.h 1000 relative realpaths: none under cwd

Ninja will have to call realpath() on all of those just in case, or maintain some kind of table of results to avoid the actual syscalls. It cannot know ahead of time when it will encounter a symlink path that resolves to the cwd, and does not even know if it will find one.

[jhasse]

Given that mixing absolute and realtive paths in the depfile currently doesn't work, I don't see how that would be "very common"??

[bradking]

depfiles of the form in my example are already very common even when not using symlinks or mixed paths. I have them in most of my build trees. Ninja can't know ahead of time whether the heuristic is needed so it will have to block on realpath() syscalls just in case.

[jhasse]

Maybe I misunderstood your example. Do you mean one depfile with 3000 entries or were those 3 seperate examples for depfiles with 1000 entries each?

[bradking]

I meant 1000 depfile entries spread across any number of depfiles. It could be one. It could be many. There could be 10000 entries across a project. Their paths can point anywhere on the filesystem and may or may not ever actually point into the build tree.

[jhasse]

Can you give an example CMakeLists.txt that results in such a mixture of paths (doesn't have to be 1000, just one of each)?

[bradking]

It's hard to show in a CMakeLists.txt file because CMake currently takes care to avoid mixing paths in most cases. However, there are issues in CMake's issue tracker about problems that cannot be fixed without Ninja fixing this first. See for example CMake Issue 13894. Users want to be able to give absolute paths to the compiler even for sources (and perhaps include directories) in the build tree, or when doing in-source builds. This works fine with CMake's Makefile generator. If we do that with Ninja then the depfiles come back with absolute paths and they don't match the relative paths in build.ninja. Maybe we could switch to absolute paths for everything, but as discussed above here we shouldn't have to do that.

[ClausKlein]

Perhaps you may try this for tests https://github.com/ClausKlein/build-performance/commit/8013836486e3a459474eb374f6c3da5e20983443

The problem shown here is based on https://github.com/ninja-build/ninja/issues/1251#issue-210080507 It generates as much target as you want. i.e. 2 subduers each with 2 source files:

PWD=/Users/clausklein/Workspace/build-performance/build

rule cp
  command = cp $in $out

rule cc
  depfile = $out.d
  deps = gcc
  command = gcc -c -I$PWD $IN -o $out $FLAGS -MMD -MT $out -MF $out.d

rule link
  command = gcc -o $out $in $LINK_PATH $LINK_LIBRARIES

build foo: phony foo.h
build foo.h: cp foo.h.in

build bin/main0.o: cc /Users/clausklein/Workspace/build-performance/build/subdir0/main.c || foo.h
  IN = /Users/clausklein/Workspace/build-performance/build/subdir0/main.c
build /Users/clausklein/Workspace/build-performance/build/subdir0/file0.o: cc /Users/clausklein/Workspace/build-performance/build/subdir0/file0.c || foo.h
  IN = /Users/clausklein/Workspace/build-performance/build/subdir0/file0.c

build bin/main1.o: cc /Users/clausklein/Workspace/build-performance/build/subdir1/main.c || foo.h
  IN = /Users/clausklein/Workspace/build-performance/build/subdir1/main.c
build /Users/clausklein/Workspace/build-performance/build/subdir1/file0.o: cc /Users/clausklein/Workspace/build-performance/build/subdir1/file0.c || foo.h
  IN = /Users/clausklein/Workspace/build-performance/build/subdir1/file0.c

build bin/main0: link bin/main0.o /Users/clausklein/Workspace/build-performance/build/subdir0/file0.o 
build bin/main1: link bin/main1.o /Users/clausklein/Workspace/build-performance/build/subdir1/file0.o 

build all: phony foo bin/main0 bin/main1 
default all

[jhasse]

This works fine with CMake's Makefile generator.

How does Make handle the symlink issue?

[bradking]

How does Make handle the symlink issue?

The Makefile generator is not currently using depfiles and does its own approximate scanning. That will likely be updated as part of adding C++20 modules support to the Makefile generator, at which point the interpretation of depfile paths will have to be reconciled similarly to what is proposed here.

[bradking]

One option is to skip ninja_workdir for now and at least teach Ninja to reconcile the mixed paths assuming that getcwd() is the top. That way at least the mixed paths will work in the common no-symlink case. Then ninja_workdir can be added to support the more complex cases later.

[GrandChris]

Pleas someone fix this. I'm so annoyed because of this.

I'm using ARM-GCC with CMake and VSCode. When I use an in-source build, the paths are relative to the "build" directory (instead of the root directory), which causes VSCode to no longer be able to resolve the paths, which means, you can't just click the paths in the command line window, instead, you have to locate each file yourself. When I use a CMake out-of-tree build, it breaks the "IntelliSense" of Eclipse and makes problems with CMake subprojects. My solution for now is to use MakeFiles, because it uses absolute Paths per default. But Ninja is better, it produces better readable files (rules.ninja...) and the build output on the command line is better. Make has problems with outputting warnings on the command line in a multithreaded context, means, multiple warnings are interleaved. Which causes me to build single threaded, which is again, super annoying.

[ClausKlein]

Pleas someone fix this. I'm so annoyed because of this.

I'm using ARM-GCC with CMake and VSCode. When I use an in-source build, the paths are relative to the "build" directory ...

There exists a workaround: chose the build dir outside the source tree! i.e. $TMPDIR/...

[GrandChris]

Thank you ClausKlein, but the workaround is an out of source build, but I need the files inside my tree, because it is referenced by other scripts. Also it is a non-standard build requirement.

My solution for now is, to change CMake to just always use absolute paths. I don't understand why there isn't an option for this. However, if somebody else needs this, here is the patch of CMake v3.18.1:

@@ -241,7 +241,7 @@ void cmNinjaTargetGenerator::AddIncludeFlags(std::string& languageFlags,
   // Add include directory flags.
   std::string includeFlags = this->LocalGenerator->GetIncludeFlags(
     includes, this->GeneratorTarget, language,
-    language == "RC", // full include paths for RC needed by cmcldeps
+    true, // full include paths for RC needed by cmcldeps
     false, config);
   if (this->GetGlobalGenerator()->IsGCCOnWindows()) {
     std::replace(includeFlags.begin(), includeFlags.end(), '\\', '/');
@@ -1133,8 +1133,7 @@ void cmNinjaTargetGenerator::WriteObjectBuildStatement(
   const std::string& fileConfig, bool firstForConfig)
 {
   std::string const language = source->GetLanguage();
-  std::string const sourceFileName =
-    language == "RC" ? source->GetFullPath() : this->GetSourceFilePath(source);
+  std::string const sourceFileName = source->GetFullPath();
   std::string const objectDir = this->ConvertToNinjaPath(
     cmStrCat(this->GeneratorTarget->GetSupportDirectory(),
              this->GetGlobalGenerator()->ConfigDirectory(config)));

[ClausKlein]

Attention, there was a patch for cmake, but see https://github.com/ninja-build/ninja/issues/1251

We need a patch for Ninja!

[GrandChris]

In my last comment, I showed a patch for CMake to use absolute paths instead of relative paths. Turns out this is a bad idea, because it breaks custom CMake header dependencies with Ninja (for example a generated header file is dependent on a .json file). Guess this problem derives from this issue here, which I was too stupid to understand.

[GrandChris]

If someone is interested about it, I now use Ninja with relative paths. I have created an extension for VSCode to be able to click relative paths inside the integrated terminal. extension

[weliveindetail]

@GrandChris Great! Any chance to get it to work in the Output tab too?