Fixed #31765 -- Disabled bundled SQLite renaming atomic references on macOS 10.15. #13153
Conversation
orf
changed the title
| def supports_atomic_references_rename(self): | ||
| if Database.sqlite_version_info < (3, 26, 0): | ||
| return False | ||
|
|
There was a problem hiding this comment.
Could we possibly skip the detection feature if Database.sqlite_version_info >= (3, 29, 0)? Since this is only present on macOS could we branch off platform.platform? I think this would partially address @claudep's concerns.
There was a problem hiding this comment.
We can do this, but I feel that it's not correct. This code should work with 3.29.0 but there's clearly a configuration option or a bug that prevents it from working. As such, are we confident that >=(3, 29, 0) will always work?
For the same reason I'd be against branching of platform.platform. I don't think the aim here should be to "detect this specific MacOS version of SQLite" but should be more generally "Detect if sqlite actually supports renaming atomic references". I don't see how we can do that without feature detection.
There was a problem hiding this comment.
So do we know what's causing the bug in the first place? If not, shouldn't we investigate the cause before finding the workaround to apply?
There was a problem hiding this comment.
In an ideal world, yes. But I don't have the time to deep-dive into this, all I can say for sure is that the default sqlite library on MacOS doesn't exhibit the expected behaviour across the three machines I've tried it on. I'm not sure if it's a bug or an explicit decision made by Apple for backwards compatibility reasons.
I'd suggest it might be the latter, and that they probably maintain an internal fork of SQLite with custom patches rather than the "vanilla" distribution. So we can either:
- exclude sqlite3
3.28.0when running on MacOS10.15.5 - use feature detection
- deep dive into the MacOS XNU sources they publish, or otherwise report it to apple via RADAR, and see what they say
Honestly I'm not that bothered about which direction we take - this whole thing spun out of me trying to get the GSOC branch passing on my local machine 😭. I'm OK to exclude MacOS 10.15.x from this feature, but in case anyone in the future re-visits this because a future version of MacOS also doesn't work with this feature, I'd like to register my dislike!
|
Given the comments above I've switched the PR to exclude sqlite 3.28.0 on MacOS 10.15 👍 |
There was a problem hiding this comment.
OK, thanks for this @orf. I'd still like to find a way to introspect SQLite to detect this but (from ticket) I think you're right that the version combo is specific enough.
I skipped 10.15, so I'm struggling to reproduce: I'm going to upgrade to 11 so I'll look again at this then.
Possibly we'll need to revisit, but this should allow the tests to pass for folks on the current version now.
(I think 😀)
carltongibson
changed the title
|
Thanks all |
|
I reopened the issue here, since it continues to apply on new versions. We may have to revisit @orf's original approach d5b88be, perhaps only for macOS. 🤔 Update: as per comment on ticket it looks like we can adjust the |
Ticket: https://code.djangoproject.com/ticket/31765
tl;dr Python compiled with SQLite 3.28.0 that is bundled with MacOS Catalina fails the
schema.tests.SchemaTests.test_db_tabletest on master. Python compiled with SQLite 3.28.0 installed through Homebrew passes.