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.

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

gh-115119: Switch Windows build to mpdecimal external #115182

Merged
merged 5 commits into from
Mar 18, 2024
Merged

gh-115119: Switch Windows build to mpdecimal external #115182

merged 5 commits into from
Mar 18, 2024

Conversation

zware
Copy link
Member

@zware zware commented Feb 8, 2024 •
edited by bedevere-app bot
Loading

@zware zware requested review from rhettinger and a team as code owners February 8, 2024 20:49
@bedevere-app bedevere-app bot mentioned this pull request Feb 8, 2024
Open
15 tasks
@zware zware force-pushed the add_mpdecimal_external branch from 3cfe39b to 7fc2f57 Compare February 8, 2024 20:50
@zware zware marked this pull request as draft February 8, 2024 21:14
@rhettinger rhettinger removed their request for review February 8, 2024 21:19
@zware
Copy link
Member Author

zware commented Feb 8, 2024

I missed that mpdecimal.h does not actually exist in the source distribution, but is created (or copied) by the building of the library. This will need to be handled somehow, either by adding a Windows-specific shim that picks the right mpdecimal{32,64}vc.h to include somewhere, or by building the library separately using its own build scripts.

@zware zware force-pushed the add_mpdecimal_external branch from 22bfcc4 to c128618 Compare February 25, 2024 20:25
@zware zware marked this pull request as ready for review February 25, 2024 22:01
@zware
Copy link
Member Author

zware commented Feb 25, 2024

Made it back around to this and fixed the build issue. I don't like the added Modules/_decimal/windows/mpdecimal.h as a permanent solution, but I think it's a decent stopgap to avoid changing the sources somehow. Ideally we'll eventually switch to using mpdecimal's own PGO build script to produce binaries, but that will be a bigger change that we can make happen later. It will also require some attention on ARM, unless 4.0.0 already has handling for that.

@zware
Copy link
Member Author

zware commented Feb 25, 2024

!buildbot Windows

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @zware for commit c128618 🤖

The command will test the builders whose names match following regular expression: Windows

The builders matched are:

  • AMD64 Windows11 Bigmem PR
  • AMD64 Windows11 Refleaks PR
  • ARM64 Windows Non-Debug PR
  • AMD64 Windows10 PR
  • ARM64 Windows PR
  • AMD64 Windows11 Non-Debug PR
  • AMD64 Windows Server 2022 NoGIL PR

@zware zware requested a review from sethmlarson as a code owner March 15, 2024 21:08
@zooba
Copy link
Member

zooba commented Mar 18, 2024

@zware This looks like it's good enough shape - any reason to not merge yet?

@zware
Copy link
Member Author

zware commented Mar 18, 2024

The limitation of 24h/day, mostly :)

@zware zware merged commit 849e071 into python:main Mar 18, 2024
33 of 34 checks passed
@zware zware deleted the add_mpdecimal_external branch March 18, 2024 17:07
@bedevere-bot

This comment was marked as off-topic.

Sign in to view
vstinner pushed a commit to vstinner/cpython that referenced this pull request Mar 20, 2024
@zware @vstinner
pythongh-115119: Switch Windows build to mpdecimal external (pythonGH…
81a50ac 
…-115182)

This includes adding what should be a relatively temporary
`Modules/_decimal/windows/mpdecimal.h` shim to choose between `mpdecimal32vc.h`
or `mpdecimal64vc.h` based on which of `CONFIG_64` or `CONFIG_32` is defined.
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
@zware @adorilson
pythongh-115119: Switch Windows build to mpdecimal external (pythonGH…
d0f7804 
…-115182)

This includes adding what should be a relatively temporary
`Modules/_decimal/windows/mpdecimal.h` shim to choose between `mpdecimal32vc.h`
or `mpdecimal64vc.h` based on which of `CONFIG_64` or `CONFIG_32` is defined.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@zware @diegorusso
pythongh-115119: Switch Windows build to mpdecimal external (pythonGH…
047a994 
…-115182)

This includes adding what should be a relatively temporary
`Modules/_decimal/windows/mpdecimal.h` shim to choose between `mpdecimal32vc.h`
or `mpdecimal64vc.h` based on which of `CONFIG_64` or `CONFIG_32` is defined.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zooba zooba zooba left review comments

@sethmlarson sethmlarson Awaiting requested review from sethmlarson sethmlarson is a code owner

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

Successfully merging this pull request may close these issues.
3 participants
@zware @bedevere-bot @zooba