feat: Add human readable date to logging formats #441

Kaju-Bubanja · 2023-11-27T14:48:04Z

No description provided.

Kaju-Bubanja · 2023-11-27T15:01:17Z

Some linters are failing, is there an autoformatter I can run on the diff?

ahcorde

there are some magic number thatwe should define as constant

include/rcutils/time.h

src/time.c

clalancette · 2023-11-27T15:30:16Z

Some linters are failing, is there an autoformatter I can run on the diff?

Yes, if you run colcon test --packages-select rcutils locally, it will run the linters for you.

src/logging.c

Kaju-Bubanja · 2023-11-27T15:51:30Z

All good suggestions, I implemented them. I'm fighting with the linter now. When I try and run:

colcon test --packages-select rcutils
Starting >>> rcutils 
--- stderr: rcutils                     
Errors while running CTest
Output from these tests are in: /home/user/ros2_humble/build/rcutils/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.
---
Finished <<< rcutils [27.7s]	[ with test failures ]

Summary: 1 package finished [30.4s]
  1 package had stderr output: rcutils
  1 package had test failures: rcutils

I tried:

colcon test --packages-select --output-on-failure --rerun-failed rcutils

but only get unrecognized argument. I checked the LastTest.log but there is nothing there. How can I show the offending file/line?

clalancette · 2023-11-27T15:59:53Z

but only get unrecognized argument. I checked the LastTest.log but there is nothing there. How can I show the offending file/line?

Try:

colcon test --event-handlers console_direct+ --packages-select rcutils

You can also look in log/latest_test/rcutils/stderr.log, which should also show the output.

Kaju-Bubanja · 2023-11-27T16:17:32Z

Some linters are failing, is there an autoformatter I can run on the diff?

Yes, if you run colcon test --packages-select rcutils locally, it will run the linters for you.

What I meant with autoformatter is a tool, which will automatically change the format applying some standard stylesheet, so that the style conforms with the linters. I got the linter to output what it's doing with --event-handlers console_direct+ as you suggested, but there are a few issues. Is the current workflow indeed to manually fix issues which the linter brings up?

clalancette · 2023-11-27T16:35:09Z

Is the current workflow indeed to manually fix issues which the linter brings up?

That's how I usually do it. I think you may be able to run ament_uncrustify --reformat to have it do some of the work for you.

Kaju-Bubanja · 2023-11-27T16:57:02Z

Alright, thank you for the comment. I fixed the linting issues.

include/rcutils/time.h

src/time.c

fujitatomoya

@Kaju-Bubanja can you add some test for this?

src/time.c

src/logging.c

Kaju-Bubanja · 2023-12-21T15:33:31Z

I added some tests but I don't get why the zero time on the jenkins machine is 1969-12-31 16:00:00 instead of 1970-01-01 01:00:00. Anybody got an idea how to fix this? And also what timepoint I should use for the test? Is 100ns fine?

fujitatomoya

a couple of comments.

src/time.c

ahcorde

tests are failing : rcutils.TestTimeFixture.test_rcutils_time_point_value_as_date_string

Kaju-Bubanja · 2024-01-24T15:55:45Z

I added some tests but I don't get why the zero time on the jenkins machine is 1969-12-31 16:00:00 instead of 1970-01-01 01:00:00. Anybody got an idea how to fix this? And also what timepoint I should use for the test? Is 100ns fine?

Yeah because of this. Not really my domain

fujitatomoya · 2024-01-24T20:15:28Z

Yeah because of this. Not really my domain

@Kaju-Bubanja i think we need to set timezone explicitly during this test?

Kaju-Bubanja · 2024-02-05T13:03:29Z

Yeah because of this. Not really my domain

@Kaju-Bubanja i think we need to set timezone explicitly during this test?

