Skip to content

gh-98718: re-use already calculated is_python_build from getpath #98719

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

FFY00
Copy link
Member

@FFY00 FFY00 commented Oct 26, 2022

This also gets rid of the duplicated logic.

@FFY00 FFY00 requested review from jaraco and zooba October 26, 2022 13:19
@FFY00 FFY00 changed the title bpo-98718: re-use already calculated is_python_build from getpath gh-98718: re-use already calculated is_python_build from getpath Oct 26, 2022
@FFY00 FFY00 force-pushed the bpo-98718-is_python_build branch from 665bcf7 to 080a70a Compare October 26, 2022 13:21
@zooba
Copy link
Member

zooba commented Oct 26, 2022

Seems fine to me, though I assume some others have more opinions about tacking stuff onto the sys module. I think it's fine.

@FFY00
Copy link
Member Author

FFY00 commented Oct 26, 2022

I think the biggest win here is really unifying the logic in one place.

@serhiy-storchaka
Copy link
Member

Does not it need documentation?

Would not it be more useful to expose build_prefix (None or not set if is not Python ) rather of simple boolean _is_python_build?

@jaraco
Copy link
Member

jaraco commented Nov 6, 2022

Does not it need documentation?

As proposed, I see it as an internal implementation detail, so wouldn't be documented.

Would not it be more useful to expose build_prefix (None or not set if is not Python ) rather of simple boolean _is_python_build?

+1. is_python_build() could still return bool(sys._build_prefix).

Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm unsure about the correctness. Is sys._is_python_build always a correct indicator at runtime? How can we have more confidence it's the correct value in different scenarios?

@zooba
Copy link
Member

zooba commented Nov 7, 2022

Is sys._is_python_build always a correct indicator at runtime?

Nope, but we don't have a proper one. Any marker file could disappear or be copied, so we may as well look for the things we actually care about (stdlib modules) and guess, rather than look for something that's there solely to help us guess.

At least if we expose it properly, it'll be easier to change it to something more reliable if we come up with one.

Signed-off-by: Filipe Laíns <lains@riseup.net>
@FFY00
Copy link
Member Author

FFY00 commented Nov 9, 2022

Does not it need documentation?

It's missing a news entry, but other than that, no, it's an internal change.

Would not it be more useful to expose build_prefix (None or not set if is not Python ) rather of simple boolean _is_python_build?

We can do that. The reason I didn't do it here was because I didn't want to add any more public API, so I just used the already exported _is_build_prefix. I opened #99273 following this suggestion.

@FFY00 FFY00 force-pushed the bpo-98718-is_python_build branch from 080a70a to eaa03ca Compare August 8, 2023 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants