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-43312: Functions returning default and preferred sysconfig schemes by uranusjr · Pull Request #24644 · python/cpython

Conversation

@uranusjr

@uranusjr uranusjr commented Feb 25, 2021 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue43312

Sorry, something went wrong.

Uh oh!

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

barneygale, gaborbernat, and henryiii reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 25, 2021
@bedevere-bot bedevere-bot added the awaiting review label Feb 25, 2021
@github-actions

github-actions Bot commented Mar 28, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

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

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Mar 28, 2021
@uranusjr
Make default scheme function public
a0d613f
@uranusjr
uranusjr force-pushed the sysconfig-preferred-scheme branch from acadc40 to a0d613f Compare March 29, 2021 20:17
@uranusjr
uranusjr marked this pull request as ready for review March 29, 2021 21:36
@uranusjr

uranusjr commented Mar 29, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Uh, how do I remove the stale label?

All reactions

Sorry, something went wrong.

Uh oh!

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

@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Mar 30, 2021
pfmoore
pfmoore approved these changes Apr 6, 2021
View reviewed changes

@pfmoore pfmoore 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.

I can't say whether the choices in _get_preferred_schemes are correct, but they seem reasonable. Unless there's anything we can check them against, I'd say we should go with them.

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Apr 6, 2021
gaborbernat
gaborbernat reviewed Apr 7, 2021
View reviewed changes
Comment thread Lib/sysconfig.py


def get_paths(scheme=_get_default_scheme(), vars=None, expand=True):
def get_paths(scheme=get_default_scheme(), vars=None, expand=True):

@gaborbernat gaborbernat Apr 7, 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.

Do we need to call this function every time? Wouldn't be better to call it just once and save it into a private global variable, and use that as the default value for all these methods?

Sorry, something went wrong.

Uh oh!

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

All reactions

@uranusjr uranusjr Apr 7, 2021

Copy link
Copy Markdown
Contributor 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.

That’s a separate issue already presented in the previous implementation. I’m not sure this is the best plac eto discuss the possible implications in e.g. multiprocessing settings (I don’t even have enough expertise to comment on it).

As it currently stands, the function is only called twice (get_paths and get_path) on import time, so the performance difference is pretty close to none anyway.

Sorry, something went wrong.

Uh oh!

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

hroncok reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@pfmoore pfmoore Apr 7, 2021

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.

Agreed, let's keep the change to the minimal required.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/sysconfig.py Outdated
print('Platform: "%s"' % get_platform())
print('Python version: "%s"' % get_python_version())
print('Current installation scheme: "%s"' % _get_default_scheme())
print('Current installation scheme: "%s"' % get_default_scheme())

@gaborbernat gaborbernat Apr 7, 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.

Would be a good opportunity to migrate this to f-strings?

Sorry, something went wrong.

Uh oh!

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

All reactions

@uranusjr uranusjr Apr 7, 2021

Copy link
Copy Markdown
Contributor 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.

Probably, I’ll combine this to other needed changes, if there are any.

Sorry, something went wrong.

Uh oh!

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

All reactions
hroncok
hroncok reviewed Apr 14, 2021
View reviewed changes

@hroncok hroncok 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.

I ❤️ it!

Several non-blocking questions attached.

However, I have a very strong suggestion: Please add tests.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/sysconfig.py Outdated
Comment on lines +244 to +245
if scheme not in _INSTALL_SCHEMES:
raise KeyError(key)

@hroncok hroncok Apr 14, 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.

Will the above lookup not raise already? Should this be done before calling _get_preferred_schemes?

Sorry, something went wrong.

Uh oh!

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

All reactions

@uranusjr uranusjr Apr 14, 2021

Copy link
Copy Markdown
Contributor 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.

The main intention is to guard against incorrect implementation, since _get_preferred_schemes() is expected to be modified by redistributors and implementers. Say an implementer makes a typo:

def _get_preferred_schemes():
    return {
        "prefix": "posix_prefix",
        "home": "posix_home",
        "user": "posix_uesr",  # Oops!
    }

And when the value is later used:

scheme = sysconfig.get_preferred_scheme("user")

...  # Much later...

paths = sysconfig.get_paths(scheme)

This check would catch that posix_uesr is not actually a valid scheme and error early, instead of later when the user actually tries to use the (invalid) scheme.

Sorry, something went wrong.

Uh oh!

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

hroncok reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@zooba zooba Apr 19, 2021

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.

Can we raise a ValueError with a better message then? "Key '{}' returned scheme '{}' which is not valid on this platform" (or something more correct)

Sorry, something went wrong.

Uh oh!

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

All reactions

@uranusjr uranusjr Apr 19, 2021

Copy link
Copy Markdown
Contributor 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.