I checked tzinfo which says:

   The tzset() function initializes the tzname variable from the TZ
   environment variable.  This function is automatically called by
   the other time conversion functions that depend on the timezone.
   In a System-V-like environment, it will also set the variables
   timezone (seconds West of UTC) and daylight (to 0 if this
   timezone does not have any daylight saving time rules, or to
   nonzero if there is a time, past, present, or future when
   daylight saving time applies).

   If the TZ variable does not appear in the environment, the system
   timezone is used.  The system timezone is configured by copying,
   or linking, a file in the [tzfile(5)](https://man7.org/linux/man-pages/man5/tzfile.5.html) format to /etc/localtime.  A
   timezone database of these files may be located in the system
   timezone directory (see the FILES section below).

   If the TZ variable does appear in the environment, but its value
   is empty, or its value cannot be interpreted using any of the
   formats specified below, then Coordinated Universal Time (UTC) is
   used.

   The value of TZ can be one of two formats.  The first format is a
   string of characters that directly represent the timezone to be
   used:

So it looks like the runner needs to add the TZ variable and ideally set it to UTC. I don't know how to do that or if I even have the rights to change the environment variables of the runner. It would be great to get this useful feature into ROS. But I think somebody with more knowledge and maybe access needs to help us get this going.

test/test_time.cpp

fujitatomoya · 2024-02-20T19:37:31Z

CI:

Linux
Linux-aarch64
Windows

Kaju-Bubanja · 2024-02-26T13:53:24Z

I think it's ready @clalancette @fujitatomoya

fujitatomoya · 2024-02-26T20:12:55Z

@Kaju-Bubanja thanks for the fix!

CI:

Linux
Linux-aarch64
Windows

fujitatomoya · 2024-02-27T04:42:39Z

src/time.c

+  time_t now_t = (time_t)(seconds);
+  struct tm ptm = {.tm_year = 0, .tm_mday = 0};
+#ifdef _WIN32
+  if (localtime_s(&now_t, &ptm) == NULL) {


getting closer but windows CI is still unhappy, see https://ci.ros2.org/job/ci_windows/21119/msbuild/new/

Ofc win and Linux have source/dest switched around :D

Kaju-Bubanja · 2024-02-27T13:51:20Z

@fujitatomoya Can I do that too, I wonder:

CI:

Ok, doesn't look like it, I thought maybe there is a bot which adds the labels when you write CI:

clalancette · 2024-02-27T15:27:24Z

CI:

Linux
Linux-aarch64
Windows

clalancette

Besides the comment inline, Windows testing is failing in this code: https://ci.ros2.org/job/ci_windows/21127/testReport/junit/(root)/projectroot/test_time/

clalancette · 2024-02-28T13:00:58Z

src/time.c

+  time_t now_t = (time_t)(seconds);
+  struct tm ptm = {.tm_year = 0, .tm_mday = 0};
+#ifdef _WIN32
+  if (localtime_s(&ptm, &now_t) == NULL) {


MSVC is saying:

'==': 'errno_t' differs in levels of indirection from 'void *'

Indeed, looking up the documentation for localtime_s shows that it returns an errno_t, not a pointer. So at least this needs to be fixed.

Signed-off-by: Kaju Bubanja <bubanja.kaju@gmail.com>

Kaju-Bubanja · 2024-03-04T16:53:04Z

CI:

* Linux [![Build Status](https://camo.githubusercontent.com/b6ff6b6b4d14dccc76a72aa09b11bf43b0f658feeec07affe6cd49d8979a4a72/687474703a2f2f63692e726f73322e6f72672f6275696c645374617475732f69636f6e3f6a6f623d63695f6c696e7578266275696c643d3230333737)](http://ci.ros2.org/job/ci_linux/20377/)

* Linux-aarch64 [![Build Status](https://camo.githubusercontent.com/34957372f61666bb5834485aaeea740b608daa7c25244119009cc09b4a214402/687474703a2f2f63692e726f73322e6f72672f6275696c645374617475732f69636f6e3f6a6f623d63695f6c696e75782d61617263683634266275696c643d3134373836)](http://ci.ros2.org/job/ci_linux-aarch64/14786/)

* Windows [![Build Status](https://camo.githubusercontent.com/bd1f3c34dca61579747d61643719f02fac942a721578ce7eebeac7dd421bc0ad/687474703a2f2f63692e726f73322e6f72672f6275696c645374617475732f69636f6e3f6a6f623d63695f77696e646f7773266275696c643d3231313237)](http://ci.ros2.org/job/ci_windows/21127/)

Can I see these jobs myself, so to not loop you in unecessarily each time I fixed something? Because the checks which are running below always show 3 successful checks. Are these CI runs manually run by you and you link to the results or are they automatically run?

clalancette · 2024-03-04T16:54:11Z

Are these CI runs manually run by you and you link to the results or are they automatically run?

They are unfortunately run manually.

clalancette · 2024-03-08T14:10:11Z

Let's give CI another shot with the latest changes:

Linux
Linux-aarch64
Windows

clalancette

This looks good to me now. Thanks @Kaju-Bubanja for all of the iterations. @ahcorde you have the last remaining review, are you good with this now?

clalancette · 2024-03-15T14:14:18Z

I'm going to go ahead with this one. Thanks @Kaju-Bubanja .

Kaju-Bubanja · 2024-03-17T20:54:46Z

Thank you guys, happy we got this useful feature into rolling :)

* feat: Add human readable date to logging formats Signed-off-by: Kaju Bubanja <bubanja.kaju@gmail.com> Signed-off-by: Dave Rensberger <davidr@beechwoods.com>

ros-discourse · 2024-05-23T14:29:55Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-jazzy-jalisco-released/37862/1

Kaju-Bubanja requested review from ahcorde, clalancette and wjwwood as code owners November 27, 2023 14:48

Kaju-Bubanja mentioned this pull request Nov 27, 2023

Add date formatting #439

Closed

ahcorde requested changes Nov 27, 2023

View reviewed changes

include/rcutils/time.h Outdated Show resolved Hide resolved

src/time.c Outdated Show resolved Hide resolved

src/time.c Outdated Show resolved Hide resolved

src/time.c Outdated Show resolved Hide resolved

clalancette reviewed Nov 27, 2023

View reviewed changes

src/logging.c Outdated Show resolved Hide resolved

Kaju-Bubanja force-pushed the rolling branch from f00477e to 52dcd6b Compare November 27, 2023 15:54

Kaju-Bubanja force-pushed the rolling branch from 52dcd6b to a007a9d Compare November 27, 2023 16:56

Kaju-Bubanja force-pushed the rolling branch 2 times, most recently from 46bc8b0 to efa8356 Compare November 27, 2023 17:09

clalancette requested changes Nov 27, 2023

View reviewed changes

include/rcutils/time.h Outdated Show resolved Hide resolved

src/time.c Outdated Show resolved Hide resolved

Kaju-Bubanja force-pushed the rolling branch 3 times, most recently from dcf9497 to 1618bf4 Compare November 27, 2023 17:36

clalancette requested changes Nov 27, 2023

View reviewed changes

src/time.c Outdated Show resolved Hide resolved

src/time.c Outdated Show resolved Hide resolved

src/time.c Outdated Show resolved Hide resolved

src/time.c Outdated Show resolved Hide resolved

Kaju-Bubanja force-pushed the rolling branch 2 times, most recently from 378734b to d606f70 Compare November 28, 2023 16:53

fujitatomoya reviewed Dec 1, 2023

View reviewed changes

src/time.c Show resolved Hide resolved

clalancette requested changes Dec 2, 2023

View reviewed changes

src/time.c Outdated Show resolved Hide resolved

src/time.c Show resolved Hide resolved

src/logging.c Show resolved Hide resolved

Kaju-Bubanja force-pushed the rolling branch 2 times, most recently from 42d9c30 to bc7df0a Compare December 21, 2023 15:24

fujitatomoya requested changes Dec 21, 2023

View reviewed changes

src/time.c Outdated Show resolved Hide resolved

src/time.c Outdated Show resolved Hide resolved

ahcorde requested changes Jan 24, 2024

View reviewed changes

fujitatomoya reviewed Feb 5, 2024

View reviewed changes

test/test_time.cpp Show resolved Hide resolved

Kaju-Bubanja force-pushed the rolling branch from b2319f2 to fe7ea77 Compare February 14, 2024 13:41

fujitatomoya reviewed Feb 14, 2024

View reviewed changes

test/test_time.cpp Outdated Show resolved Hide resolved

Kaju-Bubanja force-pushed the rolling branch from fe7ea77 to c1e2b88 Compare February 20, 2024 13:50

Kaju-Bubanja force-pushed the rolling branch from c1e2b88 to 1e147e9 Compare February 22, 2024 15:18

fujitatomoya reviewed Feb 27, 2024

View reviewed changes

Kaju-Bubanja force-pushed the rolling branch from 1e147e9 to 247c9a5 Compare February 27, 2024 13:37

clalancette requested changes Feb 28, 2024

View reviewed changes

Kaju-Bubanja added 2 commits February 28, 2024 14:18

feat: Add human readable date to logging formats

4238790

Signed-off-by: Kaju Bubanja <bubanja.kaju@gmail.com>

feat: Add tests for rcutils_time_point_value_as_date_string

3ad9786

Signed-off-by: Kaju Bubanja <bubanja.kaju@gmail.com>

Kaju-Bubanja force-pushed the rolling branch from 247c9a5 to 3ad9786 Compare February 28, 2024 13:18

clalancette approved these changes Mar 8, 2024

View reviewed changes

clalancette merged commit 961c4c8 into ros2:rolling Mar 15, 2024
3 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add human readable date to logging formats #441

feat: Add human readable date to logging formats #441

Kaju-Bubanja commented Nov 27, 2023

Kaju-Bubanja commented Nov 27, 2023 •

edited

ahcorde left a comment

clalancette commented Nov 27, 2023

Kaju-Bubanja commented Nov 27, 2023

clalancette commented Nov 27, 2023

Kaju-Bubanja commented Nov 27, 2023

clalancette commented Nov 27, 2023

Kaju-Bubanja commented Nov 27, 2023 •

edited

fujitatomoya left a comment

Kaju-Bubanja commented Dec 21, 2023

fujitatomoya left a comment

ahcorde left a comment

Kaju-Bubanja commented Jan 24, 2024

fujitatomoya commented Jan 24, 2024

Kaju-Bubanja commented Feb 5, 2024

fujitatomoya commented Feb 20, 2024

Kaju-Bubanja commented Feb 26, 2024

fujitatomoya commented Feb 26, 2024

fujitatomoya Feb 27, 2024

Kaju-Bubanja Feb 27, 2024

Kaju-Bubanja commented Feb 27, 2024 •

edited

clalancette commented Feb 27, 2024

clalancette left a comment

clalancette Feb 28, 2024

Kaju-Bubanja commented Mar 4, 2024

clalancette commented Mar 4, 2024

clalancette commented Mar 8, 2024

clalancette left a comment

clalancette commented Mar 15, 2024

Kaju-Bubanja commented Mar 17, 2024

ros-discourse commented May 23, 2024

feat: Add human readable date to logging formats #441

feat: Add human readable date to logging formats #441

Conversation

Kaju-Bubanja commented Nov 27, 2023

Kaju-Bubanja commented Nov 27, 2023 • edited

ahcorde left a comment

Choose a reason for hiding this comment

clalancette commented Nov 27, 2023

Kaju-Bubanja commented Nov 27, 2023

clalancette commented Nov 27, 2023

Kaju-Bubanja commented Nov 27, 2023

clalancette commented Nov 27, 2023

Kaju-Bubanja commented Nov 27, 2023 • edited

fujitatomoya left a comment

Choose a reason for hiding this comment

Kaju-Bubanja commented Dec 21, 2023

fujitatomoya left a comment

Choose a reason for hiding this comment

ahcorde left a comment

Choose a reason for hiding this comment

Kaju-Bubanja commented Jan 24, 2024

fujitatomoya commented Jan 24, 2024

Kaju-Bubanja commented Feb 5, 2024

fujitatomoya commented Feb 20, 2024

Kaju-Bubanja commented Feb 26, 2024

fujitatomoya commented Feb 26, 2024

fujitatomoya Feb 27, 2024

Choose a reason for hiding this comment

Kaju-Bubanja Feb 27, 2024

Choose a reason for hiding this comment

Kaju-Bubanja commented Feb 27, 2024 • edited

clalancette commented Feb 27, 2024

clalancette left a comment

Choose a reason for hiding this comment

clalancette Feb 28, 2024

Choose a reason for hiding this comment

Kaju-Bubanja commented Mar 4, 2024

clalancette commented Mar 4, 2024

clalancette commented Mar 8, 2024

clalancette left a comment

Choose a reason for hiding this comment

clalancette commented Mar 15, 2024

Kaju-Bubanja commented Mar 17, 2024

ros-discourse commented May 23, 2024

Kaju-Bubanja commented Nov 27, 2023 •

edited

Kaju-Bubanja commented Nov 27, 2023 •

edited

Kaju-Bubanja commented Feb 27, 2024 •

edited