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.

Sign up for GitHub

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

Jump to bottom

Remove action msgs dependency #115

Merged
merged 3 commits into from
Mar 16, 2024
Merged

Remove action msgs dependency #115

merged 3 commits into from
Mar 16, 2024

Conversation

christophfroehlich
Copy link
Contributor

action_msgs is not needed within this repo.

@christophfroehlich christophfroehlich mentioned this pull request Mar 14, 2024
Merged
saikishor
saikishor requested changes Mar 14, 2024
View reviewed changes
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

control_msgs/control_msgs/CMakeLists.txt

Line 13 in 7c90617

find_package(action_msgs REQUIRED)

Can you also remove this line?
Apart from that looks good to me. Thanks for the cleanup.

@christophfroehlich
Copy link
Contributor Author

Can you also remove this line? Apart from that looks good to me. Thanks for the cleanup.

sure, sorry I forgot that.

saikishor
saikishor approved these changes Mar 14, 2024
View reviewed changes
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

LGTM

fmauch
fmauch approved these changes Mar 15, 2024
View reviewed changes
Copy link
Contributor

@fmauch fmauch left a comment

Choose a reason for hiding this comment

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

Looks clean to me.

Simply out of interest: Do you have any source of why that's not needed? I always added it in the past.
I noticed that in the action tutorial it went away from humble to iron, but it doesn't show in the Iron release notes.

More specifically, this post suggests that it was indeed needed in the past and then was stripped down to having it in the package.xml was still required.

@christophfroehlich
Copy link
Contributor Author

Phew. I haven't looked that up at all to be honest. I saw no direct dependency in our messages, and it just compiled without.

@bmagyar bmagyar merged commit 93b7ca7 into master Mar 16, 2024
5 of 7 checks passed
@bmagyar bmagyar deleted the rm_action_msgs branch March 16, 2024 09:49
This was referenced Mar 18, 2024
Merged
Merged
christophfroehlich added a commit that referenced this pull request Mar 18, 2024
@christophfroehlich
Revert "Remove action msgs dependency (#115)"
11897d5 
This reverts commit 93b7ca7.
@christophfroehlich christophfroehlich mentioned this pull request Mar 18, 2024
Merged
bmagyar pushed a commit that referenced this pull request Mar 22, 2024
@christophfroehlich
Readd action msgs (revert #115) (#119)
ffddb62
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmauch fmauch fmauch approved these changes

@bmagyar bmagyar bmagyar approved these changes

@saikishor saikishor saikishor approved these changes

@arne48 arne48 Awaiting requested review from arne48

@destogl destogl Awaiting requested review from destogl

@erickisos erickisos Awaiting requested review from erickisos

@jaron-l jaron-l Awaiting requested review from jaron-l

@moriarty moriarty Awaiting requested review from moriarty

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@christophfroehlich @fmauch @bmagyar @saikishor