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.

Sign up for GitHub

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

Jump to bottom

tizinfo.fromutc changed for tzinfo wih StdOffset=0, DstOffset=1 #67788

Closed
PeterJCLaw mannequin opened this issue Mar 7, 2015 · 15 comments
Closed

tizinfo.fromutc changed for tzinfo wih StdOffset=0, DstOffset=1 #67788

PeterJCLaw mannequin opened this issue Mar 7, 2015 · 15 comments
Assignees
@abalkin
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@PeterJCLaw
Copy link
Mannequin

PeterJCLaw mannequin commented Mar 7, 2015

BPO 23600
Nosy @tim-one, @abalkin, @PeterJCLaw, @serhiy-storchaka
Files
  • time_issues.py: Shorter demo of the issue
  • issue23600.diff
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/abalkin'
closed_at = <Date 2015-09-28.17:48:47.544>
created_at = <Date 2015-03-07.00:43:03.247>
labels = ['extension-modules', 'type-bug']
title = 'tizinfo.fromutc changed for tzinfo wih StdOffset=0, DstOffset=1'
updated_at = <Date 2015-09-28.19:30:39.956>
user = 'https://bugs.python.org/peterjclaw'

    bugs.python.org fields:

    activity = <Date 2015-09-28.19:30:39.956>
actor = 'tim.peters'
assignee = 'belopolsky'
closed = True
closed_date = <Date 2015-09-28.17:48:47.544>
closer = 'belopolsky'
components = ['Extension Modules']
creation = <Date 2015-03-07.00:43:03.247>
creator = 'peterjclaw'
dependencies = []
files = ['38372', '40603']
hgrepos = []
issue_num = 23600
keywords = ['patch']
message_count = 15.0
messages = ['237407', '237414', '237440', '251672', '251675', '251717', '251725', '251734', '251735', '251737', '251741', '251743', '251744', '251785', '251793']
nosy_count = 7.0
nosy_names = ['tim.peters', 'belopolsky', 'python-dev', 'PeterJCLaw', 'serhiy.storchaka', 'alynn', 'peterjclaw']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue23600'
versions = ['Python 3.4', 'Python 3.5', 'Python 3.6']
    @PeterJCLaw
    Copy link
    Mannequin Author

    PeterJCLaw mannequin commented Mar 7, 2015

    There's a difference in behaviour between the fromutc method on a tzinfo between Python 2 and Python 3, though only under the specific case of Summer Time in regions whose usual offset is 0.

    From what I can tell, it's the Python 3 one which is wrong, based on it no longer matching the sample implementation provided in the docs and on it appearing to report the wrong times for datetime.now(tz) when on a machine configured for BST during June 2015.
    Similar results can also be achieved using a manually constructed datetime for dates during summer 2014.

    I've put the python script (and sample outputs) I used to investigate in a gist at https://gist.github.com/PeterJCLaw/d8bcc168d68acf066811#file-time_issues-py. The outputs there are for pythons 2.7.9 and 3.4.0.

    @PeterJCLaw PeterJCLaw mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Mar 7, 2015
    @abalkin
    Copy link
    Member

    abalkin commented Mar 7, 2015

    Peter,

    Can you attach your demo script to the issue? Better yet, is it possible to explain the issue without referring to 100 lines of code?

    @PeterJCLaw
    Copy link
    Mannequin Author

    PeterJCLaw mannequin commented Mar 7, 2015

    Hi,

    Sorry for the overkill demo. I've attached a much shorter version, the key portion of which seems to be that, for the case of UK summer time the timezone, the tzinfo's dst() and utcoffset() methods return the same value.

    This results in the delta between the two (which my understanding suggests equates to the non-dst offset of the timezone) is zero (which is right).

    The python implementations (both in datetime.py and in the docs) cope with this by checking the DST difference and applying this after the timezone adjustments have happened.

    From a look through the CPython implementation, it looks to me like it's checking the wrong value before applying the DST difference. Specifically, on line 3033 in Modules/_datetimemodule.c, it checks delta before applying the DST.

    For the majority of timezones this happens to work since the standard offset is not 0 and it ends up doing the addition anyway (and no functionality is lost if the DST value is 0).

    Having tested changing the checked value to being dst rather than delta this does appear to fix this and all the existing tests still pass.

    Peter

    @abalkin
    Copy link
    Member

    abalkin commented Sep 26, 2015

    I am afraid you misunderstand how fromutc() method works. Note that you rarely need to call it directly: use astimezone() method to convert between timezones.

    @tim-one
    Copy link
    Member

    tim-one commented Sep 27, 2015

    I expect Peter is correct: the C fromutc() doesn't match the logic of the Python fromutc(), and there are no comments explaining why the C version changed the logic.

    The last 4 lines of his time_issues.py show the difference. The simplified UKSummerTime tzinfo always says the total UTC offset and the DST offset are +1:00:00. The Python .fromutc() adds that hour to the datetime passed in, but the C .fromutc() does not. That's because they implement different functions, not because Peter is using .fromutc() ;-)

    @abalkin abalkin self-assigned this Sep 27, 2015
    @abalkin
    Copy link
    Member

    abalkin commented Sep 27, 2015

    It looks like I introduced this bug ~ 5 years ago when I made a switch from integer minutes offset to an arbitrary timedelta. It's rather amazing that it took so long to discover it.

    @abalkin
    Copy link
    Member

    abalkin commented Sep 27, 2015

    Attaching a patch that should fix the issue.

    @tim-one
    Copy link
    Member

    tim-one commented Sep 28, 2015

    Patch looks good to me! Thanks :-)

    @abalkin
    Copy link
    Member

    abalkin commented Sep 28, 2015

    Thanks, Tim. Now I need to figure out how to commit to multiple branches. This goes to 3.4 through 3.6, right? or just to 3.5 and 3.6.

    @abalkin abalkin added extension-modules C modules in the Modules dir and removed stdlib Python modules in the Lib dir labels Sep 28, 2015
    @tim-one
    Copy link
    Member

    tim-one commented Sep 28, 2015

    Afraid that's a question for python-dev - I lost track of the active branches over year ago :-(

    @abalkin
    Copy link
    Member

    abalkin commented Sep 28, 2015

    I give up. Somehow my changes conflict with

    parent: 98335:0d3b64bbc82c
    user: Serhiy Storchaka <storchaka@gmail.com>
    date: Sun Sep 27 22:38:33 2015 +0300
    summary: Issue bpo-25203: Failed readline.set_completer_delims() no longer left the

    and my knowledge of hg is not enough to get out. When are we switching to git?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 28, 2015

    New changeset ff68705c56a8 by Alexander Belopolsky in branch '3.4':
    Closes issue bpo-23600: Wrong results from tzinfo.fromutc().
    https://hg.python.org/cpython/rev/ff68705c56a8

    New changeset c706c062f545 by Alexander Belopolsky in branch '3.5':
    Closes issue bpo-23600: Wrong results from tzinfo.fromutc().
    https://hg.python.org/cpython/rev/c706c062f545

    New changeset 4879c10ce982 by Alexander Belopolsky in branch 'default':
    Closes issue bpo-23600: Wrong results from tzinfo.fromutc().
    https://hg.python.org/cpython/rev/4879c10ce982

    New changeset cbcf82f92c25 by Alexander Belopolsky in branch '3.4':
    Closes issue bpo-23600: Wrong results from tzinfo.fromutc().
    https://hg.python.org/cpython/rev/cbcf82f92c25

    New changeset 848d665bc312 by Alexander Belopolsky in branch '3.5':
    Closes issue bpo-23600: Wrong results from tzinfo.fromutc().
    https://hg.python.org/cpython/rev/848d665bc312

    @abalkin
    Copy link
    Member

    abalkin commented Sep 28, 2015

    OK, I have no idea how I managed to create two commits in 3.4 and 3.5 and loose the NEWS entry in the end.

    @abalkin abalkin closed this as completed Sep 28, 2015
    @PeterJCLaw
    Copy link
    Mannequin Author

    PeterJCLaw mannequin commented Sep 28, 2015

    Awesome, thanks for fixing this.

    @tim-one
    Copy link
    Member

    tim-one commented Sep 28, 2015

    Thank you for your persistence and patience, Peter! It shouldn't have been this hard for you :-(

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @abalkin abalkin

    Labels
    extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @abalkin @tim-one