Basic setup for Python interface using SWIG#216
Conversation
Signed-off-by: Marcos Wagner <wagnermarcos@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
|
Some of the changes I've pushed:
|
Codecov Report
@@ Coverage Diff @@
## ign-math6 #216 +/- ##
==========================================
Coverage 99.21% 99.21%
==========================================
Files 65 65
Lines 6089 6089
==========================================
Hits 6041 6041
Misses 48 48 Continue to review full report at Codecov.
|
Signed-off-by: Marcos Wagner <wagnermarcos@ekumenlabs.com>
bca2353 to
4d09fe6
Compare
…on test files to some of the classes that had an associated .i file. Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
Signed-off-by: Marcos Wagner <wagnermarcos@ekumenlabs.com>
|
The interface has been implemented for Python using SWIG for the files that already had Ruby implementation. At the moment we have pushed tests for these files, to match the Ruby implementation. Something to note is that when running |
francocipollone
left a comment
There was a problem hiding this comment.
Looks good, I added some comments.
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
francocipollone
left a comment
There was a problem hiding this comment.
LGTM, I left some minor comments.
|
I've just left some minor comments however It is in a good shape to be reviewed @chapulina. |
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
Signed-off-by: Marcos Wagner <wagnermarcos@ekumenlabs.com>
ca8aebb to
e8a7e6c
Compare
|
This PR should fix ubuntu CI gazebo-tooling/release-tools#496 |
|
I think the following will fix the ABI checker: gazebo-tooling/release-tools#497 |
|
|
||
| set(CMAKE_SWIG_OUTDIR "${CMAKE_BINARY_DIR}/lib/python") | ||
| if(CMAKE_VERSION VERSION_GREATER 3.8.0) | ||
| SWIG_ADD_LIBRARY(${SWIG_PY_LIB} LANGUAGE python SOURCES python/python.i ${sources}) |
There was a problem hiding this comment.
We are recompiling sources instead of just linking, for both python and ruby bindings.
There was a problem hiding this comment.
It fails now, it's like now it can't find libignition-math6.so but I couldn't replicate this locally, locally it works. Will check
There was a problem hiding this comment.
I can reproduce the problem on my machine. I'm looking into it
scpeters
left a comment
There was a problem hiding this comment.
the test failure in the GitHub actions occur because make test is run without make install. We created the FAKE_INSTALL target in the test folder to support testing without make install. I've updated the python tests to use FAKE_INSTALL_PREFIX in 3e9109a. It required moving the definition of the FAKE_INSTALL_PREFIX variable to the root CMakeLists.txt in order to be accessed by both the src and test folders, which seems acceptable to me
| import ignition.math | ||
|
|
||
|
|
||
| class TestVector3(unittest.TestCase): |
There was a problem hiding this comment.
nit: rename TestVector3 since this is a more general test
|
looks like the only remaining CI issue is that 9e2dafc needs to be signed @francocipollone |
Co-authored-by: Louise Poubel <louise@openrobotics.org> Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Move the variable definition to the root CMakeLists.txt Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Point to fake install lib folder. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
2192b29 to
3fca80f
Compare
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
I see, thanks @scpeters for both the patch and the explanation.
Fixed! |
| a1 = ignition.math.Angle(1.5707) | ||
| a2 = ignition.math.Angle(0.7854) | ||
| print("a1 = {} radians, {} degrees\n".format(a1.Radian(), a1.Degree())) | ||
| print("a2 = {} radians, {} degrees\n".format(a2.Radian(), a2.Degree())) |
There was a problem hiding this comment.
nit for the future (not this pull request): I find f"" strings are more readable than using .format()
for example:
print(f"a2 = {a2.Radian()} radians, {a2.Degree(} degrees\n")
|
I just pulled the latest
Full log
I'm not sure yet how to fix my workspace. Looking into it Update Solved for now with |
what OS and swig version do you have? we should consider reverting this if others can reproduce the problem |
This happened on Bionic, |
|
I couldn't reproduce the error in
So removing swig and installing it again did the trick? That's interesting. |
Not exactly 😅 Removing SWIG just allowed me to skip that entire block of code, so I didn't run into the problem and could compile the core library. Reinstalling SWIG brings back the issue to me. My problem is the CMake version, trying a few things here: #223 |
Signed-off-by: Marcos Wagner wagnermarcos@ekumenlabs.com
🎉 New feature
Summary
Basic setup for a Python interface using SWIG to address #210 . (Related to #101) This creates a python module with the cpp classes with an associated .i file. To achieve dual interface output with Python and Ruby, the ruby.i and the python.i used at compilation had to be moved into separate subdirectories, to avoid collisions between the auxiliary files created by SWIG for Python and Ruby.
Test it
To test this interface
export PYTHONPATH=/ws/install/lib/pythonpython3.import ignition.math.Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge