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 — bpo-45573: Move mandatory core modules to Modules/Setup.bootstrap (GH-29616) by tiran · Pull Request #29616 · python/cpython

Conversation

@tiran

@tiran tiran commented Nov 18, 2021 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Signed-off-by: Christian Heimes [email protected]

https://bugs.python.org/issue45573

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 18, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Nov 18, 2021
@tiran
tiran force-pushed the bpo-45573-setup-core branch 2 times, most recently from be740b2 to 0db8a04 Compare November 18, 2021 13:37
@tiran
tiran marked this pull request as ready for review November 18, 2021 15:34
@tiran
tiran force-pushed the bpo-45573-setup-core branch from 0db8a04 to 05c1b88 Compare November 18, 2021 16:11
erlend-aasland
erlend-aasland requested changes Nov 18, 2021
View reviewed changes

@erlend-aasland erlend-aasland left a comment •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is a nice improvement. Not all modules that #define Py_BUILD_CORE_MODULE are included in Modules/Setup.core. Is Modules/Setup.builtin a more "correct" name?

Modules that define Py_BUILD_CORE_MODULE 
$ grep -r "#.*define.*Py_BUILD_CORE_MODULE" Modules | sort
Modules/_abc.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_asynciomodule.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_blake2/blake2b_impl.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_blake2/blake2module.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_blake2/blake2s_impl.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_ctypes/_ctypes.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_ctypes/callbacks.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_ctypes/cfield.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_ctypes/stgdict.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_cursesmodule.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_datetimemodule.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_decimal/_decimal.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_hashopenssl.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_heapqmodule.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_json.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_lsprof.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_pickle.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_posixsubprocess.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_queuemodule.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_randommodule.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_sha3/sha3module.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_struct.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_testinternalcapi.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_testmultiphase.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_xxsubinterpretersmodule.c:#  define Py_BUILD_CORE_MODULE 1
Modules/_zoneinfo.c:#  define Py_BUILD_CORE_MODULE 1
Modules/arraymodule.c:#  define Py_BUILD_CORE_MODULE 1
Modules/binascii.c:#  define Py_BUILD_CORE_MODULE 1
Modules/cmathmodule.c:#  define Py_BUILD_CORE_MODULE 1
Modules/mathmodule.c:#  define Py_BUILD_CORE_MODULE 1
Modules/md5module.c:#  define Py_BUILD_CORE_MODULE 1
Modules/sha1module.c:#  define Py_BUILD_CORE_MODULE 1
Modules/sha256module.c:#  define Py_BUILD_CORE_MODULE 1
Modules/sha512module.c:#  define Py_BUILD_CORE_MODULE 1
Modules/unicodedata.c:#  define Py_BUILD_CORE_MODULE 1

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/Setup.core Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/Setup.core Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread configure.ac Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread configure.ac Outdated
Comment on lines 6008 to 6009

@erlend-aasland erlend-aasland Nov 18, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why the alias?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread setup.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread setup.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread setup.py Resolved

Uh oh!

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

tiran commented Nov 19, 2021

Copy link
Copy Markdown
Member Author

This is a nice improvement. Not all modules that #define Py_BUILD_CORE_MODULE are included in Modules/Setup.core. Is Modules/Setup.builtin a more "correct" name?

The term core is overloaded. I have not found a better term, though.

Py_BUILD_CORE_MODULE enables some private APIs that not available to 3rd party extension modules. The flag is orthogonal to static / shared state. You can build a shared C extension that uses core module APIs.

Setup.builtin does not fit well either. Other modules can be build as builtin modules, too. Setup.mandatory is also incorrect. Some modules like _abc could be build as shared extensions. Maybe Setup.bootstrap ?
erlend-aasland reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Nov 19, 2021

Copy link
Copy Markdown
Contributor

Yeah, Setup.bootstrap sounds like the best candidate.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tiran tiran changed the title bpo-45573: Move mandatory core modules to Modules/Setup.core bpo-45573: Move mandatory core modules to Modules/Setup.bootstrap Nov 19, 2021
@tiran
tiran force-pushed the bpo-45573-setup-core branch from 05c1b88 to a1b915f Compare November 19, 2021 08:50
erlend-aasland
erlend-aasland reviewed Nov 19, 2021
View reviewed changes
Comment thread setup.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
tiran added 5 commits November 19, 2021 15:19
@tiran
bpo-45573: Move mandatory core modules to Modules/Setup.core
a613d44 
Signed-off-by: Christian Heimes <[email protected]>
@tiran
Use Setup.bootstrap
8c29e52
@tiran
Fix comment
e11c9b6
@tiran
Single quotes don't work
e749a53
@tiran
Consolidate comments about extensions in Setup.bootstrap
db3e45e
@tiran
tiran force-pushed the bpo-45573-setup-core branch from ff2d1e0 to db3e45e Compare November 19, 2021 15:01
erlend-aasland
erlend-aasland approved these changes Nov 19, 2021
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Great! LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tiran reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@tiran tiran changed the title bpo-45573: Move mandatory core modules to Modules/Setup.bootstrap bpo-45573: Move mandatory core modules to Modules/Setup.bootstrap (GH-29616) Nov 19, 2021
@tiran
tiran merged commit 7e44dc0 into python:main Nov 19, 2021
@tiran
tiran deleted the bpo-45573-setup-core branch November 19, 2021 15:40
@bedevere-bot bedevere-bot removed the awaiting core review label Nov 19, 2021
remykarem pushed a commit to remykarem/cpython that referenced this pull request Dec 7, 2021
@tiran remykarem
bpo-45573: Move mandatory core modules to Modules/Setup.bootstrap (py…
1d22328 
…thonGH-29616)
@tiran tiran mentioned this pull request Jan 22, 2023
Use pkg-config autoconf macros to detect flags for Modules/Setup #89736
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland approved these changes

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.

4 participants

@tiran @erlend-aasland @the-knights-who-say-ni @bedevere-bot
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.