Skip to content

Clean CMake for HDE V2#87

Closed
lrapetti wants to merge 3 commits intogbionics:release/HDEv2from
lrapetti:remove-dependency-from-xsens-mvn
Closed

Clean CMake for HDE V2#87
lrapetti wants to merge 3 commits intogbionics:release/HDEv2from
lrapetti:remove-dependency-from-xsens-mvn

Conversation

@lrapetti
Copy link
Contributor

@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
Contributor Author

@diegoferigo Can you review the changes?

set(CMAKE_POSITION_INDEPENDENT_CODE ON)
endif()

set(CMAKE_POSITION_INDEPENDENT_CODE ON)
Copy link
Contributor

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

Choose a reason for hiding this comment

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

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

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
Contributor

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
Contributor

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
Contributor

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
Contributor

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
Contributor

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

@traversaro
Copy link
Contributor

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
Contributor Author

@diegoferigo What is failing?

@diegoferigo
Copy link
Contributor

@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
Contributor Author

This PR is deprecated after #100.
closing

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

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

@traversaro
Copy link
Contributor

@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
traversaro pushed a commit that referenced this pull request Sep 23, 2024
Add wearable yarp dumper application to run HDE offline
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