Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SWIG 4.1.x, 4.2.x & 4.3x #956

Open
Tracked by #1246
ptheywood opened this issue Nov 1, 2022 · 5 comments
Open
Tracked by #1246

SWIG 4.1.x, 4.2.x & 4.3x #956

ptheywood opened this issue Nov 1, 2022 · 5 comments
Labels

Comments

@ptheywood
Copy link
Member

ptheywood commented Nov 1, 2022

@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 ptheywood added the SWIG label Nov 1, 2022
@ptheywood
Copy link
Member Author

ptheywood commented Apr 22, 2024

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 swig/swig@c7ab6a0.

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 ptheywood changed the title SWIG 4.1.0 SWIG 4.1.0+ Apr 22, 2024
@ptheywood ptheywood changed the title SWIG 4.1.0+ SWIG 4.1.x Apr 22, 2024
@ptheywood ptheywood changed the title SWIG 4.1.x SWIG 4.1.x and 4.2.x Apr 22, 2024
@Robadob
Copy link
Member

Robadob commented Oct 2, 2024

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.

swig/swig#2954

@Robadob
Copy link
Member

Robadob commented Oct 27, 2024

Have reported the issue to SWIG: swig/swig#3060

@ptheywood ptheywood changed the title SWIG 4.1.x and 4.2.x SWIG 4.1.x, 4.2.x & 4.3x Oct 28, 2024
@ptheywood
Copy link
Member Author

ptheywood commented Oct 28, 2024

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 [Test Failure] Swig RunPlan != can't compare with None? #1233 (comment)

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
Copy link
Member Author

Swig 4.1 and 4.2 tasks now resolved.

Swig 4.3 appears to work, but should be more thoroughly checked along with if it is required for python 3.13 support or not as part of #1236

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants