Dual-Life / Time-Piece

Object Oriented time objects
Other
15 stars 33 forks source link

Time::Piece reversibility #32

Open mla opened 7 years ago

mla commented 7 years ago

Thanks for maintaining this module.

I'm trying to parse a timestamp that includes the timezone, and then output it as a localtime date string. IOW, I want to do the following, except using Time::Piece instead of Time::Local:

my $time = timelocal(42, 7, 10, 19, 6, 2017);
print "  Epoch seconds: $time\n";
print "  CORE::localtime: ", scalar CORE::localtime($time), "\n";

That, for me, produces:

Using Time::Local
  Epoch seconds: 1500484062
  CORE::localtime: Wed Jul 19 10:07:42 2017

So the input was 10 o'clock, and the output is 10 o'clock.

However, if I try parsing that with strptime and outputting the result, I'm always getting GMT, no matter how I do it:

#!/usr/bin/env perl

use strict;
use warnings;
use Time::Local;
use Time::Piece;

my $datestr = '2017-06-19 10:07:42-0700';
print "Original date string: $datestr\n\n";

print "Using Time::Local\n";
my $time = timelocal(42, 7, 10, 19, 6, 2017);
print "  Epoch seconds: $time\n";
print "  CORE::localtime: ", scalar CORE::localtime($time), "\n";
print "\n";

print "Using Time::Piece:\n";

print "  strptime as static method:\n";
my $obj = Time::Piece->strptime($datestr, '%Y-%m-%d %H:%M:%S%z');
print "     stringified: $obj\n";
print "     datetime: ", $obj->datetime, "\n";
print "     tzoffset: ", $obj->tzoffset, "\n";
print "     hour: ", $obj->hour, "\n";

print "  strptime as localtime instance method:\n";
$obj = localtime->strptime($datestr, '%Y-%m-%d %H:%M:%S%z');
print "     stringified: $obj\n";
print "     datetime: ", $obj->datetime, "\n";
print "     tzoffset: ", $obj->tzoffset, "\n";
print "     hour: ", $obj->hour, "\n";

print "  strptime as gmtime instance method:\n";
$obj = gmtime->strptime($datestr, '%Y-%m-%d %H:%M:%S%z');
print "     stringified: $obj\n";
print "     datetime: ", $obj->datetime, "\n";
print "     tzoffset: ", $obj->tzoffset, "\n";

That produces:

Using Time::Piece:
  strptime as static method:
     stringified: Mon Jun 19 17:07:42 2017
     datetime: 2017-06-19T17:07:42
     tzoffset: 0
     hour: 17
  strptime as localtime instance method:
     stringified: Mon Jun 19 17:07:42 2017
     datetime: 2017-06-19T17:07:42
     tzoffset: -25200
     hour: 17
  strptime as gmtime instance method:
     stringified: Mon Jun 19 17:07:42 2017
     datetime: 2017-06-19T17:07:42
     tzoffset: 0
     hour: 17

The hour is always 17. Even when I successfully get tzoffset set, it's still displaying as 17.

I would think that if tzoffset is set, the object should be a "localtime" instance and anything generated should be in localtime. If you want to convert it to a gmtime instance, then call gmtime($localtime_instance) (just as you would with the the CORE:: localtime and gmtime functions).

Am I missing something?

Thanks.

smith153 commented 7 years ago

Hmm looks like there are no tests with strptime and '%z' currently. Looking at how the source handles the %z offset: https://github.com/Dual-Life/Time-Piece/blob/1.31_04/Piece.xs#L853 it simply adds the offset to the tm struct minutes and hours, sets the "got_gmt" flag (which it then does nothing with). So that explains why the time is wrong (it converts it to gmt essentially).

Seems like this should be fixable, but the issue is I don't think strptime was ever really made to convert time zones from one locale to another. I don't know of an OS portable way to take a broken down tm struct in gmt, and then convert that into something else.

What I can try is if '%z' is passed, I'll know that the tm parts that come back will have been offset into gmt, then perhaps I can do (internally) timegm(@tm_parts) to get epoch, then CORE::localtime($epoch) to return the right parts for what ever the current timezone is... But open to suggestions :)

