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

Fix compilation with YARP 3.2.101 #65

Merged
merged 1 commit into from
Nov 22, 2019

Conversation

lrapetti
Copy link
Member

@lrapetti lrapetti commented Nov 14, 2019

This PR fixes #56.

The current implemented solution is working both with YARP master (3.2.0) and devel (3.2.101). In order to enable this we had to remove the scoping from the messages include. Eventually, we can ripristinate those after the release of YARP considering the changing in the yarp installing directoy for the autogenerated headers.

@traversaro
Copy link
Member

Ack, but I guess the current devel is 3.2.101, see https://github.com/robotology/yarp/blob/devel/CMakeLists.txt#L37 .

@lrapetti lrapetti changed the title Fix compilation with YARP 3.2.1 Fix compilation with YARP 3.2.101 Nov 14, 2019
@lrapetti lrapetti closed this Nov 14, 2019
@lrapetti lrapetti force-pushed the fixing-compilation-YARP-devel branch from feaa128 to 4ea2588 Compare November 14, 2019 16:53
@lrapetti lrapetti reopened this Nov 14, 2019
@lrapetti
Copy link
Member Author

Ack, but I guess the current devel is 3.2.101, see https://github.com/robotology/yarp/blob/devel/CMakeLists.txt#L37 .

thanks, I have fixed the description in the commit

@lrapetti
Copy link
Member Author

Probably @diegoferigo can have a look before merging

@lrapetti lrapetti self-assigned this Nov 19, 2019
@traversaro
Copy link
Member

Friendly ping?

@diegoferigo
Copy link
Member

@lrapetti is in mission this week. Last time I spoke with him we agreed to try simplifying the include situation that is a bit convoluted in this moment. He told me that as soon as he's be back he will dofew tests and then merge this PR.

@lrapetti
Copy link
Member Author

Last time I spoke with him we agreed to try simplifying the include situation that is a bit convoluted in this moment

@traversaro If there is no particular need of having it merged this week, as @diegoferigo said it would be good to clean it a bit before merging since some parts can be probably improved/removed. Otherwise we can merge and then improve.

@traversaro
Copy link
Member

Last time I spoke with him we agreed to try simplifying the include situation that is a bit convoluted in this moment

@traversaro If there is no particular need of having it merged this week, as @diegoferigo said it would be good to clean it a bit before merging since some parts can be probably improved/removed. Otherwise we can merge and then improve.

The build of wearables against YARP devel branch as been broken for more than 1.5 months, and we already had experienced in the past (for robotology/yarp#2054) that delaying testing with new version of our dependencies may create problems in our demo, especially because the policy for IIT's iCub robots is to track unstable branches of software developed at IIT to catch as early as possible integration problems. As a YARP release is expected by the end of the month, I would at least try to fix the compilation of wearables, so that if somebody needs to reproduce wearables-related demo in a setup and needs to use the latest YARP it can without problems, and we can test also just compilation as part of robotology/robotology-superbuild#277 .

You may think that if somebody needs to get wearables to compile with latest YARP, he/she can easily switch to use the branch of @lrapetti's fork, but can you imagine if the maintainers of any dependency of Wearables/HDE such as YARP and iDynTree had the same attitude, and you had to manually search branches to which to switch to get the system to compile in the version necessary to be consistent with the software running on the robot? As you may imagine, that would be a great productivity problem.

To be honest, I do not seem any kind of specific regression in the PR, and I do not see the downside to get it merged/ If a cleanup is desirable for maintainability reasons, I think it can easily be done after this PR is merged and #56 is fixed.

cc @DanielePucci

@DanielePucci
Copy link

If there is no specific regression, I agree with @traversaro that having compilation problems against YARP devel branch for 1.5 month is not good, all the more so because the PR could address these issues. So, if there is no other problem with the merge, we may keep further clean up in subsequent PRs and proceed with this one. Thank you guys

@lrapetti @diegoferigo

@lrapetti lrapetti merged commit aa991c5 into robotology:master Nov 22, 2019
@lrapetti lrapetti mentioned this pull request Nov 22, 2019
@traversaro
Copy link
Member

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.

Compilation failing against YARP devel and master
4 participants