Use time zone string instead of offset. #78

Merged
merged 2 commits into from Mar 22, 2012

Projects

None yet

4 participants

@sorbits
Contributor
sorbits commented Oct 9, 2011

The advantage is that I can set the string to Europe/Copenhagen and not have to update my time zone offset twice a year (for daylight savings time).

I also did a commit which removes the configuration support for the (old) time zone offset (sorbits/znc@6c2a91b), but since this gives a load error for users who doesn’t manually remove TimezoneOffset from their configuration file, I am hesitant to submit a pull request for that — what is the migration strategy for removing configuration options?

sorbits added some commits Oct 9, 2011
@sorbits sorbits Introduce time zone (string) setting.
Presently unused but intended to supersede the current time zone offset.

The advantage of a string is that it can be set to something like Europe/Copenhagen and thereby adapt to daylight savings time.
b824709
@sorbits sorbits Use time zone string instead of offset.
This is done by setting the TZ variable prior to calling localtime() or ctime().

There are a few calls to ctime()/localtime() which doesn’t adjust for time zone offset. This patch affects the behavior of these since the TZ variable is global.
eca0211
@DarthGandalf
Member

On 09.10.2011 22:50, Allan Odgaard wrote:

what is the migration strategy for removing configuration options?

To try to still be able to read old option from existing configs, and to
try to still have the same behavior as before, if possible and not too
difficult. And maybe print a warning to user that the config was
converted to new format.

@DarthGandalf
Member

In this case it would be something like...
When reading old deprecated option TimezoneOffset from config, somehow to understand from current TZ and from the offset, which TZ need to set for the user.
As for implementation, not sure how to do that, but need to read TZ env variable at beginning, to have the default value not affected by our own setenv(), and also to reset TZ to it before doing /znc restart so that restarted ZNC would have the same default TZ

@sorbits
Contributor
sorbits commented Oct 9, 2011

Seeing how the offset was only applied in half the places that deals
with time, that the offset is relative to the sytem’s time zone (not
GMT/UTC), and that a typical server (running bouncers) probably
doesn’t update time zone during daylight savings (unlike users of the
bouncer), I concluded that the old system had too many flaws to be kept
around ;)

But if you think it is beneficial, I see a few approaches.

One is to keep the old setting and use it if set (or if new setting is
unset). This has code complexity costs, but will ensure same behavior
and is probably the simplest to implement.

Another solution is to convert the old offset into a string like
“GMT+HH:MM”, this eliminates having two paths for all the time
handling code, but for bouncers not running in GMT/UTC, it will not
result in the same offset as the old code.

A third solution is to investigate system’s time zone, the user’s
offset, and reference a database to find a symbolic name that matches
— this is hairy.

On 9 Oct 2011, at 18:07, Alexey Sokolov wrote:

In this case it would be something like...
When reading old deprecated option TimezoneOffset from config, somehow
to understand from current TZ and from the offset, which TZ need to
set for the user.
As for implementation, not sure how to do that, but need to read TZ
env variable at beginning, to have the default value not affected by
our own setenv(), and also to reset TZ to it before doing /znc restart
so that restarted ZNC would have the same default TZ

Reply to this email directly or view it on GitHub:
#78 (comment)

@DarthGandalf
Member

One more approach:

Get current time in current TZ, get current time in UTC, then add the
difference between them to offset, and use the sum in "UTC+HH:MM" string

@sorbits
Contributor
sorbits commented Oct 9, 2011

This only works when TZ is set to something absolute (like CET). But TZ
can contain DST start/stop dates and for the GNU C lib it defaults to
‘:/etc/localtime’ (i.e. the contents of the file /etc/localtime).

On 9 Oct 2011, at 18:58, Alexey Sokolov wrote:

One more approach:

Get current time in current TZ, get current time in UTC, then add the
difference between them to offset, and use the sum in "UTC+HH:MM"
string

Reply to this email directly or view it on GitHub:
#78 (comment)

@Mikaela
Contributor
Mikaela commented Oct 30, 2011

This would be nice to see in ZNC. This morning all buffer playbacks had weird timestamps, because of moving of clocks and ZNC using old timezone offset. System of ZNC is on UTC time and as admin, I fixed timestamps of all users. After setting correct timestamps, I was directed to here from #ZNC at freenode.

@psychon
Member
psychon commented Oct 30, 2011

Mkaysi: That problem should already be accidentally-fixed in commit c36480c. Timestamps are now added on playback and not when the message is received.

@sorbits
Contributor
sorbits commented Oct 30, 2011

psychon: The issue isn’t that the time stamp gets added when writing the line to the buffer, the issue is when creating the time stamp in the first place as most servers do not observe DST so now all European users will have to adjust their config to alter the TZ offset by one hour and Americans will have to do the same in a week.

My patch changes the TZ setting from an hours offset to specify a region, e.g. Europe/Copenhagen.

That said, the commit you refer to mention something about server time, so perhaps other things have been changed in ZNC to address the problem my patch fixes.

@DarthGandalf
Member

If users who change offset every half year anyway because of DST, will need to fix it once more (for the last time), it's ok.
Those who don't, shouldn't need to fix it after upgrade, IMHO...
How many countries have/don't have DST?

@Mikaela
Contributor
Mikaela commented Jan 14, 2012

How many countries have/don't have DST?

Wikipedia says:

Most areas of North America and Europe observe daylight saving time (DST), while most areas of Africa and Asia do not. South America is mixed, with most countries in the warmer north of the continent near the equator not observing DST, while Chile, Paraguay, Uruguay and parts of southern Brazil do. Oceania is also mixed, with New Zealand and parts of southern Australia observing DST, while most islands do not.

@DarthGandalf DarthGandalf merged commit eca0211 into znc:master Mar 22, 2012
@MrLenin MrLenin pushed a commit to evilnet/znc that referenced this pull request Mar 29, 2014
@DarthGandalf DarthGandalf Merge znc/znc#78 (TZ)
Conflicts:
	modules/admin.cpp
	src/User.cpp
f0cab46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment