View Issue Details Jump to Notes ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0014972CMakeCMakepublic2014-06-12 14:022015-04-06 09:07
ReporterAdam Strzelecki 
Assigned ToBrad King 
PrioritynormalSeverityminorReproducibilityalways
StatusclosedResolutionfixed 
PlatformAppleOSOSXOS Version10.9.3
Product VersionCMake 3.0 
Target VersionCMake 3.1Fixed in VersionCMake 3.1 
Summary0014972: Ninja gen generates invalid phony dependencies for source files with in-source build
DescriptionRelated commit
==========

539356f1 Ninja: Custom Command file depends don't need to exist before building

Description
=======

Commit above creates phony dependencies for all files that are dependencies to project outputs and reside in build folder. Unfortunately when using in-source build this behavior causes all source files to have phony dependency which is undesired behavior.

I don't see simple workaround, as I frankly don't understand the reasoning behind the change above.
Steps To ReproduceSetup following for in-source build with Ninja generator, then inspect generated build.ninja has phony dependency for test.cc, which is undesired.

CMakeLists.txt
=========
cmake_minimum_required(VERSION 2.6)
project(NinjaTest CXX)
add_executable(ninja_test test.cc)

test.cc
=====
int main(int argc, char const *argv[]) { return 0; }
TagsNo tags attached.
Attached Filespatch file icon 0001-Ninja-Remove-CMake-includes-from-explicit-depends.patch [^] (1,534 bytes) 2014-06-13 06:19 [Show Content]
patch file icon 0002-Ninja-Generate-phony-rule-only-for-CMakeFiles.patch [^] (1,940 bytes) 2014-06-13 06:19 [Show Content]
patch file icon 0003-Ninja-Consider-only-custom-commands-deps-as-side-eff.patch [^] (4,176 bytes) 2014-06-23 18:28 [Show Content]
patch file icon 0004-Ninja-Don-t-limit-custom-cmd-side-effects-to-build-f.patch [^] (2,086 bytes) 2014-06-23 18:29 [Show Content]
patch file icon 0005-Ninja-Skip-generating-empty-phony-rules.patch [^] (4,008 bytes) 2014-06-26 06:57 [Show Content]

 Relationships
related to 0014963closedBrad King Add explicit specification of custom command side effect outputs 
related to 0015216closedKitware Robot Ninja makes bad phony targets with parallel "sub" directory and custom CMAKE_LIBRARY_OUTPUT_DIRECTORY 

  Notes
(0036180)
Adam Strzelecki (reporter)
2014-06-13 06:21
edited on: 2014-06-13 14:30

Attaching patches that fix this issue, also fixes FindCUDA problems described at:

http://public.kitware.com/pipermail/cmake-developers/2014-June/010652.html [^]

(0036187)
Brad King (manager)
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 (reporter)
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 (manager)
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 (reporter)
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 (reporter)
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 (manager)
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 (reporter)
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 (manager)
2014-06-24 13:26

Re 0014972:0036236: Custom commands may generate sources that are then compiled later, so it is not only dependencies of other custom commands that need phony targets.

This will be much simpler if Ninja were to behave as I propose here:

 https://github.com/martine/ninja/issues/760#issuecomment-46540858 [^]
(0036258)
Adam Strzelecki (reporter)
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 (manager)
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 (reporter)
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 (manager)
2014-06-30 09:34

Re 0014972:0036279: Thanks. For reference, the patch thread is here:

 http://thread.gmane.org/gmane.comp.programming.tools.cmake.devel/10407 [^]
(0036289)
Brad King (manager)
2014-06-30 09:38

Re 0014972:0036288: I've applied the patches and merged for testing here:

 Ninja: Consider only custom commands deps as side-effects
 http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=a33cf6d0 [^]

 Ninja: Don't limit custom cmd side-effects to build folder
 http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=7243c951 [^]

 Ninja: Skip generating empty phony rules
 http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=93371ed5 [^]
(0036294)
Brad King (manager)
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 (manager)
2014-10-08 09:18

Re 0014972:0036289: We've had to revert commit 7243c951:

 Ninja: Limit custom command side-effects to build folder
 http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=de8e534b [^]

See the mailing list thread here:

 http://thread.gmane.org/gmane.comp.programming.tools.cmake.devel/11208 [^]

I'm leaving this issue resolved because that change was incidental and not part of fixing this issue. See 0014963 for discussion of the underlying problem.
(0038426)
Robert Maynard (manager)
2015-04-06 09:07

Closing resolved issues that have not been updated in more than 4 months.

 Issue History
Date Modified Username Field Change
2014-06-12 14:02 Adam Strzelecki New Issue
2014-06-13 06:19 Adam Strzelecki File Added: 0001-Ninja-Remove-CMake-includes-from-explicit-depends.patch
2014-06-13 06:19 Adam Strzelecki File Added: 0002-Ninja-Generate-phony-rule-only-for-CMakeFiles.patch
2014-06-13 06:21 Adam Strzelecki Note Added: 0036180
2014-06-13 06:21 Adam Strzelecki Note Added: 0036181
2014-06-13 08:33 Adam Strzelecki Note Added: 0036183
2014-06-13 08:50 Brad King Note Added: 0036187
2014-06-13 08:51 Brad King Relationship added related to 0014963
2014-06-13 09:05 Adam Strzelecki Note Added: 0036188
2014-06-13 09:56 Brad King Note Added: 0036192
2014-06-13 11:54 Adam Strzelecki Note Added: 0036195
2014-06-13 14:30 Adam Strzelecki Note Deleted: 0036181
2014-06-13 14:30 Adam Strzelecki Note Deleted: 0036183
2014-06-13 14:30 Adam Strzelecki Note Edited: 0036180
2014-06-13 14:31 Adam Strzelecki Note Edited: 0036195
2014-06-17 12:16 Adam Strzelecki Note Added: 0036215
2014-06-18 18:34 Brad King Note Added: 0036222
2014-06-23 18:28 Adam Strzelecki File Added: 0003-Ninja-Consider-only-custom-commands-deps-as-side-eff.patch
2014-06-23 18:29 Adam Strzelecki File Added: 0004-Ninja-Don-t-limit-custom-cmd-side-effects-to-build-f.patch
2014-06-23 18:32 Adam Strzelecki Note Added: 0036236
2014-06-24 13:26 Brad King Note Added: 0036243
2014-06-26 06:54 Adam Strzelecki Note Added: 0036258
2014-06-26 06:57 Adam Strzelecki File Added: 0005-Ninja-Skip-generating-empty-phony-rules.patch
2014-06-27 09:51 Brad King Note Added: 0036267
2014-06-27 16:22 Adam Strzelecki Note Added: 0036279
2014-06-30 09:34 Brad King Note Added: 0036288
2014-06-30 09:38 Brad King Note Added: 0036289
2014-06-30 09:39 Brad King Assigned To => Brad King
2014-06-30 09:39 Brad King Status new => assigned
2014-06-30 09:39 Brad King Target Version => CMake 3.1
2014-07-01 09:27 Brad King Note Added: 0036294
2014-07-01 09:28 Brad King Status assigned => resolved
2014-07-01 09:28 Brad King Resolution open => fixed
2014-07-01 09:28 Brad King Fixed in Version => CMake 3.1
2014-10-08 09:18 Brad King Note Added: 0037004
2014-10-22 10:24 Brad King Relationship added related to 0015216
2015-04-06 09:07 Robert Maynard Note Added: 0038426
2015-04-06 09:07 Robert Maynard Status resolved => closed


Copyright © 2000 - 2018 MantisBT Team