GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-115119: Switch Windows build to mpdecimal external by zware · Pull Request #115182 · python/cpython

Conversation

@zware

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Unvendor libmpdec sources #115119

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zooba and erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@zware
zware requested review from a team and rhettinger as code owners February 8, 2024 20:49
@bedevere-app bedevere-app Bot added the awaiting core review label Feb 8, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Feb 8, 2024
Unvendor libmpdec sources #115119
Closed
15 tasks done
@zware
pythongh-115119: Switch Windows build to mpdecimal external
7fc2f57
@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
@bedevere-app bedevere-app Bot removed the awaiting core review label Feb 8, 2024
@rhettinger
rhettinger removed their request for review February 8, 2024 21:19
@zware

zware commented Feb 8, 2024

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zware added 2 commits February 25, 2024 14:25
@zware
Add mpdecimal.h selection shim
0b71ce2
@zware
Define BUILD_LIBMPDEC
c128618
@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
@bedevere-app bedevere-app Bot added the awaiting core review label Feb 25, 2024
@zware

zware commented Feb 25, 2024

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zware

zware commented Feb 25, 2024

Copy link
Copy Markdown
Member Author

!buildbot Windows
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Feb 25, 2024

Copy link
Copy Markdown

🤖 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zooba
zooba reviewed Feb 26, 2024
View reviewed changes
Comment thread PCbuild/_decimal.vcxproj Resolved

Uh oh!

There was an error while loading. Please reload this page.
zware added 2 commits March 15, 2024 14:54
@zware
Merge branch 'main' into add_mpdecimal_external
7cc95d8
@zware
Add mpdecimal to external.spdx.json
0fce72d
@zware
zware requested a review from sethmlarson as a code owner March 15, 2024 21:08
@zooba

zooba commented Mar 18, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zware

zware commented Mar 18, 2024

Copy link
Copy Markdown
Member Author

The limitation of 24h/day, mostly :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zware
zware merged commit 849e071 into python:main Mar 18, 2024
@zware
zware deleted the add_mpdecimal_external branch March 18, 2024 17:07
@bedevere-app bedevere-app Bot removed the awaiting core review label Mar 18, 2024
@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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@zware @bedevere-bot @zooba
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.