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

Fix SofaPython3 fetching #99

Merged
merged 5 commits into from
Mar 14, 2024

Conversation

alxbilger
Copy link
Member

No description provided.

@adagolodjo adagolodjo closed this Jan 26, 2024
@alxbilger
Copy link
Member Author

Hey @adagolodjo, why is this PR closed?

@adagolodjo
Copy link
Collaborator

Oops, I didn't do it on purpose.

@alxbilger
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Contributor

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

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

@olivier-roussel
Copy link
Contributor

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
Copy link
Contributor

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

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

@olivier-roussel
Copy link
Contributor

Looks like sofa-framework/sofa#4471 broke the CI of all plugins.

@alxbilger
Copy link
Member Author

@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
Copy link
Collaborator

bakpaul commented Mar 12, 2024

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

Thanks @bakpaul

@bakpaul
Copy link
Collaborator

bakpaul commented Mar 12, 2024

See sofa-framework/SofaPython3#404

@olivier-roussel
Copy link
Contributor

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
Copy link
Collaborator

bakpaul commented Mar 12, 2024

Yes, and normally patches are compatibke

@alxbilger
Copy link
Member Author

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

@olivier-roussel olivier-roussel merged commit 19d7f57 into SofaDefrost:master Mar 14, 2024
6 of 10 checks passed
bakpaul pushed a commit that referenced this pull request Mar 26, 2024
* Fix SofaPython3 fetching

* Restore Cmake variables related to Python

* Update ci

* set python version to 3.10

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

Successfully merging this pull request may close these issues.

4 participants