[vtk-developers] Improvements to the Python wrapping in VTK

[Marcus D. Hanwell]

On Tue, Mar 19, 2013 at 5:53 PM, Marcus D. Hanwell
<marcus.hanwell at kitware.com> wrote:
> On Tue, Mar 19, 2013 at 5:47 PM, David Gobbi <david.gobbi at gmail.com> wrote:
>> On Tue, Mar 19, 2013 at 3:18 PM, Marcus D. Hanwell
>> <marcus.hanwell at kitware.com> wrote:
>>> On Tue, Mar 19, 2013 at 5:03 PM, Marcus D. Hanwell
>>> <marcus.hanwell at kitware.com> wrote:
>>>> On Tue, Mar 19, 2013 at 4:30 PM, David Gobbi <david.gobbi at gmail.com> wrote:
>>>>> On Tue, Mar 19, 2013 at 1:58 PM, David Gobbi <david.gobbi at gmail.com> wrote:
>>>>>> On Tue, Mar 19, 2013 at 1:34 PM, Marcus D. Hanwell
>>>>>> <marcus.hanwell at kitware.com> wrote:
>>>>>>> On Tue, Mar 19, 2013 at 9:43 AM, David Gobbi <david.gobbi at gmail.com> wrote:
>>>>>>>> On Mon, Mar 18, 2013 at 6:08 PM, Marcus D. Hanwell
>>>>>>>> <marcus.hanwell at kitware.com> wrote:
>>>>>>>>> On Mon, Mar 11, 2013 at 1:51 PM, David Gobbi <david.gobbi at gmail.com> wrote:
>>>>>>>>>> On Mon, Mar 11, 2013 at 11:37 AM, Marcus D. Hanwell
>>>>>>>>>> <marcus.hanwell at kitware.com> wrote:
>>>>>>>>>>> On Mon, Mar 11, 2013 at 1:34 PM, David Gobbi <david.gobbi at gmail.com> wrote:
>>>>>>>>>>>> On Mon, Mar 11, 2013 at 11:01 AM, Marcus D. Hanwell
>>>>>>>>>>>> <marcus.hanwell at kitware.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> That sounds great - I will make that move (and agree on reducing
>>>>>>>>>>>>> moves). My final question if you look at the patch is on the right way
>>>>>>>>>>>>> to handle vtkType.h and friends - should they just be marked as
>>>>>>>>>>>>> WRAP_SPECIAL, should we do something different with WRAP_HEADER (which
>>>>>>>>>>>>> is currently going into the classes file).
>>>>>>>>>>>>
>>>>>>>>>>>> I want to export all python-wrappable files, including vtkType.h.  But
>>>>>>>>>>>> there is no vtkCommonCore-Headers.cmake, so I don't know where to put
>>>>>>>>>>>> vtkType.h except in vtkCommonCore-Classes.cmake, which is a poor fit.
>>>>>>>>>>>>
>>>>>>>>>>>> The main purpose of both WRAP_HEADER and WRAP_SPECIAL is
>>>>>>>>>>>> just to wrap files in python that don't get wrapped in tcl or java.  The
>>>>>>>>>>>> WRAP_HEADER is just for wrapping headers that aren't already in the
>>>>>>>>>>>> class list.  So of course I'd like for WRAP_HEADER headers to be
>>>>>>>>>>>> exported, but really I'd prefer to export all non-class headers
>>>>>>>>>>>> anyway.
>>>>>>>>>>>>
>>>>>>>>>>>> Since I never extended the "special" wrapping to the java and tcl
>>>>>>>>>>>> wrappers (and almost certainly won't) maybe a WRAP_PYTHON
>>>>>>>>>>>> should replace both WRAP_HEADER and WRAP_SPECIAL.
>>>>>>>>>>>>
>>>>>>>>>>> I feel like if WRAP_SPECIAL is simply ignored by the other wrapped
>>>>>>>>>>> languages keeping that name seems reasonable. If we want to extend
>>>>>>>>>>> either one (or another language in the future) then we can use
>>>>>>>>>>> WRAP_SPECIAL as an extended set of files that your wrapper must
>>>>>>>>>>> support and get rid of WRAP_HEADER entirely. It seems to me like they
>>>>>>>>>>> belong in the -Classes file, but perhaps that name could be a little
>>>>>>>>>>> more general (-Headers as we only really care about headers for
>>>>>>>>>>> wrapping?)
>>>>>>>>>>
>>>>>>>>>> I'd also prefer -Headers over -Classes.  A complete list of
>>>>>>>>>> every header file that is part of the install, along with a list
>>>>>>>>>> of properties.
>>>>>>>>>>
>>>>>>>>> I am just working on cleaning this topic up. I noticed that since
>>>>>>>>> moving the vtkWrappingPython module three Python tests are now
>>>>>>>>> failing,
>>>>>>>>>
>>>>>>>>> http://open.cdash.org/viewTest.php?onlyfailed&buildid=2842500
>>>>>>>>>
>>>>>>>>> It seems that there is some hardwired logic I am not aware of - any
>>>>>>>>> ideas? This is the test string, templates and variants so it all fits
>>>>>>>>> that the special code is being missed somehow.
>>>>>>>>
>>>>>>>> These failures happen because some classes appear in both the
>>>>>>>> SRCS and HDRS, and that confuses the vtkWrapHierarchy  tool.
>>>>>>>> Specifically, vtkTypedArray, vtkSparseArray, vtkDenseArray appear
>>>>>>>> as both .h and as .txx files.  The Hierarchy.txt files end up with two
>>>>>>>> entries for each, one marked as WRAP_EXLUDE;WRAP_SPECIAL
>>>>>>>> (which is correct) with a duplicate marked as just WRAP_EXCLUDE.
>>>>>>>
>>>>>>> I am just getting my head around this now, I guess this is because the
>>>>>>> txx need to be installed, but why not list the txx in the source (as
>>>>>>> they are closer to the source) and add some code to deal with txx
>>>>>>> files correctly? This doesn't seem to affect the -Headers file either,
>>>>>>> which only shows one occurrence of each of these classes. So if I fix
>>>>>>> the duplication then wrap hierarchy should be happy? It would be good
>>>>>>> to get all of the wrapping tools looking at the same information, but
>>>>>>> I am not sure I will have the time necessary to carry this through for
>>>>>>> everything.
>>>>>>
>>>>>> Yes, hierarchy will be happy if:
>>>>>> 1) the duplication is removed
>>>>>> 2) whichever file appears in SRCS also appears in the WRAP_SPECIAL list.
>>>>>>
>>>>>> So right now only vtkDenseArray.h appears in WRAP_SPECIAL which
>>>>>> is why the hierarchy listing that corresponded to vtkDenseArray.txx lacked
>>>>>> the WRAP_SPECIAL property and wasn't properly wrapped.
>>>>>>
>>>>>> Having the .txx in SRCS and the .h unlisted makes sense to me for these
>>>>>> classes.
>>>>>
>>>>> But I just thought of something.  I have some vague memories of some classes
>>>>> that had .h, .txx, and .cxx (all three).  I can't remember if it was VTK or ITK.
>>>>> Really this should never happen because VTK is supposed to have one class
>>>>> per header, and that class is either templated or it isn't.
>>>>>
>>>> I haven't come across that, and like you say it seems like it should
>>>> not be possible. I pushed an update which (locally at least) builds
>>>> with Tcl turned on and the templates test in Python is working.
>>>> Effectively I added some code to treat txx a little more like a source
>>>> file, with the caveat that we would install the txx (not the case with
>>>> cxx). I will wait on the dashboards, and build Java while I am
>>>> waiting.
>>>>
>>>> Feedback welcome, I think this is getting pretty close and would
>>>> appreciate any further feedback you have on the topic.
>>>>
>>> As far as I can tell, on Linux at least, this is fine with all wrapped
>>> languages (Python, Tcl, Java) and instantiators turned on. To be
>>> exported you need to place the .cxx, .txx, .mm or .h in the list of
>>> sources for the module, we will then attempt to find the matching
>>> header file. It might be cleaner to directly list all public headers
>>> that are candidates for wrappnig/part of our public API but that would
>>> also take quite a bit of time. I feel like all of the properties
>>> should be on the headers too, but this again would take some time and
>>> only becomes reliable if all of the wrapping tools are consulting the
>>> same source of information.
>>
>> If the .cxx/.txx is already listed it would be redundant to have the
>> corresponding .h listed, too.  I have mixed feelings about applying
>> the properties to the headers.  On the "pro" side, the header is the
>> interface and the property reflects the interface more than anything
>> else.  On the "con" side, listing the .cxx in SRCS but then applying
>> properties to the .h files seems confusing.
>>
> Yeah, I was more thinking out loud, but was it is in the topic is all
> I intended to do in the short term (unless you spot problems with it).
> I think the code to make the transformation from source to header is
> fairly robust, and I see the tests all passed on the dashboard
> machines too (along with Tcl, Java and instantiators locally on
> Linux).
>
I added what will hopefully be the final commit in that topic, it
compiles and works on a fresh build tree for Tcl, Java and Python
(Linux) and we will wait on the CDash at Home builds. I need to head out
early today, but please let me know if you spot any problems and I
will respond later/tomorrow. The main porting points for ParaView (or
other applications using our wrapping) are vtkWrappingPython is now
vtkWrappingPythonCore and the vtk_module_classes_load is now
vtk_module_headers_load with the variables using HEADER/HEADERS rather
than CLASS/CLASSES.

Thanks for the feedback, I think this topic is in a pretty good state now.

Marcus