FLAMEGPU2 icon indicating copy to clipboard operation
FLAMEGPU2 copied to clipboard

SWIG 4.1.x and 4.2.x

Open ptheywood opened this issue 2 years ago • 2 comments

@Robadob found that manylinux builds were failing, this appears to be due to a new swig release 4.1.0 8 days ago which has since been incorporated into the manylinux containers used for building (2 days ago).

Currently we require swig 4.0.2 or greater (due to required bugfixes), and 4.1.0 is the first release after this.

We should probably allow either, i.e. keep the current CMake Logic though we have a few potential options too:

  • Update regular CI to use 4.1.0 too?
  • Keep 4.0.2 as the minimum, but fetch 4.1.0 if swig is not found?
  • Or strictly require 4.0.2 and not allow more recent versions

Edit

More recently, swig 4.2.x has been released, which is again causing problems. See comments below.

ptheywood avatar Nov 01 '22 12:11 ptheywood

Swig has had several more releases, including 4.2.1. Currently investigating if this is a cause of errors which do not occur in wheels built from 2024-01-12.

AttributeError: 'MessageSpatial3D_Description' object has no attribute 'newVariableID'. Did you mean: 'newVariableInt'?

A local build using 4.2.1 reproduced the error (and built much quicker than older swigs).

4.2.0 errors during pyflamepgu compilation, as an assertion is encountered within swig. This appears to be related to friend statements, and was fixed in 4.2.1 by https://github.com/swig/swig/commit/c7ab6a01c6582b92db9328e2f3daa67081f05f6e.

4.2.1 does appear to cause errors, so we need to either:

  1. Fix our .i file when swig >= 4.2.0 is used.
  2. Prevent Swig >= 4.2.0 from being used

Given 4.2.0 is known to cause problems, we should probably explicitly block that one regardless? Or atleast add an cmake warning that it'll probably not work (incase someone is using 4.2.0 but with the above patch applied).

ptheywood avatar Apr 22 '24 11:04 ptheywood

Ok I had a very quick play with this.

Changed

%define TEMPLATE_VARIABLE_INSTANTIATE_ID(function, classfunction)
TEMPLATE_VARIABLE_INSTANTIATE(function, classfunction) 
%template(function ## ID) classfunction<flamegpu::id_t>;
%enddef

// Array version, passing default 2nd template arg 0
%define TEMPLATE_VARIABLE_ARRAY_INSTANTIATE_ID(function, classfunction)
TEMPLATE_VARIABLE_ARRAY_INSTANTIATE(function, classfunction) 
%template(function ## ID) classfunction<flamegpu::id_t, 0>;
%enddef

to

%define TEMPLATE_VARIABLE_INSTANTIATE_ID(function, classfunction)
%template(function ## ID) classfunction<flamegpu::id_t>; // swapped this
TEMPLATE_VARIABLE_INSTANTIATE(function, classfunction)  // with this
%enddef

// Array version, passing default 2nd template arg 0
%define TEMPLATE_VARIABLE_ARRAY_INSTANTIATE_ID(function, classfunction)
%template(function ## ID) classfunction<flamegpu::id_t, 0>;
TEMPLATE_VARIABLE_ARRAY_INSTANTIATE(function, classfunction) 
%enddef

And it seems to have now generated the ID methods that weren't there before.

Possibly related to this issue, but it doesn't appear any other methods have gone missing, though according to the reply it should warn us if things are being ignored.

https://github.com/swig/swig/issues/2954

Robadob avatar Oct 02 '24 18:10 Robadob

Have reported the issue to SWIG: https://github.com/swig/swig/issues/3060

Robadob avatar Oct 27 '24 17:10 Robadob

Swig 4.3.0 adds python 3.13 support, so we'll probably want to switch to that for CI atleast and make sure that 4.3.0 works as intended (it appears to, and fixes #1233).

In which case I'm inclined for us to:

  • Keep swig 4.0.2 as our minimimum required
  • Add a CMake Warning if 4.2.0 is found - stating that it is a bit broken? Maybe even an error.
  • Add a CMake warning if 4.2.1 about the == None issue?
    • Replace the 3 occurences of != None with is not None to let 4.2.1 test suite pass, but still prefer to not have people use 4.2.1 / warn them about comparing to none with ==/!=?
  • (maybe?) Update our swig download if not required version to 4.3.0?
  • (maybe?) Update Windows/ubuntu CI builds to use 4.3.0 when we add 3.13 to the build matrices?
  • (maybe?) Configure pytest to not emit deprecatation notices, which Swig 4.3.0 causes to appear (but marked for fixing in 4.4.0). See https://github.com/FLAMEGPU/FLAMEGPU2/issues/1233#issuecomment-2440166825

Currently, manylinux still contains swig 4.2.1, but it will likely be updated automatically by an every friday CI workflow at some point soon (whenever 4.3.0 makes it onto pypi, the latest version is 4.2.1.post0, uploaded on 2024-10-23, which is after the 4.3.0 release date. The pypi package doens't seem to be maintained by the (main) swig devs).

I would have suggested we add a Swig CI workflow which checks different versions of swig behave as intended, but as we don't actaully run any tests in CI due to the absence of GPUs it wouldn't have actually spotted most of these issues, so not worth it.

ptheywood avatar Oct 28 '24 11:10 ptheywood