Skip to content

bpo-36025: Restore original function API for PyDate_FromTimestamp#11922

Merged
berkerpeksag merged 1 commit intopython:masterfrom
pganssle:add_fromtimestamp_test_master
Apr 27, 2019
Merged

bpo-36025: Restore original function API for PyDate_FromTimestamp#11922
berkerpeksag merged 1 commit intopython:masterfrom
pganssle:add_fromtimestamp_test_master

Conversation

@pganssle
Copy link
Member

@pganssle pganssle commented Feb 18, 2019 •
edited
Loading

Per PyO3/pyo3#352, I believe we've broken the C API for PyDateTimeAPI->Date_FromTimestamp. As a test (and to make sure we don't do it again), I've developed these tests. They are currently passing on 3.7 and they should be failing on master / Python 3.8 since we're passing a tuple to a function that strictly takes int or float.

Once I verify that these tests are indeed failing on CI, (they are anomalously passing in my local directory against master), I will also fix the underlying problem.

We don't need a news item for this, as it fixes a regression that was only ever released in alpha.

https://bugs.python.org/issue36025

@pganssle
Copy link
Member Author

pganssle commented Feb 18, 2019

Ah, found the reason why it's anomalously passing - I accidentally shadowed the failing test with a passing one. 😅

@pganssle pganssle force-pushed the add_fromtimestamp_test_master branch from 6959922 to 0aadc17 Compare February 18, 2019 17:40
@pganssle pganssle mentioned this pull request Feb 18, 2019
Closed
@pganssle pganssle changed the title bpo-36025: Add tests for datetime fromtimestamp API bpo-36025: Restore original function API for PyDate_FromTimestamp Feb 18, 2019
serhiy-storchaka
serhiy-storchaka reviewed Feb 18, 2019
View reviewed changes
@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Feb 18, 2019

Since it fixes a regression in released version, I think it should be documented with a news entry.

@pganssle
Copy link
Member Author

pganssle commented Feb 18, 2019

@serhiy-storchaka To be clear, this is only a regression on the 3.8 branch. I didn't think alpha versions counted as "released versions", but if they do I can add a news entry.

@pganssle pganssle force-pushed the add_fromtimestamp_test_master branch from 760634c to f268f7a Compare February 18, 2019 20:50
@pganssle
Copy link
Member Author

pganssle commented Feb 18, 2019

@serhiy-storchaka Thanks for the review, I made all the suggested changes, except I haven't added a NEWS entry yet.

serhiy-storchaka
serhiy-storchaka reviewed Feb 19, 2019
View reviewed changes
@pganssle pganssle force-pushed the add_fromtimestamp_test_master branch 3 times, most recently from ded3ef2 to d9069e6 Compare February 19, 2019 19:43
@pganssle pganssle force-pushed the add_fromtimestamp_test_master branch from d9069e6 to 4040647 Compare February 19, 2019 19:44
@pganssle
Copy link
Member Author

pganssle commented Feb 27, 2019

@serhiy-storchaka Does this look good? Can we merge or are we waiting on something else?

@pganssle pganssle force-pushed the add_fromtimestamp_test_master branch from 4040647 to 8a597f8 Compare February 27, 2019 14:26
@pganssle pganssle force-pushed the add_fromtimestamp_test_master branch from 8a597f8 to 314f828 Compare March 8, 2019 16:09
@pganssle
Copy link
Member Author

pganssle commented Mar 8, 2019 •
edited
Loading

Just realized I never did this for this issue:

CC @abalkin

@abalkin abalkin self-requested a review March 8, 2019 20:42
@pganssle pganssle force-pushed the add_fromtimestamp_test_master branch from 314f828 to 016deca Compare March 17, 2019 19:13
@pganssle pganssle force-pushed the add_fromtimestamp_test_master branch from 016deca to 8f8f617 Compare March 17, 2019 19:16
@pganssle
Copy link
Member Author

pganssle commented Mar 17, 2019

The first force push since approval was an accident - included a What's New entry that was intended for another PR. The second one corrects that mistake and rebases this against the current master.

@abalkin @ambv Can we merge this before the next alpha release? It would be good to get at least the pyo3 3.8-dev build job working again.

@pganssle pganssle force-pushed the add_fromtimestamp_test_master branch from 8f8f617 to 2a231bc Compare March 17, 2019 19:45
@pganssle pganssle force-pushed the add_fromtimestamp_test_master branch from 2a231bc to 2cd3128 Compare April 13, 2019 00:14
ZackerySpytz
ZackerySpytz reviewed Apr 13, 2019
View reviewed changes
ZackerySpytz
ZackerySpytz reviewed Apr 13, 2019
View reviewed changes
Copy link
Contributor

@ZackerySpytz ZackerySpytz left a comment

Choose a reason for hiding this comment

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

There are reference leaks.

@pganssle pganssle force-pushed the add_fromtimestamp_test_master branch from 9c07733 to a61ec40 Compare April 13, 2019 20:15
@pganssle
Copy link
Member Author

pganssle commented Apr 13, 2019

@ZackerySpytz Thanks for the review! You are right about the refcounts, I have removed them. I guess PyTuple_Pack "steals" the existing reference to both its arguments, so I don't have to increase or decrease the reference count, just pass it into the tuple. I've checked that the reference count is unchanged before and after calling get_datetime_fromtimestamp and it seems it is now!

@pganssle pganssle force-pushed the add_fromtimestamp_test_master branch from a61ec40 to d971844 Compare April 20, 2019 18:31
@pganssle
Copy link
Member Author

pganssle commented Apr 23, 2019

@abalkin @serhiy-storchaka Next alpha release is coming up soon. Anything else you'd like me to do before merge?

@pganssle
bpo-36025: Fix PyDate_FromTimestamp API (pythonGH-11922)
ffe34ed 
In the process of converting the date.fromtimestamp function to use
argument clinic in pythonGH-8535, the C API for PyDate_FromTimestamp was
inadvertently changed to expect a timestamp object rather than an
argument tuple.

This PR fixes this backwards-incompatible change by adding a new wrapper
function for the C API function that unwraps the argument tuple and
passes it to the underlying function.

This PR also adds tests for both PyDate_FromTimestamp and
PyDateTime_FromTimestamp to prevent any further regressions.

bpo-36025
@pganssle pganssle force-pushed the add_fromtimestamp_test_master branch from d971844 to ffe34ed Compare April 27, 2019 19:16
@berkerpeksag berkerpeksag merged commit 4d8c8c0 into python:master Apr 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag berkerpeksag approved these changes

@abalkin abalkin abalkin approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

+1 more reviewer

@ZackerySpytz ZackerySpytz ZackerySpytz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@pganssle @serhiy-storchaka @berkerpeksag @abalkin @ZackerySpytz @the-knights-who-say-ni @bedevere-bot