smith153 commented 7 years ago

Oh and for a quick and dirty fix, if I take your test script and remove the time zone offset from the date string and remove the %z from the format string, I think I get the right behavior. So perhaps try parsing out the offset from your date strings.

mla commented 7 years ago

Hey, thanks for the quick response. I posted this on perlmonks too and got some helpful responses:

http://www.perlmonks.org/?node_id=1193021

I think what cguevara showed may be the same as what you're suggesting above? He parsed the timestamp with timezone using Time::Piece->strptime. And then passed its epoch seconds to localtime().

So I guess strptime always returns a gmtime, even when it parses a timezone. I guess that makes sense. And if you want to display with a different offset, you need to explicitly create a localtime() instance from it.

mla commented 7 years ago

It would maybe be nice though if something like what I originally tried did work. e.g.,

localtime->strptime($datestr, '%Y-%m-%d %H:%M:%S%z');

would indicate that, since it's parsing within the context of localtime, we want a localtime instance back. And if there is no %z specified, it would assume the localtime zone. Not sure how difficult that last bit is.

smith153 commented 7 years ago

So I guess strptime always returns a gmtime, even when it parses a timezone

Not quite. An "epoch" is the seconds from 1970. It is the same everywhere. But strptime returns actual "time parts", year, month, hour, minute, etc. Those can be local or gmt, the difference being gmt will never have a daylight savings period. Time::Piece->strptime() looks to default to returning a Time::Piece object with gmt set to true. localtime->strptime() will of course have gmt set to false, unless you have gmt set as your current system TZ. But there is obviously a bug with how strptime handles %z at the moment.

mla commented 7 years ago

Thanks. Attached are some tests that I think exhibit the issue with localtime->strptime and time zones.

I'm not sure how portable the tests are. I'm setting TZ to PST8PDT. Not sure if Windows can handle that?

time-piece-tz-test.txt

smith153 commented 7 years ago

I will look into adding the tests.

For my own reference this seems to be also related: https://rt.cpan.org/Ticket/Display.html?id=93095

mla commented 7 years ago

I was just skimming the code, and I suspect the got_GMT flag from _strptime is the key. If got_GMT is true, then _mktime should use timegm() regardless of whether the caller $islocal.

e.g., something like:

$time->[c_epoch] = $got_gmt || !$islocal ? timegm(@tm_parts): timelocal(@tm_parts);

And then $got_gmt needs to be a third, optional param to _mktime.

smith153 commented 7 years ago

Yea something like that I was thinking too. You can see how a more modern version of libc handles it at the bottom of the page here:https://opensource.apple.com/source/Libc/Libc-583/stdtime/strptime-fbsd.c.auto.html

See on the case 'z': line, tm->tm_gmtoff is set to the %z offset and then later on the case CONVERT_ZONE: block is ran which has t = timeoff(tm, offset);

Strangely, on an even newer libc: https://opensource.apple.com/source/Libc/Libc-1044.40.1/stdtime/FreeBSD/strptime.c.auto.html

the process is different and only the CONVERT_GMT flag is set.

Either way, in C, I don't think we have a cross platform variant of timegm() or timeoff(). I could use mktime(), but that is not considered thread safe since it modifies a global TZ variable.

But we do have timelocal and timegm, but those are in perl. So from the perl strptime sub, we need to flag that the call to the C _strptime did not return as local... And seems we are in luck. Within return_11part_tm() in Piece.xs, the last object pushed onto the return stack array is for islocal: PUSHs(newSViv(0)), and its always zero (doesn't matter since the perl _mktime takes a separate $islocal arg). We can change the signature on return_11part_tm() to take an extra arg for islocal. Then we can do PUSHs(newSViv(islocal)). From the return on _strptime (on the perl side), we can then look at the @vals that came back. If we passed in islocal as 1 and within the returned @vals we see islocal is 0, then we will know we will need to convert and use the perl variants of timelocal and timegm. Or I think....