fix test suite

[doudou]

It was not passing. base/types ... !

[2maz]

Looks like it is still not passing

../base/types/test/test_Time.cpp(110): fatal error: in "Time/fromString": critical check formatNow.toMicroseconds() == expected_utc_us + 1001 has failed [1339668306001001 != 1339671906001001]

[doudou]

Looks like it is still not passing

Damn.

A major difference with the behavior on my machine is that formatNow.toMicroseconds() returns the UTC time on your machine, local time on mine.

Could you run timedatectl ? To see whether your system clock is set in local time or UTC time. Could be a major difference.

[doudou]

OK, so ... I dug even a little bit more.

strptime does not change the time fields when given a time zone. %Z is ignored on libc. %z sets a non-standard field (tm_gmtoff). See the source code

tm_gmtoff is documented here

mktime assumes its argument to be in local time. There is timegm which does the same for UTC, but then it's also non-standard.

To be honest, I don't see a robust way to implement this method without spending an incredible amount of time (and probably still having a few corner-cases). I personally move to deprecate it wholesale and flag it as "don't use, unexpected behaviors may happen in various time zones. If you need to save/load time, save the seconds-since-epoch value returned by toMilliseconds()".

[2maz]

Could you run timedatectl ? To see whether your system clock is set in local time or UTC time. Could be a major difference.

$timedatectl [12:23:06]
Local time: Do 2019-12-12 12:23:20 CET
Universal time: Do 2019-12-12 11:23:20 UTC
RTC time: Do 2019-12-12 11:23:20
Time zone: Europe/Berlin (CET, +0100)
System clock synchronized: yes
systemd-timesyncd.service active: yes
RTC in local TZ: no

What issues would you see if we rely on http://man7.org/linux/man-pages/man3/tzset.3.html which is used by 'date in coreutils' and use "timezone" to set the value to be UTC.

btw that is still #109

[doudou]

OK, misread the timestamps.

The value returned by fromString is 10:05, the expected value is actually 11:05 (once corrected by local time). The test value from the string is 12:05. So, I'm guessing problem detecting summer time since you're currently in winter (+1) and 14/06 was obviously summer. The code assumes that mktime is going to detect summer time. It maybe does, but using the current date and not the date in the tm struct ?

Which leads me back to my previous comment:

To be honest, I don't see a robust way to implement this method without spending an incredible amount of time (and probably still having a few corner-cases). I personally move to deprecate it wholesale and flag it as "don't use, unexpected behaviors may happen in various time zones. If you need to save/load time, save the seconds-since-epoch value returned by toMilliseconds()".

tzset() initializes the timezone variable, which allows to correct for the effect of mktime, which turns a local time ("local time" being in this case the test value of 12:05) into GMT time.

[2maz]

If you need to save/load time, save the seconds-since-epoch value returned by toMilliseconds()".

If you are in control of the full process ok, but you still might what to read a formatted timestamp from a different source.

I see the issue in the assumption of base Time implying local time..
If we want to avoid it altogether we have to base all values on UTC, i.e. enforce toString to return UTC and fromString as well. I tried to sketch something along these lines in #144

[doudou]

I see the issue in the assumption of base Time implying local time..

Just for the record, since I already said that in #144, but the safest assumption by far is to assume that base::Time stores UTC. base::Time::now() returns system time, which is in most cases UTC.

[doudou]

If you are in control of the full process ok, but you still might what to read a formatted timestamp from a different source.

My conclusion is that if you do need to do that, use another language that has a decent time representation and parsing, and use that language to convert to epoch. C/C++ does not give us the tools to do so, and I'd rather not pull a library in base/types to do that - or spend any more time to do it. I think that having the test suite for the robust parts of base/types passing is of widely more importance.

In addition, given that nobody was hit by this situation so far, I'm guessing that these methods are not widely used - or are at least not used to interpret historical data. Hence my position of "let's drop it".

For the record (again :P), C++20 will provide the tools for clock conversion it seems.

[2maz]

In addition, given that nobody was hit by this situation so far, I'm guessing that these methods are not widely used - or are at least not used to interpret historical data. Hence my position of "let's drop it".

I am using it and would prefer not to be forced to look for a replacement - so updated #144.