I couldn’t think of a better message and implemented yours for now, any ideas are very welcomed. Ideally I want to point users to the implementer/redistributor instead of reporting the error to CPython, but that’s probably not really feasible.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/sysconfig.py Resolved

Uh oh!

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

uranusjr added 2 commits April 15, 2021 00:09
@uranusjr
Make get_default_scheme call get_preferred_scheme
1f15613
This reduces logic duplication with a slight performance tradeoff.
@uranusjr
Add tests for scheme-related sysconfig functions
122bac5
@uranusjr

uranusjr commented Apr 14, 2021

Copy link
Copy Markdown
Contributor Author

I’ve changed get_default_scheme() to use get_preferred_scheme(), and a couple of simple tests for the new functions.

All reactions

Sorry, something went wrong.

Uh oh!

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

@frenzymadness

frenzymadness commented Apr 15, 2021

Copy link
Copy Markdown
Contributor

The implementation looks good to me.

All reactions

Sorry, something went wrong.

Uh oh!

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

@uranusjr

uranusjr commented Apr 15, 2021

Copy link
Copy Markdown
Contributor Author

I’m wondering whether we should add documentation for the private _get_preferred_schemes() as well, to tell redistributors this is the function they’re advised to override/modify.

All reactions

Sorry, something went wrong.

Uh oh!

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

@encukou

encukou commented Apr 16, 2021

Copy link
Copy Markdown
Member

I’m wondering whether we should add documentation for the private _get_preferred_schemes() as well, to tell redistributors this is the function they’re advised to override/modify.

At least a docstring would be great, yes.

All reactions

Sorry, something went wrong.

Uh oh!

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

@uranusjr
Add documentation entry for Python redistributors
01ffb68
@uranusjr

uranusjr commented Apr 17, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Documentation entry added 👍


Edit: Plus converting %-formatting to f-strings.

All reactions

Sorry, something went wrong.

Uh oh!

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

@uranusjr
Convert string formatting to use f-strings
fd46323
Drive-by refactoring.
@hroncok

hroncok commented Apr 19, 2021

Copy link
Copy Markdown
Contributor

Curiosity question: Should distutils use this?

All reactions

Sorry, something went wrong.

Uh oh!

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

@uranusjr

uranusjr commented Apr 19, 2021

Copy link
Copy Markdown
Contributor Author

Ideally yes, but distutils’s implementation around this is structured very differently, and tangled with a lot of things inside distutils.command.install, I’m not sure if it’s really worthwhile (or even doable without risking behavioural changes) 😞

All reactions

Sorry, something went wrong.

Uh oh!

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

zooba
zooba reviewed Apr 19, 2021
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.

Would like to see that error message improved, but the rest seems fine to me. I'm just taking on trust that it's sufficient for distros to override what they need, because I don't know that side of it well enough.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/sysconfig.py Outdated
Comment on lines +244 to +245
if scheme not in _INSTALL_SCHEMES:
raise KeyError(key)

@zooba zooba Apr 19, 2021

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.

Can we raise a ValueError with a better message then? "Key '{}' returned scheme '{}' which is not valid on this platform" (or something more correct)

Sorry, something went wrong.

Uh oh!

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

All reactions
@uranusjr
Better error message on invalid scheme
1425a92
@uranusjr

uranusjr commented Apr 27, 2021

Copy link
Copy Markdown
Contributor Author

Another nudge 🙂

All reactions

Sorry, something went wrong.

Uh oh!

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

@pfmoore

pfmoore commented Apr 27, 2021

Copy link
Copy Markdown
Member

I believe all comments have been addressed, and I see no remaining concerns raised and general approval of the change, so I'm going to merge this based on my approval above.

uranusjr reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

@pfmoore
pfmoore merged commit d925133 into python:master Apr 27, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label Apr 27, 2021
@uranusjr
uranusjr deleted the sysconfig-preferred-scheme branch April 27, 2021 08:46
@carlbordum carlbordum mentioned this pull request May 27, 2022
bpo-37592: sysconfig no longer relies on sys.version #14776
Closed
@mattip mattip mentioned this pull request Nov 16, 2022
sysconfig should not rely on sys.version #81773
Closed
@FFY00 FFY00 mentioned this pull request Dec 13, 2022
sysconfig.get_default_scheme versionadded vs versionchanged #100206
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pfmoore pfmoore pfmoore approved these changes
@zooba zooba zooba left review comments
+2 more reviewers
@gaborbernat gaborbernat gaborbernat left review comments
@hroncok hroncok hroncok left review comments
Reviewers whose approvals may not affect merge requirements

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.

9 participants

@uranusjr @frenzymadness @encukou @hroncok @pfmoore @gaborbernat @zooba @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.