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

Added compatibility with SWIG 2.0 #476

Merged

Conversation

francesco-romano
Copy link
Collaborator

Removed Span from the bindings if SWIG < 3.0

I marked the PR as WIP as we still have to decide what to do with some deprecated headers in idyntree-modelio-urdf.

@traversaro should we remove from the bindings URDFGenericSensorsImport.h ?

@francesco-romano
Copy link
Collaborator Author

I didn't updated the tests. I will do as soon as I have some time.

@traversaro
Copy link
Member

@traversaro should we remove from the bindings URDFGenericSensorsImport.h ?

I think not installing it was my error. Let's install it, and remove the bindings in the next iDynTree feature release (that may be a major version, if we want to follow SemVer).

Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, but with this modifications the Span is visible just when SWIG_VERSION is defined, that is not the case for normal builds.
I guess we should modify all the #if conditions to have the following form:

#if !defined(SWIG_VERSION) || SWIG_VERSION >= 0x030000

@traversaro
Copy link
Member

If you are interested in having SWIG 2.0 compatibility in future feature releases, I think it is imperative to add a SWIG 2.0 to Travis, otherwise we will for sure break it again. Fortunately there is still a swig2.0 package in Xenial ( https://repology.org/metapackage/swig/versions ), so this should not be too hard.

Note: SWIG 2.0 is the reason why we cannot use directly the [[deprecated]] attributes!

@francesco-romano
Copy link
Collaborator Author

Added the missing headers and fixed the swig 2 define.

I did not touch travis though.

@traversaro
Copy link
Member

No problem, that is more a concern for future releases, let's discuss f2f (more and less) and then I can handle that.

@traversaro traversaro changed the title [WIP]Added compatibility with SWIG 2.0 Added compatibility with SWIG 2.0 Sep 9, 2018
CMakeLists.txt Outdated
@@ -25,7 +25,7 @@ set(VARS_PREFIX "iDynTree")

set(${VARS_PREFIX}_MAJOR_VERSION 0)
set(${VARS_PREFIX}_MINOR_VERSION 11)
set(${VARS_PREFIX}_PATCH_VERSION 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking to stick to patch version 1 because I still need to release 0.11.1 .

Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version comment.

@francesco-romano
Copy link
Collaborator Author

Ok, commit dropped

@traversaro traversaro merged commit 874d88d into robotology:master Sep 10, 2018
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.

2 participants