search

cellml / libcellml

Repository for libCellML development.
https://libcellml.org
Apache License 2.0
16 stars 21 forks source link

SWIG 4.1.0 has changed the way static methods are wrapped #1062

Closed hsorby closed 1 year ago

hsorby commented 1 year ago

The gihub actions containers we depend on have updated to SWIG 4.1.0. Ubuntu, one of our primary target platforms does not yet have a standard package for SWIG 4.1.0.

In SWIG 4.0.2 static methods of a class had an extra function created as an alternative?? for the class static method. SWIG 4.1.0 has removed these extra functions. This is a problem for us as we are requiring that we have 100% coverage for the Python API. Now that the Python API is different depending on which version of SWIG has wrapped the library, we will also need to have an adaptive test based on the version of SWIG the library was wrapped with.

The alternative is to require SWIG 4.1.0 as the minimum version of SWIG.

agarny commented 1 year ago

I would require SWIG 4.1.0 as the minimum version of SWIG. Simple as that.

nickerso commented 1 year ago

I'm probably missing something, but #1063 is not making tests adaptable to SWIG version but updating the tests to cope with libCellML having a different Python API. Which in this case is caused by using a different version of SWIG to generate the bindings. This seems, to me, to be large problem, not simply dealing with coverage. My Python scripts would now not depend on a particular version of the bindings but on the version of SWIG used to build them...so potentially I'd have a broken environment even though my dependencies are met, right? i.e., I'd presumably need a different requirements.txt depending on the platform rather than having a single one that works across all supported platforms...in which case there should definitely be some documentation updates added to #1063.

Ideally a given version of the libCellML bindings would have a consistent API across all supported platforms. If there are reasons for why we need different versions of SWIG on different platforms that will result in different APIs, then I'd be more inclined to see if we can mangle the generated bindings to ensure consistency. If this difference introduced by SWIG 4.1.0 vs 4.0.2 is internal API that is never going to be used outside the coverage tests, then I'd be very tempted to simply remove it from the generated bindings regardless of the version of SWIG used.

hsorby commented 1 year ago

There is a flag on SWIG version 4.1.0 -flatstaticmethod which adds back in the flattened static methods. For me the best solution going forward is to use this for SWIG version 4.1.0 and maintain the current Python bindings API. When a package for SWIG 4.1.0 becomes available for Ubuntu we can look at setting the minimum SWIG version.

We should probably also state the minimum platforms that we are targeting somewhere.

nickerso commented 1 year ago

That seems like a great solution to me.

agarny commented 1 year ago

There is a flag on SWIG version 4.1.0 -flatstaticmethod which adds back in the flattened static methods. For me the best solution going forward is to use this for SWIG version 4.1.0 and maintain the current Python bindings API. When a package for SWIG 4.1.0 becomes available for Ubuntu we can look at setting the minimum SWIG version.

We should probably also state the minimum platforms that we are targeting somewhere.

Definitely happy with that.