Should this replace #17413?

@csabella I was not aware of #17413, but that PR includes a couple of things I missed (docs, tests, setup.py). I could pull in those bits from #17413 (or we could close this and merge the other one.) Also, there is a mismatch in the contents of that PR and the bpo it references.

UPDATE: Please see commit 602c81153fba3dc3d8e745048ea49d7388cafb0b, @csabella. As this PR now stands, it replaces #17413.

Ping, @csabella

@erlend-aasland, @berkerpeksag was already requested for review, so you'll need to wait for him to have a chance to look into this.

Ok, thank you, @csabella !

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

I just saw that you've combined changes from #17413. I think it would be better if you could add Sergey's email address in the co-authored-by trailer so he could properly attributed:

Sergey Fedoseev <fedoseev.sergey@gmail.com>

Please add Sergey's name to the NEWS entry as well.

And you can remove my name from the commit body since I haven't authored anything in the original PR :)

And you can remove my name from the commit body since I haven't authored anything in the original PR :)

Well, I incorporated your requested changes from #17413, so technically you did :)

Also, perhaps you can combine bpo-40810 in this PR as well?

Are you sure about that? This PR can (IMHO shoud) be backported to 3.9, 3.8 and probably even 3.7, because SQLite 3.7.3 won't work for any of them anyway (because of sqlite3_create_function_v2.) bpo-40810 should not be backported, no? Awaiting your answer before I update the PR.

I have made the requested changes; please review again. I did not include bpo-40810. I'll open a separate PR for that one.

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.

BTW, today it's 20 years since the first public SQLite alpha release. #funfact
https://sqlite.org/changes.html

Sorry for force-pushing and repeatedly touching this PR, but I made some (IMO) required changes:

• Add run-time version check
• Since this patch is against master, update Doc/whatsnew/3.10.rst, not Doc/whatsnew/3.9.rst
• Normalize build-time and run-time error messages

Are you ok with this, @berkerpeksag ?

Are you sure about that? This PR can (IMHO shoud) be backported to 3.9, 3.8 and probably even 3.7, because SQLite 3.7.3 won't work for any of them anyway (because of sqlite3_create_function_v2.)

Unless I'm missing something obvious, sqlite3_create_function_v2() was introduced in Python 3.9 (b9a0376) so SQLite 3.7.3 is a hard requirement for only Python 3.9+. Backporting this PR to 3.9 may make sense if we can demonstrate that compiling Python 3.9 with pre 3.7.3 SQLite is possible. Maybe we can just fix the version number at https://github.com/python/cpython/blob/3.9/setup.py#L1467 and be done with it.

Unless I'm missing something obvious, sqlite3_create_function_v2() was introduced in Python 3.9 (b9a0376) so SQLite 3.7.3 is a hard requirement for only Python 3.9+.

You're quite right. My bad. sqlite3_create_function_v2() came with b9a0376.

Backporting this PR to 3.9 may make sense if we can demonstrate that compiling Python 3.9 with pre 3.7.3 SQLite is possible. Maybe we can just fix the version number at https://github.com/python/cpython/blob/3.9/setup.py#L1467 and be done with it.

Just tested 3.9 w/SQLite 3.7.2, and it does not build bco. sqlite3_create_function_v2().

So, since this is only relevant for 3.9 and 3.10, we could of course include bpo-40810 in this PR. Since you already requested this, @berkerpeksag, I'll make the necessary changes and push again.

Just tested 3.9 w/SQLite 3.7.2, and it does not build bco. sqlite3_create_function_v2().

What happens if you change the following line to MIN_SQLITE_VERSION_NUMBER = (3, 7, 3)?

If it makes Python built without sqlite3 module, then I think it's the best behavior for 3.9.

What happens if you change the following line to MIN_SQLITE_VERSION_NUMBER = (3, 7, 3)?
[…]
If it makes Python built without sqlite3 module, then I think it's the best behavior for 3.9.

As expected, it does not include the sqlite3 module in the build. (Tested with a bare bones Debian w/SQLite 3.7.2 and a patched setup.py).

I guess built-in modules are statically linked anyway, so we can discard run-time checks, no?

How about this: Let's close this PR, close bpo-40744, and apply bpo-40810 to both 3.10 and 3.9.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Patches for master and branches (3.9) here must be applied to master first and backported.
This must be fixed or probably redone.

@berkerpeksag, are you ok with this plan:

1. Rebase this PR onto master, apply it, and backport to 3.9.
2. Rewrite bpo-40810: Fix CheckTraceCallbackContent for SQLite pre 3.7.15 #20530 to only fix the unit test. Apply to master and backport to 3.9 (and 3.8?)
3. Open a new issue for dropping SQLite pre 3.7.15 and apply to master

I have made the requested changes; please review again

Thanks for making the requested changes!

@berkerpeksag, @terryjreedy: please review the changes made to this pull request.

A backport can be partial if not all of it applies to the previous version. (But then is has to be done manually, so if one knows ahead of time which part will be backported, separate PRs may be easier. The most common case for manual backports is all to one or more versions and part or adjusted diff to older versions.) The comment on the initial message, visible when edit, used to say that backports should have 'Cherrypicked from ...'. Now it just says to just put [GH-#####] where ##### is the PR number of the original, rather than the backport. The backport bot does both this and the [3.x] prefix for us. If you actually did that, I apologize. Anyway, at this point, do whatever Berker says.

A backport can be partial if not all of it applies to the previous version. (But then is has to be done manually, so if one knows ahead of time which part will be backported, separate PRs may be easier. The most common case for manual backports is all to one or more versions and part or adjusted diff to older versions.) The comment on the initial message, visible when edit, used to say that backports should have 'Cherrypicked from ...'. Now it just says to just put [GH-#####] where ##### is the PR number of the original, rather than the backport. The backport bot does both this and the [3.x] prefix for us. If you actually did that, I apologize. Anyway, at this point, do whatever Berker says.

Thanks for the explanation. I think separate PR's are cleaner, both generally, and spefically in this case. (I hope Berker agrees, because I've rebased this PR too many times already :) ). I've already prepared the PR's as proposed in my previous comment.

Why was my review requested?

@berkerpeksag Have you had any chance to look at this? The plan is:

1. Rebase this PR onto master (done), apply it (awaiting your review), and backport to 3.9.
2. Rewrite bpo-40810: Fix CheckTraceCallbackContent for SQLite pre 3.7.15 #20530 to only fix the unit test (done). Apply to master (awaiting your review) and backport to 3.9 (and 3.8?)
3. Open a new issue for dropping SQLite pre 3.7.15 and apply to master (will do when 1. and 2. are merged)

@berkerpeksag Closing this bco. noise and the spurious review requests. Will open a clean PR for this bpo.