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

Clean CMake for HDE V2 #87

Conversation

lrapetti
Copy link
Member

@lrapetti lrapetti commented Jan 9, 2019

This PR aims to fix the CMake configuration of HDE with two improvements:

  • remove dependency from robotology-playground/xsens-mvn (It will no longer be necessary with HDEv2 that will depend upon wearables) 914332f
  • fix the CMake configuration for HumanROSMsgs since it is a header-only library (with the current configuration, it is preventing HDE to be installed on Windows) d716e44
  • Force plugins installation as Dynamic Libraries d84cd1f

@lrapetti
Copy link
Member Author

@diegoferigo Can you review the changes?

set(CMAKE_POSITION_INDEPENDENT_CODE ON)
endif()

set(CMAKE_POSITION_INDEPENDENT_CODE ON)
Copy link
Member

Choose a reason for hiding this comment

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

What is the rationale for this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed with @diegoferigo there are no cons in having it ON with BUILD_SHARED_LIBS ON

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the rationale for this change?

So this was a trap?!

Copy link
Member

Choose a reason for hiding this comment

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

So this was a trap?!

I simply forgot about what our recommended way of handling CMAKE_POSITION_INDEPENDENT_CODE was, and I had to check how-to-export-cpp-library to remember the details.

@diegoferigo diegoferigo self-requested a review January 10, 2019 14:32
@diegoferigo
Copy link
Member

It is not compiling on Ubuntu 16.04. I think it is related to the compiler that is too old, iDynTree does not support it. We should have a discussion to define minimum requirements for the AnDy related software. Since there are no users beyond us and considering that it is a fairly new project (especially wearables), I would tend to be as less conservative as possible.

@traversaro
Copy link
Member

iDynTree does not support it

I think iDynTree should have full support for Ubuntu 16.04, no?

Since there are no users beyond us and considering that it is a fairly new project (especially wearables), I would tend to be as less conservative as possible.

Even if the project is new, as it is strictly related to Andy project, I think you should evaluate (depending on the future project demo), if it may make sense to consider if we need to be compatible with some environments of the project partners. A typical case with robotics academic lab is that sometimes they are bound to relatively old Ubuntu versions (not the latest LTS, for example) due to the need to remain compatible with some specific LTS ROS release. @DanielePucci

@diegoferigo
Copy link
Member

I think iDynTree should have full support for Ubuntu 16.04, no?

Are you sure that the default compiler version on 16.04 is compatible with iDynTree?

I think you should evaluate (depending on the future project demo), if it may make sense to consider if we need to be compatible with some environments of the project partners.

We should have a meeting on this

@traversaro
Copy link
Member

Are you sure that the default compiler version on 16.04 is compatible with iDynTree?

At least a subset of the possible configuration combinations are tested in Travis on 16.04 : https://travis-ci.org/robotology/idyntree .

@diegoferigo
Copy link
Member

I probably got confused with robotology/idyntree#436. Maybe the incompatibility was before switching to docker when Travis was not yet using Ubuntu Xenial.

@traversaro
Copy link
Member

Just to be clear, I am not opposing at all dropping support for 16.04, I just suggest to double check with @DanielePucci .

@lrapetti
Copy link
Member Author

@diegoferigo What is failing?

@diegoferigo
Copy link
Member

@lrapetti HumanDynamicsEstimator does not compile on ubuntu xenial, check the Travis build of this PR

@DanielePucci
Copy link
Contributor

@diegoferigo @lrapetti

We should have a meeting on this

Feel free to discuss this whenever you want if compilation on 16.04 will not be fixed

@lrapetti
Copy link
Member Author

This PR is deprecated after #100.
closing

@lrapetti lrapetti closed this Apr 11, 2019
@traversaro
Copy link
Member

I think we can close this PR, as after #100 it outdated.

@traversaro
Copy link
Member

@lrapetti

@lrapetti lrapetti deleted the remove-dependency-from-xsens-mvn branch April 11, 2019 14:46
@DanielePucci DanielePucci changed the title Fix CMake Clean CMake for HDE Apr 29, 2019
@DanielePucci DanielePucci changed the title Clean CMake for HDE Clean CMake for HDE V2 Apr 29, 2019
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.

None yet

4 participants