(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.

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