Skip to content
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

gh-106718: Respect PyConfig.stdlib_dir in getpath. #108730

Merged
merged 6 commits into from
Nov 1, 2023
Merged

Conversation

yilei
Copy link
Contributor

@yilei yilei commented Aug 31, 2023

@yilei
Copy link
Contributor Author

yilei commented Aug 31, 2023

(I'm looking at the test_embed failure)

@yilei
Copy link
Contributor Author

yilei commented Aug 31, 2023

Tests passed!

else:
stdlib_dir = joinpath(build_prefix, 'Lib')
# Use the build prefix for stdlib when not explicitly set
if not stdlib_dir_was_set_in_config:
Copy link
Member

Choose a reason for hiding this comment

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

stdlib_dir isn't set by this point unless it was in the config, so we can just check that it's empty and set it if so.

Later on there are more checks that may overwrite stdlib_dir without checking. Those also ought to check that it's empty.

stdlib_dir_was_set_in_config shouldn't be needed, I don't think. But it might be (if e.g. we need one of the later stdlib_dir assignments to overwrite an earlier one if it were set from a build directory, but that shouldn't be the case).

Copy link
Contributor Author

@yilei yilei Sep 6, 2023

Choose a reason for hiding this comment

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

Thanks for the review!

Before making a change, a question:

I added stdlib_dir_was_set_in_config for this case:

stdlib_dir = None
as it was unconditionally setting stdlib_dir to None. At this point, stdlib_dir might be already set either in the config or some conditions above. So stdlib_dir_was_set_in_config is needed I think.

And given stdlib_dir_was_set_in_config is needed here, do you prefer using stdlib_dir_was_set_in_config everywhere, or check if stdlib_dir is empty in some cases?

Copy link
Member

Choose a reason for hiding this comment

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

You'll need to determine whether setting home should override other parts of the config. Historically, I believe that's what it has done, and so arguably it should continue to do it. Or maybe it should change? But that will need to be a deliberate (and documented) change of behaviour.

Any changes made to this script are incredibly fragile. Hopefully if this change goes in then we'll have all of the 3.13 prereleases to find out who it broke, but it would be best to figure out which change is least breaking, and be very explicit about which scenarios will change. Even then, we're basically guaranteed to miss some things.

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense for more specific settings to override more general ones. 'home' sets a bunch of things, one of which is stdlib_dir, so setting stdlib_dir explicitly should override home. (If the user doesn't want stdlib_dir to override home, they can just not set stdlib_dir if they're setting home, after all).

As for stdlib_dir_was_set_in_config, can't you just test if stdlib_dir is None instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I reworded the changelog to say "When PyConfig.stdlib_dir is explicitly set, it's now respected and won't be overridden by PyConfig.home."

For stdlib_dir_was_set_in_config, I can test if stdlib_dir is None here. But below when home is set, it will override the stdlib_dir found by the build directory here. And I don't want to change this behavior so stdlib_dir_was_set_in_config is necessary there (similarly the two places below). Since there is stdlib_dir_was_set_in_config, I'm using it everywhere. Let me know if you prefer testing test if stdlib_dir is None here.

@gpshead gpshead requested a review from Yhg1s September 6, 2023 20:20
else:
stdlib_dir = joinpath(build_prefix, 'Lib')
# Use the build prefix for stdlib when not explicitly set
if not stdlib_dir_was_set_in_config:
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense for more specific settings to override more general ones. 'home' sets a bunch of things, one of which is stdlib_dir, so setting stdlib_dir explicitly should override home. (If the user doesn't want stdlib_dir to override home, they can just not set stdlib_dir if they're setting home, after all).

As for stdlib_dir_was_set_in_config, can't you just test if stdlib_dir is None instead?

Modules/getpath.py Show resolved Hide resolved
@yilei yilei requested review from Yhg1s and zooba September 11, 2023 19:56
@yilei
Copy link
Contributor Author

yilei commented Sep 21, 2023

Any further comments? (The Windows x64 failure seems to be a flake / timeout.)

@zooba zooba merged commit 834b7c1 into python:main Nov 1, 2023
27 checks passed
@zooba
Copy link
Member

zooba commented Nov 1, 2023

Done. We've got plenty of prereleases to find out what goes wrong from here (hopefully nothing, but I've hoped that for every single other getpath change and have been wrong every time 😄 )

FullteaR pushed a commit to FullteaR/cpython that referenced this pull request Nov 3, 2023
@yilei yilei deleted the stdlib_dir branch December 13, 2023 21:09
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
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.

None yet

5 participants