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 impl pointer for ExecutorOptions #2523

Merged
merged 2 commits into from
May 9, 2024

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented May 9, 2024

Follow up of #2505 (comment), we need this to potentially add more functionality to Jazzy post release, as well it gives us more flexibility to fix bugs in the future.

We already have this for the Executor, so this class is the only one that needs adjustment:

std::unique_ptr<ExecutorImplementation> impl_;

Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood wjwwood self-assigned this May 9, 2024
@wjwwood
Copy link
Member Author

wjwwood commented May 9, 2024

CI:

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

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Windows is failing, probably you are missing some RCLCPP_PUBLIC in the new class

Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
Copy link
Member Author

wjwwood commented May 9, 2024

Yup, I definitely did, but it should be fixed now, new 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.

Looks good to me with green CI.

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.

looks good to me, test failures are unrelated? (currently CI is unstable?)

@clalancette
Copy link
Contributor

looks good to me, test failures are unrelated? (currently CI is unstable?)

Yeah, aarch64 is currently unstable. We can ignore those for now.

@clalancette clalancette merged commit 343b29b into rolling May 9, 2024
3 checks passed
@clalancette clalancette deleted the wjwwood/executor_options_impl_pointer branch May 9, 2024 20:32
@clalancette
Copy link
Contributor

@Mergifyio backport jazzy

Copy link

mergify bot commented May 9, 2024

backport jazzy

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request May 9, 2024
* add impl pointer for ExecutorOptions

Signed-off-by: William Woodall <william@osrfoundation.org>
(cherry picked from commit 343b29b)
clalancette pushed a commit that referenced this pull request May 10, 2024
* add impl pointer for ExecutorOptions

Signed-off-by: William Woodall <william@osrfoundation.org>
(cherry picked from commit 343b29b)

Co-authored-by: William Woodall <william@osrfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants