MantisBT - CMake
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0010048CMakeCMakepublic2009-12-14 05:252012-11-03 13:58
ReporterMichael Wild 
Assigned ToAlex Neundorf 
PrioritynormalSeveritymajorReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target VersionCMake 2.8.11Fixed in Version 
Summary0010048: ADD_CUSTOM_COMMAND cannot handle multiple IMPLICIT_DEPENDS files
DescriptionThe data-structures in CMake allow for only on IMPLICIT_DEPENDS file in a ADD_CUSTOM_COMMAND call, only the last file is preserved (later files overwrite earlier ones). Further, the cmDepends::Write() and cmDependC::WriteDependencies() functions assume that there is only a single source file for every produced file, which is not true for the IMPLICIT_DEPENDS case. Thus multiple entries for every product are entered into depend.internal and depend.make. However, the parsing algorithm of depend.internal in cmDepends::CheckDependencies assumes that every product is only listed once, causing earlier dependencies to be overwritten by the last occurrence.
Steps To Reproduce
Additional InformationThe attached patch (against 2.8.0) does the following:

- Expand the documentation of ADD_CUSTOM_COMMAND to mention that the language has to be prefixed in front of every file listed.

- Change the function cmLocalUnixMakefileGenerator3::AddImplicitDepends() and the type cmLocalUnixMakefileGenerator3::ImplicitDependFileMap to allow for multiple IMPLICIT_DEPENDS files.

- In cmDepends::Write() accumulate all the source files for a single product into a std::set<std::string> and pass that set to cmDepends::WriteDependencies().

- In cmDependsC::WriteDependencies() accumulate all the dependencies into a std::set<std::string> and write the depend.internal and depend.make files at the very end, avoiding multiple entries of a single product.
TagsNo tags attached.
Relationships
related to 0012619closed Alex Neundorf cmDependsC class Scan ignore same file name 
Attached Filespatch 0001-FIX-Preserve-implicit-dependencies-and-collect-all-d.patch (22,657) 2009-12-14 05:25
https://public.kitware.com/Bug/file/2718/0001-FIX-Preserve-implicit-dependencies-and-collect-all-d.patch
patch 0001-FIX-Preserve-implicit-dependencies-and-collect-all-d-rewrapped.patch (27,159) 2009-12-14 11:10
https://public.kitware.com/Bug/file/2719/0001-FIX-Preserve-implicit-dependencies-and-collect-all-d-rewrapped.patch
tgz fix-IMPLICIT_DEPENDS-patches-v3.tgz (6,936) 2009-12-14 14:39
https://public.kitware.com/Bug/file/2721/fix-IMPLICIT_DEPENDS-patches-v3.tgz
tgz fix-IMPLICIT_DEPENDS-patches-v4.tgz (7,005) 2009-12-15 00:46
https://public.kitware.com/Bug/file/2725/fix-IMPLICIT_DEPENDS-patches-v4.tgz
Issue History
Date ModifiedUsernameFieldChange
2009-12-14 05:25Michael WildNew Issue
2009-12-14 05:25Michael WildFile Added: 0001-FIX-Preserve-implicit-dependencies-and-collect-all-d.patch
2009-12-14 08:59Bill HoffmanStatusnew => assigned
2009-12-14 08:59Bill HoffmanAssigned To => Brad King
2009-12-14 10:03Brad KingNote Added: 0018845
2009-12-14 11:09Michael WildNote Added: 0018851
2009-12-14 11:10Michael WildFile Added: 0001-FIX-Preserve-implicit-dependencies-and-collect-all-d-rewrapped.patch
2009-12-14 11:11Michael WildNote Edited: 0018851
2009-12-14 11:22Brad KingNote Added: 0018853
2009-12-14 11:29Brad KingNote Added: 0018854
2009-12-14 11:29Michael WildNote Added: 0018855
2009-12-14 14:40Michael WildFile Added: fix-IMPLICIT_DEPENDS-patches-v3.tgz
2009-12-14 14:41Michael WildNote Added: 0018870
2009-12-14 15:37Brad KingNote Added: 0018873
2009-12-14 15:38Brad KingAssigned ToBrad King => Alex Neundorf
2009-12-14 15:40Brad KingNote Added: 0018874
2009-12-14 15:57Alex NeundorfNote Added: 0018877
2009-12-14 16:02Brad KingNote Added: 0018878
2009-12-15 00:46Michael WildFile Added: fix-IMPLICIT_DEPENDS-patches-v4.tgz
2009-12-15 00:50Michael WildNote Added: 0018890
2010-05-23 09:15Michael WildNote Added: 0020815
2010-12-15 10:55Michael WildNote Added: 0024156
2010-12-15 11:21Brad KingNote Added: 0024160
2012-08-13 14:46Alex NeundorfRelationship addedrelated to 0012619
2012-08-13 14:46Alex NeundorfTarget Version => CMake 2.8.10
2012-09-29 04:54Alex NeundorfNote Added: 0031113
2012-09-30 12:41Alex NeundorfNote Added: 0031115
2012-10-18 14:14Alex NeundorfNote Added: 0031278
2012-10-18 14:14Alex NeundorfTarget VersionCMake 2.8.10 => CMake 2.8.11
2012-11-03 13:58Alex NeundorfNote Added: 0031400
2012-11-03 13:58Alex NeundorfStatusassigned => closed
2012-11-03 13:58Alex NeundorfResolutionopen => fixed

Notes
(0018845)
Brad King   
2009-12-14 10:03   
Thanks for the patch! I have a few comments.

Several of the hunks need line length adjustments. Our C and C++ sources/headers have a 78-column width style requirement.

- " [IMPLICIT_DEPENDS <lang1> depend1 ...]\n"
+ " [IMPLICIT_DEPENDS <lang1> depend1 [<lang2> depend2] ...]\n"

Pre-formatted documentation blocks (those beginning with spaces) should fit nicely in 80-column terminals. Please wrap the <lang2> part to its own line.

- "Dependencies discovered from the scanning are added to those of "
- "the custom command at build time. Note that the IMPLICIT_DEPENDS "
- "option is currently supported only for Makefile generators and "
- "will be ignored by other generators."
- "\n"
+ "The language has to be specified for every file listed in the "
+ "IMPLICIT_DEPENDS list. Dependencies discovered from the scanning are "
+ "added to those of the custom command at build time. Note that the "
+ "IMPLICIT_DEPENDS option is currently supported only for Makefile "
+ "generators and will be ignored by other generators.\n"

I've started putting each sentence in its own double-quoted block in these headers so that changes do not need to reformat whole paragraphs. Try just

+ "The language has to be specified for every file listed in the "
+ "IMPLICIT_DEPENDS list. "

in the proper place instead.

The hunks in Source/cmDepends.cxx look good. The test for the change looks good.

The main problem I see is the change to the cmDepends::WriteDependencies virtual function signature. All other subclasses should be updated to override it. Currently the patch does not update cmDependsFortran. Ideally the WriteDependencies method would be pure virtual, but at least one place creates an instance of cmDepends itself.
(0018851)
Michael Wild   
2009-12-14 11:09   
(edited on: 2009-12-14 11:11)
Attached a re-wrapped version which also fixes cmDepends{Java,Fortran}. The change to the cmDependsJava class is trivial, tested the implementation of cmDependsFortran::WriteDependencies locally. Honestly, I can only attribute to tiredness that I didn't see that cmDepends{Java,Fortran} implement this function ;-)

I did find an issue with the Fortran parser, though. It seems that it doesn't handle Fortran-include statements, such as

test.f90:
---------
include "hello.f90"
program test
  implicit none
  ! hello() is defined in hello.f90
  call hello()
end program test
(0018853)
Brad King   
2009-12-14 11:22   
Thanks, I'm testing the patch now.

The Fortran scanner is based on makedepf90. Your example works for me:

# depend.internal
CMakeFiles/t.dir/test.f90.o
 /path/to/FortranScan/test.f90
 /path/to/FortranScan/hello.f90
(0018854)
Brad King   
2009-12-14 11:29   
The patch looks good. Please split it into multiple pieces (like git folks do):

1.) Remove the dependency vector erasure:

      //DependencyVector tmp;
      //validDeps[this->Depender] = tmp;

The patch should just erase the lines. The commit log can contain the comment describing why they're not needed (the map operator [] will create an empty entry the first time anyway).

2.) Update the WriteDependencies signature. Justify it by describing the generalization to multiple sources (explicit dependencies) for one object.

3.) Extend/document/test IMPLICIT_DEPENDS

Thanks!
(0018855)
Michael Wild   
2009-12-14 11:29   
Of course, now that I try it again, it works *sigh*. Sorry for the noise.
(0018870)
Michael Wild   
2009-12-14 14:41   
Attached a tarball with the split patches. However, I switched the order of the first two patches, because the removal of the dependency vector erasure doesn't make sense without the other patch (unless you care for exponentially growing depend.{internal,make}).
(0018873)
Brad King   
2009-12-14 15:37   
Oh, I didn't realize the first two patches were tied. Go ahead and squash them together.

It looks like the hunks in "Source/cmLocalUnixMakefileGenerator3.*" should be in the last patch because they only affect IMPLICIT_DEPENDS.
(0018874)
Brad King   
2009-12-14 15:40   
Alex, I've assigned this to you because the interaction of the patches with your validDeps work is not clear to me. Please evaluate Michael's patches for correctness and to see how they affect your speed optimizations.

Michael, Alex is currently maintaining the cmDepends code.
(0018877)
Alex Neundorf   
2009-12-14 15:57   
That's a big patch. Can you make it smaller, mainly the part for cmDependsC.cxx ?
Does it keep all the caching of still intact dependency information working ?
I'll have a careful look at the patch, but not today.

Alex
(0018878)
Brad King   
2009-12-14 16:02   
It's not as big as it looks, there's just a lot of indentation change. Use a tool to view the patch without whitespace changes. I use "git diff -w".
(0018890)
Michael Wild   
2009-12-15 00:50   
Brad, you're right. Moved changes of Source/cmLocalUnixMakefileGenerator3.* to the last patch.

Alex: Yes, there are a lot of white-space changes, I'm sorry. But I had to move/add some loops to wrap existing code, so the indentation changed quite a lot. As Brad said, "git diff -w" or similar will do the trick. Thanks for taking care of this.
(0020815)
Michael Wild   
2010-05-23 09:15   
Topic branch rebased onto current master is here: http://github.com/themiwi/CMake/commits/patches/fixImplicitDepends [^]
(0024156)
Michael Wild   
2010-12-15 10:55   
What's the status on this issue?
(0024160)
Brad King   
2010-12-15 11:21   
I'd like Alex to review the patch since it touches his cmDepends code.

Use these commands to get a nice view of the patch:

 git remote add themiwi git://github.com/themiwi/CMake.git [^]
 git fetch themiwi
 git log -p -w master..themiwi/patches/fixImplicitDepends --color-words

The "--color-words" option shows a much cleaner (not line-wise, as in intra-line diffs) form of the patch with removed code in red and new code in green.
(0031113)
Alex Neundorf   
2012-09-29 04:54   
Michael,
can you create and attach a small testcase for this ?

Thanks
Alex
(0031115)
Alex Neundorf   
2012-09-30 12:41   
There is now a branch FixImplicitDepends2 on stage. It is based on your patches.
Please give it a try and let me know whether it works for you.
(0031278)
Alex Neundorf   
2012-10-18 14:14   
Too late for 2.8.10, this will go into 2.8.11.

Comments from Brad:

"It looks pretty good at a quick glance. It would be easier
to review if you split the commit

 cmDepends: allow multiple dependees per depender

into one that changes "const char* src" to std::set<> but
makes no functional change followed by another commit that
adds the new behavior.

Also, please write the commit messages to explain the change
without links to third-party sites like:
...

In order to credit the author of a patch that I've heavily changed
I usually add

Inspired-by: User Name <email>"
(0031400)
Alex Neundorf   
2012-11-03 13:58   
Merged the FixImplicitDepends2 branch into next, this should be in 2.8.11.