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

Just remove rcpputils::fs dependency #72

Merged
merged 5 commits into from Oct 5, 2023
Merged

Just remove rcpputils::fs dependency #72

merged 5 commits into from Oct 5, 2023

Conversation

yoneken
Copy link
Contributor

@yoneken yoneken commented Sep 28, 2023

Because the package will be no longer available. ros2/rcpputils#164

@yoneken
Remove rcpputils::fs dependency
cf60e79 
Signed-off-by: Kenta Yonekura <yoneken@ieee.org>
@yoneken
Copy link
Contributor Author

yoneken commented Sep 28, 2023

Oops.. I will fix it later.

@yoneken
fix
8534aa0 
Signed-off-by: Kenta Yonekura <yoneken@ieee.org>
@yoneken
Copy link
Contributor Author

yoneken commented Sep 28, 2023

Ready for review.

clalancette
clalancette requested changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've left some thoughts inline.

I think we can also remove rcpputils as a dependency from this package.

rmw_dds_common/src/security.cpp Outdated Show resolved Hide resolved
rmw_dds_common/src/security.cpp Outdated Show resolved Hide resolved
rmw_dds_common/src/security.cpp Outdated Show resolved Hide resolved
@yoneken
Copy link
Contributor Author

yoneken commented Sep 28, 2023

Thanks for your valuable comments and suggestions!
rcpputils is still remained at https://github.com/ros2/rmw_dds_common/blob/rolling/rmw_dds_common/src/qos.cpp#L711 .

@yoneken
Remove unnecessary loop
cc1908f 
Signed-off-by: Kenta Yonekura <yoneken@ieee.org>
fujitatomoya
fujitatomoya approved these changes Oct 2, 2023
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

clalancette
clalancette requested changes Oct 2, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This almost looks good to me, though there is one more thing I think we should do.

In particular, in

rmw_dds_common/rmw_dds_common/CMakeLists.txt

Line 24 in f4b7aa0

ament_export_dependencies(rcpputils)
, we are exporting rcpputils as a dependency. But we don't need to do that; rcpputils is now an implementation detail. So if we just remove that line, then I'll be happy with this and we can run CI on it.

@yoneken
Remove an export dependency
d6f6216 
Signed-off-by: Kenta Yonekura <yoneken@ieee.org>
clalancette
clalancette approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Thanks for the iteration! This looks good to me now. I'll run CI on it next.

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

@yoneken So it looks like specifically on Windows, the security tests are failing now. Can you take a look?

@yoneken
Copy link
Contributor Author

yoneken commented Oct 5, 2023

I have added some changes for windows. Could you run CI?

@yoneken
small treatment for windows
90d405c 
Signed-off-by: Kenta Yonekura <yoneken@ieee.org>
@clalancette
Copy link
Contributor

Here's another try at CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette merged commit d3095f8 into ros2:rolling Oct 5, 2023
3 checks passed
@yoneken yoneken deleted the migrate_std branch October 5, 2023 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno ivanpauno is a code owner

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@yoneken @clalancette @fujitatomoya