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

Adds a conda based CI workflow #53

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

olivier-roussel
Copy link
Contributor

@olivier-roussel olivier-roussel commented Jan 16, 2025

Adds a workflow for an additionnal CI based on a conda setup. This would enable an easy installation of dependencies required in other PR (such as proxsuite).

Enabled runners:

  • ubuntu 22.04 (linux x64)
  • macOS 13 (osx x64)
  • macOS 14 (osx arm64)
  • windows 2022 (win x64), using VS 2022 (MSVC 17) compiler

This should be similar platforms as currently used in our CI, ecxepted there is an additionnal macOS 14 runner for osx arm64 support.
I limited other build to latest python version (3.13) and Release build config only, but this can be discussed and adapted.

I had a strange bug due to missing library for linking stage on windows only with Sofa.SimpleApi.lib. I also already had it locally on ubuntu as well. I tried to fix it with a9231a4, but might not be the right way to do it.

@olivier-roussel olivier-roussel marked this pull request as draft January 16, 2025 08:58
@EulalieCoevoet
Copy link
Member

Thanks Olivier! It looks great to me.
I don't know about Sofa.SimpleApi.lib, maybe @alxbilger will.

@alxbilger
Copy link
Member

@olivier-roussel It seems that only the bindings needs simpleapi, so I would move your changes from a9231a4 in the CMakeLists.txt of the bindings

@olivier-roussel olivier-roussel force-pushed the conda-ci branch 2 times, most recently from 14081bc to 3d26e05 Compare January 16, 2025 14:29
@olivier-roussel
Copy link
Contributor Author

@alxbilger It seems that the bug comes from one of SoftRobots.Inverse dependency that add Sofa.SimpleApi incorrectly.
Without any CMakeLists.txt modifications, the linker exit on error on the bindings (see here ) with the error:
5>LINK : fatal error LNK1181: cannot open input file 'Sofa.SimpleApi.lib'
Checking at linker call command line just above shows that it tries to link with Sofa.SimpleApi.lib (without absolute path), but there should be a full absolute path like other Sofa libs linked (so we should have something like C:\Users\runneradmin\miniconda3\envs\sofa\Library\lib\Sofa.SimpleApi.lib instead.
If we had the dependency in the CMakeLists.txt of the bindings, both links to Sofa.SimpleApi.lib appears in the linker command line: one with full absolute path and still the previous one with relative path to current dir, see here. So the linker still fails due to the later one.
If we had the dependency in the CMakeLists.txt as I did initially, for some reason it works fine, but I don't know why.
Maybe one of the global SoftRobots.Inverse dependency is the faulty one... Any ideas ?

@alxbilger
Copy link
Member

I suspect the problem comes from SofaPython3. This PR added the dependency to simpleapi in the CMakeLists.txt, but not in bindings/Sofa/Bindings.SofaConfig.cmake.in. If I understand correctly, this is required for the libs that use the SofaPython3 bindings as a dependency. Does it make sense? @bakpaul @fredroy What is your opinion?

@olivier-roussel
Copy link
Contributor Author

Indeed, this looks like to be the cause. I will try with a fixed version of SofaPython3.

@bakpaul
Copy link
Collaborator

bakpaul commented Jan 16, 2025

Yes what you are saying is correct Alex. I think this is the issue. Olivier and myself stumbled upon a lot of issue of such during the v23.12 release.

@olivier-roussel
Copy link
Contributor Author

The conda CI using a nightly SofaPython3 integrating the fix you suggested (sofa-framework/SofaPython3#479) successfully passed. Thanks !

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