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

Revert "Remove ament macros" #99

Closed
wants to merge 1 commit into from
Closed

Conversation

ChrisThrasher
Copy link
Collaborator

This reverts commit 7cb590a.

Here I come back tucking my tail between my legs. We wrote install interface tests and even did some manual testing with this new RSL version in the wild using projects that depend on RSL. We found no issues so we felt comfortable shipping. Alas something about the Debian packaging of RSL results in an rsl::rsl target which seemingly lacks the correct interface install directories or interface link libraries resulting in numerous build failures.

We're unsure why this is the case or why any of this is happening or whether adding ament back will even fix anything.

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b63f048) 99.06% compared to head (f103fb5) 99.06%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #99   +/-   ##
=======================================
  Coverage   99.06%   99.06%           
=======================================
  Files          11       11           
  Lines         215      215           
=======================================
  Hits          213      213           
  Misses          2        2           
Flag Coverage Δ
humble 99.06% <ø> (ø)
rolling 99.06% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChrisThrasher
Copy link
Collaborator Author

moveit/moveit2#2578

We might have found a fix. Switching to consuming RSL via target_link_libraries appears to fix this issue.

@ChrisThrasher ChrisThrasher marked this pull request as ready for review December 6, 2023 22:56
@ChrisThrasher ChrisThrasher marked this pull request as draft December 6, 2023 22:56
@ChrisThrasher
Copy link
Collaborator Author

We decided to stick with our ament-free solution so no need to revert anything!

It's not a bug; it's a feature!

@ChrisThrasher ChrisThrasher deleted the apparently_we_need_ament branch December 6, 2023 23:19
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

1 participant