View Issue Details Jump to Notes ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0012307CMakeModulespublic2011-06-26 07:492016-01-04 11:51
ReporterModestas Vainius 
Assigned To 
PrioritynormalSeveritymajorReproducibilityalways
StatusclosedResolutionfixed 
Platformamd64OSDebian GNU/LinuxOS Versionsid
Product Version 
Target VersionFixed in Version 
Summary0012307: regression in 2.8.5 rc2: UseSWIG.cmake broken
DescriptionThere are multiple reports (on Fedora and Debian) that UseSWIG no longer works in 2.8.5 rc2 while it works fine in 2.8.4. For information, see:

http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=631497 [^]
http://thread.gmane.org/gmane.comp.misc.ptx/27854/focus=27903 [^]

This SWIG stuff seems to be pretty ugly (nor I know anything about it) so I have not tried to reproduce the failure myself. However, I'm pretty sure that the following commit is responsible for this since the failure is in the new code added by it.

commit 1088b0278e74526298d0821589973918da33c44b
Author: Mathieu Malaterre <mathieu.malaterre@gmail.com>
Date: Mon Mar 28 18:14:23 2011 +0200

    Add a new function SWIG_GET_WRAPPER_DEPENDENCIES to UseSWIG.cmake
    
    This commit fixes BUG: 0004147 it directly uses swig executable
    to compute a list of dependencies directly from the .i files
    to make sure to rebuild the swig module any of its direct dep.
    is touched
TagsNo tags attached.
Attached Files

 Relationships
related to 0004147closedjschueller [MODULES][UseSWIG] Use swig to compute dependencies 

  Notes
(0026972)
David Cole (manager)
2011-06-26 08:23

Mathieu, can you take a look at this...?
(0026973)
Modestas Vainius (reporter)
2011-06-26 08:26

Could you tell me which commit?
(0026974)
Modestas Vainius (reporter)
2011-06-26 08:32
edited on: 2011-06-26 08:32

Btw, if you have in mind 03809b14429edb14ffbb4e83d2cda32e86555de1 then it does not help according to http://news.gmane.org/find-root.php?message_id=%3c20110624222446.GI4448%40postle.net%3e [^]

(0026975)
David Cole (manager)
2011-06-26 08:37

These are the commits in 'master' related to swig:

  http://cmake.org/gitweb?p=cmake.git&a=search&h=HEAD&st=commit&s=swig [^]

03809b14 was the commit that I was hoping fixed this problem when I added that first note to this bug... but then I dug in and read the description, realized it's probably not fixed yet, and then sent the note to Mathieu instead...
(0026977)
David Cole (manager)
2011-06-26 08:40

Looks like this is caused by the fix for 0004147
(0026986)
Mathieu Malaterre (developer)
2011-06-27 02:22
edited on: 2011-06-27 02:37

Modestas, the only reason why you would see an issue with cmake 2.8.5 is something like this:

SWIG_ADD_MODULE(bla python bla.i)

where bla.i contains:


%{
#include "configure.h"
%}

*and* configure.h is configured by cmake *after* the cmake command SWIG_ADD_MODULE

----

In summary you have to do:

configure_file(configure.h.in configure.h)
SWIG_ADD_MODULE(bla python bla.i)

in that order now to let cmake compute dependencies correctly.

A quick hack would be to run cmake twice...

I do not believe this is a regression, this simply require all headers for swig module to be present now.

(0026987)
Modestas Vainius (reporter)
2011-06-27 04:22

For some reason you assume that those headers must present at cmake time. But that's not necessarily true. In this particular case, src/hugin_script_interface/CMakeLists.txt generates them with custom command:

...
preprocess("${HUGIN_BASE_DIR}/panodata/PanoramaData.h"
           "${CMAKE_CURRENT_BINARY_DIR}/hsi_PanoramaData.h"
           "${HUGIN_BASE_DIR}/panodata/image_variables.h"
)
...
SET_SOURCE_FILES_PROPERTIES(hsi.i PROPERTIES CPLUSPLUS ON)
SWIG_ADD_MODULE(hsi python hsi.i)
SWIG_LINK_LIBRARIES(hsi ${PYTHON_LIBRARIES} ${common_libs})
...
MACRO(preprocess in out depends)
ADD_CUSTOM_COMMAND(OUTPUT ${out}
    COMMAND ${HSI_C_PREPROCESS} ${in} > ${out}
    DEPENDS ${in} ${depends}
)
SET_SOURCE_FILES_PROPERTIES(${out} GENERATED)
ENDMACRO(preprocess)

...

>I do not believe this is a regression, this simply require all headers for swig module to be present now.

In my opinion, hugin use case is a perfect one which cmake 2.8.5 rc2 broke for no good reason. While here that custom command could be replaced by execute_process() but what if custom command was actually built from source. In my opinion, the easiest solution would be to make dependency scanning optional and OFF by default. At very least, backwards compatibility was broken in undocumented way so you need to fix that either way.
(0026988)
Mathieu Malaterre (developer)
2011-06-27 04:42

The benefit of proper dependencies is HUGE for anyone doing work with SWIG. Having a report of missing files is simply fantastic.

I would suggest you fill a bug report to hugin projet to fix there source code instead of relying on broken behavior. If a dependency is needed for a particular swig module, you need to depend on it...
(0026989)
Modestas Vainius (reporter)
2011-06-27 05:33

hugin is NOT broken and it's NOT relying on broken behaviour. It's new SWIG code that relies on broken assumption that all files must be available at cmake time.
The whole purpose of cmake is to generate scripts for building the source, not to actually build it. cmake is a mere source configuration tool. The real problem is that new SWIG code does not support nor take into account generated files (targets) while cmake itself goes through a great pain to do this.

With that said, I'm not saying you should throw away your code. For example, the easiest way would be to add SCANDEPENDS (or the like) option to SWIG_ADD_MODULE(). Another solution would be to make swig dependency scanner ignore missing include files errors (dunno if that's possible). However, IMHO the best (and the most complicated) solution would be to rewrite dependency scanner to do its work at build (make) time like cmake itself does ('Scanning dependencies of target' stage, depend.make files). Once again, I don't know how easy the latter is to do.

Don't get me wrong. I don't really particularly care about SWIG nor hugin. However, it's sad to see cmake losing flexibility it had before. It is wrong to add not-so-proper implementation as mandatory rather than optional even if it would probably work in most cases despite its flaws. At least make it disabable.
(0026992)
Brad King (manager)
2011-06-27 13:07

I've reverted the fix for issue 0004147:

  http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=2377d7a2 [^]

to avoid the regression in the 2.8.5 release. See 0004147:0026991.
(0026993)
Brad King (manager)
2011-06-27 13:40

New revert commit with better message:

 http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=fc045318 [^]
(0027007)
Modestas Vainius (reporter)
2011-07-04 13:14

Too bad this fix didn't make to 2.8.5~rc3. When will 2.8.5 final be released?
(0038990)
Brad King (manager)
2015-06-26 11:08

This regression issue was resolved by the commit linked in 0012307:0026993. Issue 0004147 trackes the actual feature request separately.
(0040084)
Robert Maynard (manager)
2016-01-04 11:51

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

 Issue History
Date Modified Username Field Change
2011-06-26 07:49 Modestas Vainius New Issue
2011-06-26 08:21 David Cole Assigned To => David Cole
2011-06-26 08:21 David Cole Status new => assigned
2011-06-26 08:22 David Cole Note Added: 0026971
2011-06-26 08:22 David Cole Note Deleted: 0026971
2011-06-26 08:22 David Cole Assigned To David Cole => Mathieu Malaterre
2011-06-26 08:23 David Cole Note Added: 0026972
2011-06-26 08:26 Modestas Vainius Note Added: 0026973
2011-06-26 08:32 Modestas Vainius Note Added: 0026974
2011-06-26 08:32 Modestas Vainius Note Edited: 0026974
2011-06-26 08:37 David Cole Note Added: 0026975
2011-06-26 08:38 David Cole Relationship added related to 0004147
2011-06-26 08:40 David Cole Note Added: 0026977
2011-06-27 02:22 Mathieu Malaterre Note Added: 0026986
2011-06-27 02:37 Mathieu Malaterre Note Edited: 0026986
2011-06-27 04:22 Modestas Vainius Note Added: 0026987
2011-06-27 04:42 Mathieu Malaterre Note Added: 0026988
2011-06-27 05:33 Modestas Vainius Note Added: 0026989
2011-06-27 13:07 Brad King Note Added: 0026992
2011-06-27 13:40 Brad King Note Added: 0026993
2011-07-04 13:14 Modestas Vainius Note Added: 0027007
2015-06-26 11:08 Brad King Note Added: 0038990
2015-06-26 11:08 Brad King Assigned To Mathieu Malaterre =>
2015-06-26 11:08 Brad King Status assigned => resolved
2015-06-26 11:08 Brad King Resolution open => fixed
2016-01-04 11:51 Robert Maynard Note Added: 0040084
2016-01-04 11:51 Robert Maynard Status resolved => closed


Copyright © 2000 - 2018 MantisBT Team