Get rid of time 0.1.* dependency

[mpizenberg]

Checklist

• I have read and agree to Contributor Guide and the Code of Conduct
• I've included a screenshot or gif (if applicable)
• I've added a line to CHANGELOG.md (if this is a big enough change to warrant it)

According to cargo audit, rerun dependency to time 0.1.* is tied to its dependency to chrono 0.4. And Rerun is dependent on chrono in three different ways:

1. Using chrono in re_log_types to format times
2. Transitively via arrow2
3. Transitively via polars

But in fact, time has been an optional dependency to chrono for quite some time, and can be avoided by setting default-features = false. Arrow2 is already doing this, but not polars. Well polars merged a PR last week removing chrono default features, but it hasn't reached a published version yet.

So I tried disabling chrono default features in rerun and overriding polars with their latest git commit. This is indeed removing time 0.1.* from our dependencies :) Now of course re_log_types does not compile anymore because of a call to chrono::offset::Utc::now().

error[E0599]: no function or associated item named `now` found for struct `Utc` in the current scope
  --> crates/re_log_types/src/time.rs:96:70
   |
96 |                     if datetime.date_naive() == chrono::offset::Utc::now().date_naive() {
   |                                                                      ^^^ function or associated item not found in `Utc`

So in theory, if we fix that, when the next version of polars is published we will be rid of the time CVE.

[mpizenberg]

Just to be clear, this PR is not ready to be merged, it's the first step of fixing the time issue, now waiting on guidance from rerun team to see how to proceed.

[emilk]

Thank you so much for working on this! ❤️

Excellent work on figuring out that time is an opt-out feature of chrono - that makes things a bit simpler!

Getting rid of the use of chrono::offset::Utc::now() in re_log_types can only be done by using the time crate directly (version 0.3). If we replace that call we might as well replace all the uses of chrono in crates/re_log_types/src/time.rs with the time crate. It's only a few format strings that need changing. time can format dates and times too, it just has a slightly different syntax for doing it:

let time_format = time::format_description::parse("[year]-[month]-[day] [hour]:[minute]:[second]Z")?;
let date_time = time::OffsetDateTime::now_utc().format(&time_format)?;

---

As for the patched crate: we avoid patching our crates because it can easily prevent us from publishing new rerun crates. polars is also an opt-in dependency that we currently don't use at all (it is never pulled in for users).

I'll convert this PR to a draft until it compiles :]

[mpizenberg]

Noted, I'll try to do the chrono/time swap later tonight.

[mpizenberg]

I have made some changes in re_log_types to use time instead of chrono. It resulted in a few logic changes as the edge cases do not seem to be exactly the same, as there is no Single | None | Ambiguous variant for datetimes.

Apparently chrono is also used in re_viewer so we might need to change it there too.

A few remarks after working on this:

The purpose of is_abolute_date(&self) seems to be to differentiate cases where Time is lower than 20 years, and in such cases, consider that it is a relative time, and format time with only the seconds.

This seems like a bad idea to me, as you leave some arbitrary implicit condition inside a formatting function. This may lead to weird to track bug in the future. I would see two ways to fix this situation. Either differentiate relative and absolute times in rerun types, with each their own set of rules and functions. A bit like the Duration (relative) and OffsetDateTime (absolute) types in the time crate. Or differentiate the formatting functions for absolute and relative time, let the caller decide.

I also do not like very much the internal logic asking for the current date, to see if we can omit the day part of the displaying logic. It does not seem robust to me because the implicit logic may prove to be annoying to deal with for other use cases. Imagine you want to format times for logging, which will later be analyzed by some other code. While logging the formatted date, they all will follow the "current day" logic, but when reviewing these logs few days later will be impossible because we won't know which date it referred to.

Basically, I think it would be better to extract all implicit logic and side effects to caller sites, where someone clearly takes responsibility of these decisions, instead of using a heuristic.

Finally, representing dates with unix timestamp in nanoseconds with i64 can only go as far as a couple hundred years. Depending on usage, for things like simulations and such, I can easily imagine weird situations where the i64 representation is not enough. For reference, the time library uses i128 for unix timestamps in nanoseconds.

PS: I haven't looked at why but arrow2 doesn't like the new polars it seems.

[mpizenberg]

The remarks above about implicit internal logic and side effects does not matter for this PR, but if you think it is relevant for later, I'll open an issue about it later.

Also, the time code type check, but I haven't executed it to check that the formatting is actually correct. Will do during the week.

[emilk]

We have chosen to use i64 for all our time types for simplicity and speed. We could distinguish between relative and absolute by associating them with different TimeType:s, but at that point we probably also want to support different epochs. For supporting century-scale data: that again could be solved with a different TimeType (intepreting the i64 as years for astronomical simulation or whatever), but I'd rather punt on all this until we have an actual user for which the current system is lacking. We have enough things to do 😅. So: if you need century-scale time data, please open an issue explaining your use case! But let's keep this PR discussion on-topic.

[emilk]

I think this function is unused and can be removed

[emilk]

Also, the time code type check, but I haven't executed it to check that the formatting is actually correct. Will do during the week.

Awesome! Adding a little unit-test would be great. I should have done so myself when I last touched the code 😬

[emilk]

Please also revert Cargo.lock - you seem to have run a full cargo update, which is outside the scope of this PR!

[emilk]

Apparently chrono is also used in re_viewer so we might need to change it there too.

I don't think it is, actually. It seems like an unused dependency!

[mpizenberg]

Awesome! Adding a little unit-test would be great. I should have done so myself when I last touched the code 😬

Alright, will try to do that

[mpizenberg]

Please also revert Cargo.lock - you seem to have run a full cargo update, which is outside the scope of this PR!

Clearing the cargo.lock was the only way I had found to make sure I had gotten rid of the time 0.1 dependency while checking with cargo audit so I thought it was worth putting in the PR, simply so can others can reproduce. I’ll get rid of the lock changes and the current patch crate.

[mpizenberg]

I've removed the chrono patching override with the git repo. So for the time being we still have time 0.1.* in the dependencies, but as soon as polars updates, it should be fixed.

polars is also an opt-in dependency that we currently don't use at all (it is never pulled in for users).

Should I do anything about this or just leave it as is?

I've minimized the changes to the cargo lock file by resetting it to its state in the main branch and doing a cargo check afterward.

I've updated the formatting function in re_viewer and removed the unused one in re_log_types.

The only thing left should be adding a unit test for formatting.

[emilk]

Code LGTM, and a couple of unit tests would make this ⭐️⭐️⭐️

[mpizenberg]

I've added tests for the formatting.

Beware that the test for checking formatting of datetimes for today may flake as It's not guaranteed that now for generating the test date, and now called inside the formatting function could differ if this is launched closed to 00:00 UTC

[emilk]

Looks great!