Skip to content

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

Merged
ivanpauno merged 2 commits intorollingfrom
ivanpauno/fix-create-timer-bug
Aug 31, 2022
Merged

Revert "Revert "Add a create_timer method to Node and LifecycleNode classes (#1975)" (#2009)#2010
ivanpauno merged 2 commits intorollingfrom
ivanpauno/fix-create-timer-bug

Conversation

@ivanpauno
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

ivanpauno commented Aug 30, 2022

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

@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented Aug 31, 2022

@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
Copy Markdown
Member Author

ivanpauno commented Aug 31, 2022

@ros-pull-request-builder retest this please

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Aug 31, 2022

@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