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-22257: Private C-API for core runtime initialization (PEP 432). by ericsnowcurrently · Pull Request #1772 · python/cpython

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented May 23, 2017

Copy link
Copy Markdown
Member

(Patch by Nick Coghlan / @ncoghlan)

Mostly this adds _PyCoreConfig/_Py_InitializeCore() and reorganizes interpreter initialization code around them. The changes align with PEP 432. The new functions are introduced here as "private" C-API while the PEP is discussed. Regardless of the status of the PEP, the changes here are an improvement to interpreter startup. As such we are making the changes now, but keeping the C-API private.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mention-bot

mention-bot commented May 23, 2017

Copy link
Copy Markdown

@ericsnowcurrently, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zooba, @ncoghlan and @birkenfeld to be potential reviewers.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 23, 2017
@ericsnowcurrently ericsnowcurrently mentioned this pull request May 23, 2017
bpo-22257: Private C-API for main interpreter initialization (PEP 432). #1729
Merged
@ericsnowcurrently

ericsnowcurrently commented May 23, 2017

Copy link
Copy Markdown
Member Author

@zooba
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently
ericsnowcurrently force-pushed the pep-432-core-config branch from 2caf35c to 6573e10 Compare May 23, 2017 22:56
zooba
zooba approved these changes May 23, 2017
View reviewed changes

@zooba zooba left a comment

Copy link
Copy Markdown
Member

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.

LGTM with the fixes mentioned

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/pylifecycle.c Outdated

@zooba zooba May 23, 2017

Copy link
Copy Markdown
Member

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.

Does this need to stay? Should we have a build option to reenable it? Or a comment explaining it?

Sorry, something went wrong.

Uh oh!

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

@ncoghlan ncoghlan May 24, 2017

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.

+1 for dropping it (this is brute force debugging hackery left over from when the modified interpreter was crashing on startup)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/pylifecycle.c Outdated

@zooba zooba May 23, 2017

Copy link
Copy Markdown
Member

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.

What's with the change in this section?

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently May 24, 2017

Copy link
Copy Markdown
Member Author

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.

Not sure. Given the comment I expect it wasn't intentional.

Sorry, something went wrong.

Uh oh!

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

@ncoghlan ncoghlan May 24, 2017

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.

Almost certainly the result of a merge glitch somewhere along the line when I was still maintaining the patch out of tree.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/pylifecycle.c Outdated

@zooba zooba May 23, 2017

Copy link
Copy Markdown
Member

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.

if (!interp)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/pylifecycle.c Outdated

@zooba zooba May 23, 2017

Copy link
Copy Markdown
Member

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 where we'd trigger the extra initialization step, right?

Also, why not put install_sigs into core_config?

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently May 24, 2017

Copy link
Copy Markdown
Member Author

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.

Both of those are sorted out in the next PR.

Sorry, something went wrong.

Uh oh!

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

@ncoghlan ncoghlan May 24, 2017

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.

But for anyone just reading these comments: "install_sigs" becomes the first field in the new config struct for the main interpreter, rather than being needed to get the core runtime going :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ncoghlan and others added 2 commits May 23, 2017 17:03
@ncoghlan @ericsnowcurrently
Add _PyCoreConfig and _Py_InitializeCore().
6438bf5
@ericsnowcurrently
Set up memory allocator before initializing runtime.
6deccd5
@ericsnowcurrently
ericsnowcurrently force-pushed the pep-432-core-config branch from 6573e10 to 6deccd5 Compare May 24, 2017 00:03
ericsnowcurrently added 3 commits May 23, 2017 20:50
@ericsnowcurrently
Drop _INIT_DEBUG_PRINT.
5fc4f82
@ericsnowcurrently
Fix a typo.
2eecfd8
@ericsnowcurrently
Restore the _PyEval_FiniThreads() call.
7d6cc25
@ericsnowcurrently
ericsnowcurrently merged commit 1abcf67 into python:master May 24, 2017
@ericsnowcurrently
ericsnowcurrently deleted the pep-432-core-config branch May 24, 2017 04:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ncoghlan ncoghlan ncoghlan left review comments
@zooba zooba zooba 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.

5 participants

@ericsnowcurrently @mention-bot @ncoghlan @zooba @the-knights-who-say-ni
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 01402520 — 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.