SofaDefrost / Cosserat

A SOFA plugin to simulate linear structures using Cosserat theory
GNU Lesser General Public License v3.0
46 stars 22 forks source link

Fix SofaPython3 fetching #99

Closed alxbilger closed 6 months ago

alxbilger commented 8 months ago

Hey @adagolodjo, why is this PR closed?

adagolodjo commented 8 months ago

Oops, I didn't do it on purpose.

alxbilger commented 8 months ago

@adagolodjo It seems the PR works, except for MacOS. But I heard SofaPython3 does not compile with Python3.12. And 3.12 seems to be the version used on the MacOS CI (I don't know why). FYI @bakpaul, @olivier-roussel, @fredroy

olivier-roussel commented 8 months ago

Indeed, pybind11 installed in the macOS CI is using python3.8 ( see pybind11 install: https://github.com/SofaDefrost/Cosserat/actions/runs/7759007374/job/21162087802?pr=100#step:3:1416 ) . But python 3.12 is also installed, and Sofa Cosserat seems to use it. So we basically mix here two python versions which sounds not so good. But clearly the compilation error is due to the use of Python 3.12 C API which has apparently changed for PyFrameObject (https://docs.python.org/3/c-api/frame.html). For information, for the conda packages the python version has to be specified in the CMake of SofaPython3 plugin and Sofa Cosserat one through the Python_EXECUTABLE cmake option, to ensure the same python version is used along all packages, as they are built separately. But the environment virtualisation of conda make it very easy to specify this variable without any ambiguity.

olivier-roussel commented 8 months ago

The python version installed by the macOS CI that should probably have been used instead of python 3.12 : https://github.com/SofaDefrost/Cosserat/actions/runs/7817011040/job/21323895202?pr=99#step:3:360.

alxbilger commented 8 months ago

@olivier-roussel thanks for the information. Do you know why Cosserat uses 3.12 and not 3.8?

olivier-roussel commented 8 months ago

You may have missed my previous comment. Cosserat might require some cmake variables such as Python_EXECUTABLE to find the same version that is set for SofaPython3, but could not say exactly how without having time to reproduce locally.

olivier-roussel commented 8 months ago

Any reason to suppress the -DPython_EXECUTABLE=$PYTHON_EXE" from the cmake call ? I would have thought it would link to the python used in SofaPython3

alxbilger commented 8 months ago

@olivier-roussel I applied stupidly what was done in SoftRobots: https://github.com/SofaDefrost/SoftRobots/pull/250/files 🥲

olivier-roussel commented 8 months ago

Looks like https://github.com/sofa-framework/sofa/pull/4471 broke the CI of all plugins.

alxbilger commented 6 months ago

@bakpaul @olivier-roussel Do you know how to fix this issue?

CMake Error at /usr/local/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find Python: Found unsuitable version "3.10.13", but required is
  exact version "3.10.12" (found
  /opt/hostedtoolcache/Python/3.10.13/x64/python, found components:
  Interpreter Development Development.Module Development.Embed)
bakpaul commented 6 months ago

I know where this is coming from. We have forced to find the same python version as the one use for compilation, in the CmakeConfig of SofaPython3. But I didn't think that the full version (with patch) would be used to generate this. So that makes patches incompatible. We need to fix this in the Config.cmake.in of SofaPython3 to generate the configuration file only with the major.minor. I'll do it when I have time...

alxbilger commented 6 months ago

Thanks @bakpaul

bakpaul commented 6 months ago

See https://github.com/sofa-framework/SofaPython3/pull/404

olivier-roussel commented 6 months ago

So for setting up the CI you ensure you force installing python version only by MAJOR.MINOR right ? (i.e. you do not care about python version PATCH consistency ?)

bakpaul commented 6 months ago

Yes, and normally patches are compatibke

alxbilger commented 6 months ago

@olivier-roussel @bakpaul @adagolodjo the CI is finally happy. Can anyone approve and merge (with squashing)?