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

Add missing build_export_depend dependency #665

Merged
merged 2 commits into from
Feb 22, 2022
Merged

Conversation

Blast545
Copy link
Contributor

Follow up PR to #660.
This PR is failing in the cyclonedds jobs, starting from this one: https://build.ros2.org/view/Rci/job/Rci__nightly-cyclonedds_ubuntu_focal_amd64/608/

I'm not sure if this is exactly the cause of the problem, I'll run a couple of tests before removing draft mode into this PR.
FYI: @sloretz

Signed-off-by: Jorge Perez jjperez@ekumenlabs.com

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545 Blast545 self-assigned this Feb 21, 2022
@Blast545
Copy link
Contributor Author

Trying to replicate on ci.ros2.org, using only cyclonedds and up-to rosidl_generator_py:
Build Status

Running same parameters with this PR:
Build Status

@Blast545 Blast545 marked this pull request as ready for review February 22, 2022 15:45
@Blast545
Copy link
Contributor Author

CI tests worked fine, running standard full CI:

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

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.

Could you explain a little more why you think this is the correct fix? Looking at the python code in here, there are no direct references to rosidl_runtime_cpp. There are references to rosidl_runtime_cpp in the tests, but that should be covered by the existing test_depend. There are also references to rosidl_runtime_cpp in the empy templates, which suggests to me that maybe this should be a build_export_depend instead. @sloretz any thoughts?

@Blast545
Copy link
Contributor Author

@clalancette The lines of code generating this issue come from #660, in particular https://github.com/ros2/rosidl/blob/master/rosidl_generator_cpp/cmake/rosidl_generator_cpp_generate_interfaces.cmake#L119-L121

I wasn't sure if build_export_depend was enough in this case and tried with exec_depend. I can change this PR to use build_export_depend instead.

@sloretz
Copy link
Contributor

sloretz commented Feb 22, 2022

I wasn't sure if build_export_depend was enough in this case and tried with exec_depend. I can change this PR to use build_export_depend instead.

build_export_depend should be enough since rosidl_runtime_cpp is a header only library.

add_library(${PROJECT_NAME} INTERFACE)

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545 Blast545 changed the title Add missing exec dependency Add missing build_export_depend dependency Feb 22, 2022
@Blast545
Copy link
Contributor Author

Re running exploratory check:
Build Status

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.

Looks good with green CI, thanks!

@Blast545
Copy link
Contributor Author

Full CI:

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

@Blast545
Copy link
Contributor Author

Warnings not related to this PR, merging, thanks for the feedback!

@Blast545 Blast545 merged commit 917a40d into master Feb 22, 2022
@delete-merged-branch delete-merged-branch bot deleted the blast545/followup_PR_660 branch February 22, 2022 21:09
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

3 participants