|
Notes |
|
|
(0036180)
|
|
Adam Strzelecki
|
2014-06-13 06:21
(edited on: 2014-06-13 14:30) |
|
|
|
|
(0036187)
|
|
Brad King
|
|
2014-06-13 08:50
|
|
|
CMake generates these phony dependencies so that Ninja does not complain about sources that do not exist when it starts but that are generated as side-effects of other rules and are available by the time they are needed due to order-only dependencies. See 0014963 and the related issues. This mess is due to a fundamental difference in the model CMake has historically used in such cases and the model Ninja has. |
|
|
|
(0036188)
|
|
Adam Strzelecki
|
|
2014-06-13 09:05
|
|
Yes I understand that. But if I understand correctly this applies only to files that are generated during build and not set to be output of some command.
Unfortunatelly current implementation considers ALL source files as such files when doing in-source build, because Output = Source dir. Which is IMHO wrong. Therefore patch 0002 considers only files residing inside CMakeFiles as build time generated files which should be generally OK, since CMake is expected put all generated files there.
0001 patch however fixes a real bug where there are two phony rules generated for files that is both (1) included by CMakeLists.txt (2) explicit dependency for some command.
Let me know if it is clear enough. |
|
|
|
(0036192)
|
|
Brad King
|
|
2014-06-13 09:56
|
|
Re 0014972:0036188: CMake puts most of the files it generates as implementation details into CMakeFiles/ but the project code is free to generate files anywhere in the build tree. I agree that extra phony deps on sources for in-source builds are wrong but I do not think there is anything that can be done to fix it without breaking other use cases.
Meanwhile I've applied 0001 here:
Ninja: Remove CMake includes from explicit depends
http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=393babd7 [^]
with a small change to add CMakeCache.txt to the known implicit deps too. |
|
|
|
(0036195)
|
|
Adam Strzelecki
|
2014-06-13 11:54
(edited on: 2014-06-13 14:31) |
|
Re 0014972:0036192
Another problem is that build graph will be different between in-source and out-of-source, and I think this may be a problem both for developers and users, since this may trigger different behavior for both cases.
If we stick however only to CMakeFiles/ then we should have same build-graph in both cases.
|
|
|
|
(0036215)
|
|
Adam Strzelecki
|
|
2014-06-17 12:16
|
|
0014972:0036187 Honestly I tried latest CMake build and non of the 0014963 related issues are fixed. So I still don't see point of creating this phony rules.
I think the whole point of making phony rules for all explicit dependencies is invalid because they don't help to resolve the issue.
Also if someone is not specifying command outputs I don't see why Ninja should restat their output!?
Finally these explicit rules breaks fundamental behavior that Ninja should complain some file is missing, but here we add these phony rules to make Ninja run commands even their dependencies ARE MISSING.
This really does not make sense to me. |
|
|
|
(0036222)
|
|
Brad King
|
|
2014-06-18 18:34
|
|
Re 0014972:0036215: I posted an example of the history of this problem in the ninja issue here:
https://github.com/martine/ninja/issues/760#issuecomment-46501896 [^]
The build.ninja code is:
rule R
command = $COMMAND
# restat = 1 # simulate lack of feature by not specifying restat
build output-stamp output: R | input
COMMAND = if ! test -f output || ! diff input output >/dev/null ; then cp input output; fi &&
touch output-stamp
build use-output: R output || output-stamp
COMMAND = cp output use-output
default use-output
As I explained over there, CMake does not put the explicit 'output' on the 'output-stamp' rule because it evolved for build systems with no restat feature and therefore does not have a way for projects to specify such side effect outputs (see 0014963). Without the explicit 'output' then on the first build ninja complains for the 'use-output' rule that 'output' does not exist and there is no rule to build it. Therefore we need to have a phony target for 'output' to convince ninja to proceed. With a restat-dependencies-after-order-only feature in ninja, CMake could use that to convince ninja to proceed with the build instead (because the order-only dependencies ensure that 'output' will exist in time) and we would not need the bogus phony targets. |
|
|
|
(0036236)
|
|
Adam Strzelecki
|
|
2014-06-23 18:32
|
|
Re 0014972:0036222: After re-thinking it, I believe we should only consider custom commands (targets) dependencies as possible side-effects, NOT all dependencies. So regular compile sources (done by CMake itself) should be get phony rules. (see patch 0003)
Also since custom command may write wherever it wants to (i.e. temp folder, source folder) why limiting only to build folder ? (see patch 0004). |
|
|
|
(0036243)
|
|
Brad King
|
|
2014-06-24 13:26
|
|
|
|
|
(0036258)
|
|
Adam Strzelecki
|
|
2014-06-26 06:54
|
|
Re 0014972:0036243: I am sorry but I disagree, if it isn't custom command dependency but compilation dependency CMake will fail it it can't find the source unless source is marked GENERATED, but generated sources generate their own phony rules at beginning WriteAssumedSourceDependencies.
So I think my patch is correct.
Consider following sample with set_source_files_properties commented out:
cmake_minimum_required(VERSION 2.6)
project(NinjaTest)
# set_source_files_properties(test.cc
# PROPERTIES GENERATED TRUE)
add_custom_target(gettest
COMMAND echo 'int main() { return 0 }' > test.cc
)
add_executable(ninjatest test.cc)
add_dependencies(ninjatest gettest)
Then running cmake -GNinja gives:
-- Configuring done
CMake Error at CMakeLists.txt:8 (add_executable):
Cannot find source file:
test.cc
Re-enabling GENERATED for test.cc we get following statement in build.ninja:
#############################################
# Assume dependencies for generated source file.
build test.cc: CUSTOM_COMMAND cmake_order_depends_target_ninjatest
Therefore WriteUnknownExplicitDependencies should only apply to custom command dependencies. |
|
|
|
(0036267)
|
|
Brad King
|
|
2014-06-27 09:51
|
|
|
Re 0014972:0036258: Good, thanks for looking into that concern. Please revise the commit message to explain how WriteAssumedSourceDependencies takes care of compilation dependencies. Then post the patch to the list. |
|
|
|
(0036279)
|
|
Adam Strzelecki
|
|
2014-06-27 16:22
|
|
|
Re 0014972:0036267: As you have suggested I revised commit messages and post series of 3 patched to ML. |
|
|
|
(0036288)
|
|
Brad King
|
|
2014-06-30 09:34
|
|
|
|
|
(0036289)
|
|
Brad King
|
|
2014-06-30 09:38
|
|
|
|
|
(0036294)
|
|
Brad King
|
|
2014-07-01 09:27
|
|
|
Re 0014972:0036289: These commits are now in 'master'. AFAICS they resolve the specific problem raised in this issue. |
|
|
|
(0037004)
|
|
Brad King
|
|
2014-10-08 09:18
|
|
|
|
|
(0038426)
|
|
Robert Maynard
|
|
2015-04-06 09:07
|
|
|
Closing resolved issues that have not been updated in more than 4 months. |
|
0001-Ninja-Remove-CMake-includes-from-explicit-depends.patch (1,534) 2014-06-13 06:19