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

Revert "Revert "Add a create_timer method to Node and LifecycleNode classes (#1975)" (#2009) #2010

Merged
merged 2 commits into from
Aug 31, 2022
Merged

Revert "Revert "Add a create_timer method to Node and LifecycleNode classes (#1975)" (#2009) #2010

merged 2 commits into from
Aug 31, 2022

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented Aug 30, 2022 •
edited
Loading

... and fixes the bug introduced in the original PR.

@ivanpauno ivanpauno added the enhancement New feature or request label Aug 30, 2022
@ivanpauno ivanpauno requested a review from sloretz August 30, 2022 20:42
@ivanpauno ivanpauno self-assigned this Aug 30, 2022
@ivanpauno ivanpauno force-pushed the ivanpauno/fix-create-timer-bug branch from b1967a6 to 4c4cd42 Compare August 30, 2022 20:45
@sloretz
Copy link
Contributor

sloretz commented Aug 30, 2022

We can merge this in replacement of #2009, or after that one is merged (adding here a "revert-revert" commit).

Oops! I just merged the revert.

@ivanpauno
Revert "Revert "Add a create_timer method to Node and `LifecycleN…
e1efffb 
…ode` classes (#1975)" (#2009)"

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

This reverts commit 11b5f8d.
@ivanpauno
Fix implementation bug in one create_timer() overload
40e26f7 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno force-pushed the ivanpauno/fix-create-timer-bug branch from 4c4cd42 to 40e26f7 Compare August 30, 2022 20:47
sloretz
sloretz approved these changes Aug 30, 2022
View reviewed changes
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

LGTM with green CI --packages-above rclcpp

@ivanpauno ivanpauno changed the title Fix implementation bug in one create_timer() overload Revert "Revert "Add a create_timer method to Node and LifecycleNode classes (#1975)" (#2009) Aug 30, 2022
@ivanpauno
Copy link
Member Author

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

@ivanpauno
Copy link
Member Author

@Blast545 have you seen those warnings on Windows before?

from the logs:

17:54:35 CMake Warning (dev) at C:/Program Files/CMake/share/cmake-3.24/Modules/ExternalProject.cmake:3071 (message):
17:54:35 The DOWNLOAD_EXTRACT_TIMESTAMP option was not given and policy CMP0135 is
17:54:35 not set. The policy's OLD behavior will be used. When using a URL
17:54:35 download, the timestamps of extracted files should preferably be that of
17:54:35 the time of extraction, otherwise code that depends on the extracted
17:54:35 contents might not be rebuilt if the URL changes. The OLD behavior
17:54:35 preserves the timestamps from the archive instead, but this is usually not
17:54:35 what you want. Update your project to the NEW behavior or specify the
17:54:35 DOWNLOAD_EXTRACT_TIM

It seems to be happening in all vendor packages.

@ivanpauno
Copy link
Member Author

@ros-pull-request-builder retest this please

@clalancette
Copy link
Contributor

@Blast545 have you seen those warnings on Windows before?

Yes, this is a known issue with the newer version of CMake (3.24). @Crola1702 is working on it.

@ivanpauno ivanpauno merged commit df994e4 into rolling Aug 31, 2022
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/fix-create-timer-bug branch August 31, 2022 13:21
@iuhilnehc-ynos iuhilnehc-ynos mentioned this pull request Sep 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloretz sloretz sloretz approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees

@ivanpauno ivanpauno

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ivanpauno @sloretz @clalancette @fujitatomoya