Ninja + Clang generator causes unnecessary rebuilds in some cases
As discovered in #17271 (closed), Ninja incorrectly parses dependency files generated by Clang if those dependencies contain relative paths (containing A/..
) where the parent directory A
is a symlink that changes the directory level.
Three components can be blamed:
- Ninja: does not correctly expand paths containing
..
, it assumes"a/b/.." == "a"
which is not the case if there is a symlink"b" -> "c/d"
. See https://github.com/ninja-build/ninja/issues/1330 - Clang: produces depfiles containing
..
which GCC does not do. See https://reviews.llvm.org/D37954 for a patch. NOTE: the root cause turns out to be missing prefix canonicalization. That is, ifPATH
contains/bin
before/usr/bin
, and/bin
is a symlink tousr/bin
, then Clang should use/usr/bin
. Instead it uses/bin
. See the discussion on the linked patch. - CMake: assumes that Clang behaves like GCC and passes its depfile to Ninja.
Minimal reproducer which works on Arch Linux: CMakeLists.txt
Reproduce with cmake -GNinja . && ninja && ninja
. The second command should show "nothing to do", but it simply executes the command again. The reason for that:
$ clang++ -MD -MF - -fsyntax-only reproducer.cpp
reproducer.o: reproducer.cpp \
/bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/cstdio \
/bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/x86_64-pc-linux-gnu/bits/c++config.h \
...
$ ninja -t deps
CMakeFiles/reproducer.dir/reproducer.cpp.o: #deps 25, deps mtime 1507299627 (VALID)
reproducer.cpp
/include/c++/7.2.0/cstdio
/include/c++/7.2.0/x86_64-pc-linux-gnu/bits/c++config.h
...
$ readlink -f /bin/../lib64/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/cstdio
/usr/include/c++/7.2.0/cstdio
$ readlink /bin
usr/bin
Suggestion: Test whether Clang* is in use with the Ninja generator, and if so, run another command that resolves the relative paths.
*) Hopefully the issue is solved in Clang 6 with the above patch, but I am still waiting for reviews on that one (that patch is a no-go, a different approach is needed).