-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mixing depfile absolute paths with manifest relative paths breaks rebuilds #1251
Comments
Over in CMake, there is relevant discussion in issue 16675, issue 13894, and MR 520. |
As workaround you can set object output directory explicitly, even "." or "..", but absolute path works also. |
@zlolik IIUC you suggest placing absolute paths in the build manifest ( It is also hypothetically possible that a compiler given 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 |
@bradking First of all, I am agree with you that we have an issue with ninja and I only suggest workaround.
I use 2 workarounds:
|
For reference, I've started a thread on the |
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. |
Any update on this? There haven't been any responses on the mailing list thread... |
Hello everyone,
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 |
@sebknzl the problem is that the absolute paths written to The |
@jhasse please see this issue and my proposal to address it. |
Why not generate a build.ninja with absolute paths controlled by an CMAKE_NINJA_GENERATOR_OPTION_ ? 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 |
@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". We really need the Ninja feature in the proposal I linked in #1251 (comment) to fix this properly. |
OK, fine
Then would this fix for mesonbuild system available too ;-)
… Am 12.03.2020 um 12:56 schrieb Brad King ***@***.***>:
@ClausKlein <https://github.com/ClausKlein> we did try teaching CMake to use absolute paths. See the discussions I linked in #1251 (comment) <#1251 (comment)>. 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) <#1251 (comment)> to fix this properly.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#1251 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN7QWWEDHMDA45DQVPLOUDRHDEVDANCNFSM4DBNL6GA>.
|
Let's say Ninja would use |
The compiler should not use Or we can consider trying to normalize w.r.t. both |
Well, the compiler should also not return an absolute path in the depfile when given relative paths ;)
What exactly broke when you used absolute paths as suggested by @ClausKlein? |
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 |
I've read that, but why not use #1251 (comment)? Simply because Ninja "discourages" absolute paths? If so, this is about a documentation change? |
|
@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 #1251 (comment) looks nicer! |
CMake's generators have a rule about not using |
I know, it was generated with mesonbuild. ;-) But than, why not deny the generation into source tree? |
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. |
I agree! |
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? |
|
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 |
We could use the result to determine the workdir. E.g.: getcwd(): This way we only need to call realpath once. |
I do not think deriving The buildsystem generator knows exactly what logical path to the workdir it used when generating absolute paths in build statements and command lines in |
CMake 3.20 changes the behavior of AUTOMOC in a subtle way (https://gitlab.kitware.com/cmake/cmake/-/issues/21977) which revealed a circular dependency between the autogen target and the files generated by qt5_wrap_ui(). The circular dependency always existed, but did not appear until CMake 3.20 due to a bug in Ninja (ninja-build/ninja#1251). Resolve this issue by using AUTOUIC as well as AUTOMOC, and not combining the CMake AUTOMOC approach with the Qt qt5_wrap_ui() approach.
CMake 3.20 changed the behavior of AUTOMOC in a subtle way (https://gitlab.kitware.com/cmake/cmake/-/issues/21977) which revealed a circular dependency between the autogen target and the files generated by qt5_wrap_ui(). The circular dependency always existed, but did not appear until CMake 3.20 due to a bug in Ninja (ninja-build/ninja#1251). Resolve this issue by using AUTOUIC as well as AUTOMOC, and not combining the CMake AUTOMOC approach with the Qt qt5_wrap_ui() approach.
Ninja silently ignoring this is a huge pain point. Maybe we should add an option |
I'm for merging the mentioned PR - #1924 It solves the problem many users have and it doesn't hurt ninja in any way. I really don't understand why it's so hard to fix ninja issues. |
I'm experiencing problems with my dev environment for years now exactly because of this issue. It's not a deal-breaker per se, I still very much prefer using Tooling is based on LLVM/Clang infrastructure (libclang) and uses
This is either bad for the end users (devs using the tooling which do not have workarounds) or it's bad for the tooling devs because they have to maintain it and make sure it does not break in future releases. I think that giving a chance to implant this behavior in |
by CMake or the compdb tool? |
|
+1. We have the same usage and issue. Commands generated by ninja have relative paths for the dependencies in the exported |
When the build tree is placed inside the source tree, CMake chooses to use relative paths starting in |
@bradking It would be really nice to get this fixed somehow. To be honest I'm just an end user of both CMake and Ninja so I don't understand the issue in full detail, but it has been annoying me for years. I started using unix makefiles generator again, because of it, and I don't mind longer build times if the tooling around it works properly. I don't really expect the PR to be merged, so maybe CMake defaulting to absolute paths or giving users some option to do so? |
@bradking Above was just an example of command. In reality I am using out-of-source builds and the issue with relative paths persists. Repro:
First obvious difference is in |
@JBakamovic thanks for the example. However, CMake's behavior would be best discussed over in CMake's issue tracker. See CMake Issue 13894. |
There is a [ninja #1251] issue that forces cmake to generate build files with relative instead of absolute paths. The effect is that debug information in binaries are relative to the build directories, which for out-of-source builds is not very compatible with running vim from the project root. When running vim from the project root the paths reported for build errors, test failures, and coverage information is wrong. [ninja #1251]: ninja-build/ninja#1251
In the same way as @kobalicek, I am the end user of CMake using Ninja as a backend. I also had to switch our usage of CMake to What really disappoints me is a 4-year holy war around relative vs absolute paths resulting in nothing. I believe the tool shall give choices. @bradking I am really thankful for your efforts in pushing this issue for such a long time. |
CMake 3.21 includes a fix to CMake Issue 13894 that does not require changes to Ninja. Instead CMake uses a workaround to this issue by expressing outputs as both relative and absolute paths. It causes Ninja to be less efficient by |
@bradking That sounds great, I'd also like to thank you for your efforts on this issue. I've been following this thread for months now, personally I've already given up that there will ever be a solution. |
@bradking I've been eager and tried the CMake 3.21.0 version which uses absolute paths and is great. What I noticed is that dependencies no longer work with generated headers. |
@GrandChris please open an issue in CMake's issue tracker for that. |
@bradking @GrandChris Has the issue with generated headers been filed in CMake's issue tracker? Could not find anything related. |
@dvolosnykh it is CMake Issue 22454, and turned out not to be a regression or a problem caused by the switch to absolute paths. |
I'm having trouble with our custom generator where i try to support both in tree and completely out of tree builds. To me it seems that I have no control over when gcc starts outputting absolute paths in the .d files, which then breaks incremental builds badly. I have taken great pains to use relative paths everywhere but despite that plain old gcc (on windows, but still) starts to spit out absolute paths seemingly when it finds a header using a relative path that 'touches root' or something like that. I'd like to be able to point the generator with e.g. --builddir=/temp/testbuild while running the configurator (and ninja) from the source tree. The problem is that I generate headers during the build that I naturally place in the build directory (which then is referenced by e.g. -I../../temp/testbuild, i.e. relatively), but despite that gcc insists on switching to an absolute path for headers found from that build directory. If I run the same build with e.g. --builddir=build, i.e. on top of the source tree, then gcc spits out a relative path for the same header. If I could work out the exact logic how and when gcc generates a relative or an absolute path (well I guess I could look at the source...) I suppose I should be able to work around it in the generator, but it would be so much easier if ninja could handle it. |
Hello @motoz , relative paths are fine, if all paths start at the root of your project directory. If this is not possible, e.g. because some tool insists of starting all paths from a build directory, I highly recommend using absolute paths for everything. Otherwise you will keep fighting all sorts of issues with your paths. |
Thanks @GrandChris for the pointer. Switching everything to absolute paths really works, at least so far I haven't came across something that doesn't. And it also seems to be the only way to make generated headers work with gcc, which really goes against the recommendation to use relative paths in the ninja documentations. The doc was probably the reason I went with all relative paths (them being generally shorter and nicer to read another). |
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
The text was updated successfully, but these errors were encountered: