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

bpo-40956: Convert _sqlite3.Cursor to Argument Clinic #24007

Merged
merged 6 commits into from
Jan 5, 2021

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Dec 30, 2020

@erlend-aasland
Copy link
Contributor Author

Current _sqlite3 AC conversion status:

@erlend-aasland
Copy link
Contributor Author

@corona10 / @serhiy-storchaka, would one of you like to review this? I've gone through the argument specs twice, but please take an extra look in case I've missed something.

Modules/_sqlite/cursor.c Outdated Show resolved Hide resolved
Modules/_sqlite/cursor.c Outdated Show resolved Hide resolved
Modules/_sqlite/cursor.c Outdated Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

FYI, Ubuntu tests fail at test_ttk_guionly, not at test_sqlite.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Dec 30, 2020

The macOS check at https://github.com/python/cpython/runs/1625929931 triggered by e9535b8 should've failed: the sqlite3 module was not built! (The fix, commit 64b9c36, was pushed right after e9535b8)

I guess this should be reported as a CI bug, @serhiy-storchaka?

EDIT
Not a CI bug, but a "build bug"?

bash$ git checkout e9535b83a13acb1b7b9b63ba43cbb57834d2e9cb
bash$ make
[…]
Modules/_sqlite/cursor.c:645:50: error: use of undeclared identifier 'parameters'
    return _pysqlite_query_execute(self, 1, sql, parameters);
                                                 ^
1 error generated.

Python build finished successfully!
[…]
Failed to build these modules:
_sqlite3                                                       
[…]
bash$ echo $?
0

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Dec 30, 2020

PTAL, @serhiy-storchaka. Any idea why test_ttk_guionly on Ubuntu keeps failing? I can't see that that test uses sqlite3 in any way...

EDIT force-pushed to trigger CI

@erlend-aasland
Copy link
Contributor Author

Thanks for reviewing, @serhiy-storchaka !

@serhiy-storchaka
Copy link
Member

test_ttk_guionly is something unstable test. Sometimes (rarely) it fails on buildbots.

But currently test_nntplib is failed because of network issues.

@erlend-aasland
Copy link
Contributor Author

FYI, I'm ready with the PR for _sqlite3.Cache when this gets merged, @serhiy-storchaka. It's a pretty small PR. The diff stat is 3 files changed, 43 insertions(+), 18 deletions(-) excluding autogenerated clinic code.

@serhiy-storchaka
Copy link
Member

Please merge with master to fix test failures.

@erlend-aasland
Copy link
Contributor Author

Please merge with master to fix test failures.

Done. Is there anything more you need from me before merging?

@berkerpeksag berkerpeksag merged commit c7f8d3c into python:master Jan 5, 2021
@erlend-aasland erlend-aasland deleted the bpo-40956/part4-cursor branch January 5, 2021 23:57
@erlend-aasland
Copy link
Contributor Author

Thanks, @berkerpeksag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants