Skip to content

bpo-45569: Change PYLONG_BITS_IN_DIGIT default to 30 #30497

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.

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

Merged
merged 8 commits into from
Jan 14, 2022

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Jan 9, 2022

This PR changes the default for PYLONG_BITS_IN_DIGIT to 30, unconditionally. 15-bit digits can still be selected explicitly via configure (Unix-like) or in pyconfig.h (Windows).

https://bugs.python.org/issue45569

@mdickinson mdickinson added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 9, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit 515c0e8 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 9, 2022
@mdickinson
Copy link
Member Author

There were 5 buildbot failures, none of which appears to be directly related to this PR. Most of them seem to be due to an enum module refleak, which has since been corrected (see https://bugs.python.org/issue46301). The ARM64/macOS PR is failing due to test_ftplib changing the environment.

I'll re-do the buildbot run, in the hope of seeing the refleak-related failures disappear.

@mdickinson mdickinson added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 12, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit 515c0e8 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 12, 2022
@mdickinson mdickinson added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 13, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit 8a761bf 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 13, 2022
@mdickinson
Copy link
Member Author

@tiran Would you have bandwidth to do a sanity check on this PR?

@mdickinson
Copy link
Member Author

Hmm. I need to update the wording in the configure script help, too. I'll do that.

Comment on lines +4 to +5
to the ``configure`` script, or by defining ``PYLONG_BITS_IN_DIGIT`` in
``pyconfig.h``.
Copy link
Member

Choose a reason for hiding this comment

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

pyconfig.h is auto-generated and not user-editable. I suggest to remove the qualifier.

Suggested change
to the ``configure`` script, or by defining ``PYLONG_BITS_IN_DIGIT`` in
``pyconfig.h``.
to the ``configure`` script, or by defining ``PYLONG_BITS_IN_DIGIT``.

Copy link
Member Author

Choose a reason for hiding this comment

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

True; I was thinking of the Windows setup. Maybe replacing pyconfig.h with PC/pyconfig.h would be enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

OTOH, probably not worth worrying about explicitly specifying how to change things for Windows here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Clarified (I hope).

@@ -5039,7 +5039,7 @@ AC_CHECK_DECLS([RTLD_LAZY, RTLD_NOW, RTLD_GLOBAL, RTLD_LOCAL, RTLD_NODELETE, RTL
# determine what size digit to use for Python's longs
AC_MSG_CHECKING([digit size for Python's longs])
AC_ARG_ENABLE(big-digits,
AS_HELP_STRING([--enable-big-digits@<:@=15|30@:>@],[use big digits (30 or 15 bits) for Python longs (default is system-dependent)]]),
Copy link
Member

Choose a reason for hiding this comment

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

You have to update configure, too. It's a trivial change, so manually editing is easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I've spent the last hour trying to get autoreconf to not generate spurious changes. :-( I'll go for the manual edit option.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I've spent the last hour trying to get autoreconf to not generate spurious changes. :-( I'll go for the manual edit option.

I have you covered: https://quay.io/repository/tiran/cpython_autoconf

@tiran tiran added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 13, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @tiran for commit 34dcb2c 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 13, 2022
@mdickinson
Copy link
Member Author

Thanks, @tiran!

@mdickinson mdickinson merged commit 025cbe7 into python:main Jan 14, 2022
@mdickinson mdickinson deleted the default-to-30-bit-pylong-digits branch January 14, 2022 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants