Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use time.tzset() to apply timezone changes when we can #1036

Merged
merged 1 commit into from Apr 28, 2017

Conversation

AdamWill
Copy link
Contributor

Threading makes this tricky, but this should at least apply any
'new' timezone whenever it gets changed to the datetime spoke
thread and the main thread. As I discovered in RHBZ #1433560,
Python's behaviour seems to have changed between 3.5 and 3.6
such that changes made to the 'TZ' environment variable don't
affect Python's "current" timezone (as used by things like the
datetime module) until time.tzset() is called.

This and #1035 would each fix RHBZ #1433560 without the other being applied. But they are also compatible and I think applying both would make sense. #1035 makes set_system_date_time work even if Python's idea of the 'current' timezone is not the same as the timezone the function is called with. This PR tries to update Python's idea of the current timezone when it's changed in anaconda (at least as well as we can, I think threading makes this more complex). This has consequences beyond set_system_date_time - the one I've noticed is that the timestamps on anaconda log files are in whatever Python currently considers the 'local time' to be, but there may be other impacts also.

@jkonecny12 jkonecny12 added the master Please, use the `f39` label instead. label Apr 24, 2017
@jkonecny12
Copy link
Member

Jenkins, it's ok to test.

@jkonecny12
Copy link
Member

I'm not sure if I understand it correctly, however my testing of these two patches compatibility went badly.
I tested it this way:

epoch = datetime.datetime.utcfromtimestamp(0).astimezone(tz)
epoch2 = tz.localize(datetime.datetime.fromtimestamp(0))
time.tzset()
epoch3 = tz.localize(datetime.datetime.fromtimestamp(0))
epoch4 = datetime.datetime.utcfromtimestamp(0).astimezone(tz)

but the output is:
epoch -> "1970-01-01 01:00:00+01:00" -- should be correct
epoch2 -> "1970-01-01 00:00:00+01:00" -- bad
epoch3 -> "1970-01-01 01:00:00+01:00" -- should be correct
epoch4 -> "1970-01-01 00:00:00+01:00" -- should be correct if patches are compatible !

Am I missing something or these patches aren't compatible?

If these patches aren't compatible I'm more for this particular one. It looks more robust solution.

@AdamWill
Copy link
Contributor Author

Aha, OK, so I think I know why it's wrong: utcfromtimestamp returns a naive object, not an aware one. If you try this code in Python 2 it actually flat out bails, with astimezone() telling you it can't work on a naive object, but for some reason Python 3 doesn't bail in this case, it just does...something wrong. (Not quite sure what).

We can fix it by making the object representing the epoch at UTC aware. This makes the line rather long, but it seems to work properly. My original proposal also seems to work:

epoch = datetime.datetime.fromtimestamp(0, tz=tz)

but if I'm understanding all the stuff at https://pypi.python.org/pypi/pytz correctly, this isn't strictly safe when daylight savings is involved. I can't quite grok if it'd ever really fail for this specific time; I think it may go wrong if the epoch date is in daylight savings but the current date is not (or vice versa). So let's go with the armor-plated version:

utc = pytz.timezone("UTC")
epoch = utc.localize(datetime.datetime.utcfromtimestamp(0)).astimezone(tz)

This actually lines up with the pytz documentation on converting between timezones. If you search the page for "Converting between timezones is more easily done" and read the example below, it's basically this, just split across more lines.

I'll adjust the PR now. Thanks for catching this.

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Well done.

Just a question @AdamWill, do you want this only to master? If you want this to f26 please rebase it on f26-devel.

Threading makes this tricky, but this should at least apply any
'new' timezone whenever it gets changed to the datetime spoke
thread and the main thread. As I discovered in RHBZ #1433560,
Python's behaviour seems to have changed between 3.5 and 3.6
such that changes made to the 'TZ' environment variable don't
affect Python's "current" timezone (as used by things like the
datetime module) until `time.tzset()` is called.
@AdamWill AdamWill changed the base branch from master to f26-devel April 28, 2017 07:31
@AdamWill
Copy link
Contributor Author

Huh. I did branch off f26-devel. No idea why the PR wound up against master. Oh, well. Fixed.

@jkonecny12 jkonecny12 added the f26 label Apr 28, 2017
@M4rtinK
Copy link
Contributor

M4rtinK commented Apr 28, 2017

@AdamWill I think GitHub automatically select the master branch when you create a pull request, regardless of the origin of your branch. This can be easy to miss if there are no merge conflicts due to the incorrect branch.

@M4rtinK M4rtinK merged commit c68cc1e into rhinstaller:f26-devel Apr 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f26 master Please, use the `f39` label instead.
3 participants