Skip to content

bpo-42686: Enable SQLite math functions in Windows build #24053

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
Merged
merged 2 commits into from
May 4, 2021
Merged

bpo-42686: Enable SQLite math functions in Windows build #24053

merged 2 commits into from
May 4, 2021

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jan 1, 2021 •
edited by bedevere-bot
Loading

@erlend-aasland erlend-aasland requested a review from a team as a code owner January 1, 2021 20:18
@erlend-aasland
Copy link
Contributor Author

@zooba Could you have a look a this? I won't have any effect until we upgrade to SQLite 3.35.0, but it won't do any harm applying it now.

@github-actions
Copy link

github-actions bot commented Feb 7, 2021

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 7, 2021
@erlend-aasland
Copy link
Contributor Author

Is there anyone else from the Windows team who can review this?

pfmoore
pfmoore approved these changes Feb 11, 2021
View reviewed changes
Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

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

I can confirm from a quick scan that this looks OK. I haven't built Python with this PR, though, so that's all I can say, sorry.

@erlend-aasland
Copy link
Contributor Author

I can confirm from a quick scan that this looks OK. I haven't built Python with this PR, though, so that's all I can say, sorry.

Thanks, Paul. The CI should fail for win32 and win64 if it didn't build, right?

@pfmoore
Copy link
Member

pfmoore commented Feb 11, 2021

I'd assume so, yes.

@zware
Copy link
Member

zware commented Feb 11, 2021

LGTM as well, but I do think we want to hold off on this until we are in fact using a version that it affects. If we merge this before then, we're giving the impression that the functionality is there when it actually isn't.

You could get fancy and include a version check, but that may be more trouble than it's worth.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Feb 11, 2021 •
edited
Loading

LGTM as well, but I do think we want to hold off on this until we are in fact using a version that it affects. If we merge this before then, we're giving the impression that the functionality is there when it actually isn't.

That depends on how we phrase the NEWS item, I guess. However, adding this now will make it slightly easier to try out the current nightly SQLite builds. OTOH, those who do so are very likely to just add the compile definition themselves, so I guess the real gain is negligible. I'm fine with holding off until 3.35.0. We can include this when we're upgrading the Windows build to 3.35.0.

UPDATE: The Windows installer will be updated in #25641

You could get fancy and include a version check, but that may be more trouble than it's worth.

Totally agree.

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Feb 12, 2021
@erlend-aasland
Copy link
Contributor Author

FYI, rebased onto master bco. #24797.

@erlend-aasland
Copy link
Contributor Author

@zooba: now that #25641 is merged, would you mind merging this as well? It's already approved by members of the Windows team.

@ambv ambv added the needs backport to 3.10 only security fixes label May 4, 2021
@ambv ambv merged commit b451bc8 into python:main May 4, 2021
@miss-islington
Copy link
Contributor

Thanks @erlend-aasland for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 4, 2021
@miss-islington
bpo-42686: Enable SQLite math functions in Windows build (pythonGH-24053
8ecf3c6 
)

(cherry picked from commit b451bc8)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@erlend-aasland erlend-aasland deleted the bpo-42686 branch May 4, 2021 13:09
ambv pushed a commit that referenced this pull request May 4, 2021
@miss-islington
bpo-42686: Enable SQLite math functions in Windows build (GH-24053) (#…
4bb0a81 
…25892)

(cherry picked from commit b451bc8)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@pablogsal pablogsal mentioned this pull request Jul 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pfmoore pfmoore pfmoore approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@erlend-aasland @pfmoore @zware @miss-islington @ambv @the-knights-who-say-ni