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

catkin not required for pure cmake packages #116

Merged
merged 2 commits into from
Dec 17, 2021

Conversation

Timple
Copy link
Contributor

@Timple Timple commented Dec 9, 2021

This prevens rosdep resolving in ROS2

This prevens rosdep resolving in ROS2
@ethzasl-jenkins
Copy link

Can one of the admins verify this patch?

@pomerlef
Copy link
Collaborator

pomerlef commented Dec 9, 2021

ok to test

@Timple
Copy link
Contributor Author

Timple commented Dec 14, 2021

If it speeds-up the review (or reduces risk), I can make the flag:

<run_depend condition="$ROS_VERSION == 1">catkin</run_depend>

@pomerlef
Copy link
Collaborator

yes, could you add that line (in libpointmatcher too). I'll try to process the PRs next week.

@pomerlef pomerlef merged commit 38d61b5 into norlab-ulaval:master Dec 17, 2021
@patriksc
Copy link

Hi! The change in the package.xml from this merge is causing issues when building our library COVINS: https://github.com/VIS4ROB-lab/covins

As soon as we call catkin build <xzy> (e.g. catkin build eigen_catkin), catkin throws
Error(s): The "run_depend" tag must not have the following attributes: condition, caused by libnabo/package.xml.

We found a workaround for now and removed the libnabo dependency, but maybe you want to consider whether this change is technically really necessary, since it seem like there might be some compatibility issues (we experienced this on 3 different machines with both Ubuntu 18 and Ubuntu 20).

@Timple
Copy link
Contributor Author

Timple commented Dec 18, 2021

Ow dear, that is a regression from my side. I forgot to add the new package format:

<package format="3">

Which supports the condition flag. PR #119

Strange though, I added this condition exactly for the reason not to break backward compatibility. Apologies for that. I would expect it to break CI though.

@patriksc
Copy link

Indeed, would as well expect CI catches this... Anyway, no problem, thanks for the quick reply and for taking care of this.

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