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

Fix condition for triggering warning on macOS bogus sysconfig #11741

Closed
wants to merge 1 commit into from

Conversation

dnicolodi
Copy link
Contributor

No description provided.

@dnicolodi
Copy link
Contributor Author

This should be considered only if #11740 is rejected.

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Feb 6, 2023
@dnicolodi
Copy link
Contributor Author

The affected code went away merging #11740. Closing

@dnicolodi dnicolodi closed this Feb 6, 2023
@pradyunsg
Copy link
Member

FWIW, a PR re-adding this warning would be welcome!

@dnicolodi
Copy link
Contributor Author

Because of the issue that this PR was fixing, the warning was never triggered. Therefore we would be adding a warning. And, as I tried to explain in #11740, there is nothing special to warn about. The existing code deals with the installation schema used by Apple's and Homebrew's Python just fine. Actually, being pedantic, the installation paths the code intended to warn about were observed only because of an incorrect usage of the sysconfig API by pip.

I would be happy to prepare a PR adding a warning if one is needed, but someone would need to explain what there is to warn about. My understanding is that now everything works as expected (famous last words...).

@uranusjr
Copy link
Member

The previous conditional is not wrong. The check is against the old (i.e. from distutils) paths, and if they are patched, the warning is emitted to tell the user to report to Apple that they only patched the old path but not the new, and that would cause breakage when we switch to use the new path. Checking against the new path is not meaningful, since the new path is what we are going to use, and Apple patching that means things are going to work.

@dnicolodi
Copy link
Contributor Author

@uranusjr Can you please describe the platform in which you expect the warning to be emitted. I tested on macOS releases from 10.15 to 12 and I was not able to trigger the warning. Also, my understanding of the comment above the check is that that the wrong paths are the one returned by sysconfig, which pip is not going to use, not the other way around.

@pradyunsg
Copy link
Member

pradyunsg commented Feb 13, 2023

What version of XCode command line tools are folks using? I wonder if Apple actually fixed this, between this being added and now.

@uranusjr
Copy link
Member

The Python installation is provided by Xcode; macOS version is irrelevant (assuming Xcode compatibility). I didn’t record the exact Xcode version unfortunately, but the check was added in #10707, and following reports in #10151 should provide some vague timeline.

@dnicolodi
Copy link
Contributor Author

I haven't kept record of the versions of xcode the different systems use.

However, on all tested systems, distutils and sysconfig return different paths, with the default sysconfig scheme being hardcoded to an absolute system path, as the one tested here, which indeed is wrong if you want to install in a prefix. However, as stated multiple times, this at least partially, pip misusing the sysconfig API (ie expecting that the default path contains placeholders that can be substituted via the vars argument).

Anyhow, the logic for raising the warning is wrong. Before this PR is it something like:

def get_isolated_environment_lib_paths()
    sysconfig_paths = get_sysconfig_paths()
    distutils_paths = get_distutils_paths()
    if broken(distutils_paths):
        warn("sysconfig paths are wrong!")
        return distutils_paths

which is wrong for two reasons: if the distutils paths are wrong, why do we use them? Why warning about sysconfig being wrong when we check the values of the distutils paths? Frankly, I'm surprised I need to explain this multiple times, just looking at the code it should be evident that something is off.

@uranusjr
Copy link
Member

I’m thinking though, since Xcode is upgraded quite eagerly, and it’s the only way people can get a fixed Python anyway, the history of how the patch changed is entirely irrelevant and we should only care about the currently available Xcode. So the best thing to do would be to add this fixed check to the current code base since that does the job.

@dnicolodi
Copy link
Contributor Author

@uranusjr The current code deals with this situation just fine. The fix is to use the correct sysconfig schema when looking up the paths. As I already wrote multiple times, it could be argued that pip was using the sysconfig API in the wrong way. There is nothing to warn about. Everything works.

@uranusjr
Copy link
Member

Does Apple patch earlier Python versions to add a venv scheme? That’s the reason why this was an issue, before the scheme was added there was no “correct” config to use.

@dnicolodi
Copy link
Contributor Author

dnicolodi commented Feb 13, 2023

posix_prefix is the right scheme, as used by pip here

if prefix is not None and scheme_name == "osx_framework_library":
scheme_name = "posix_prefix"
This works with Apple's and Homebrew's Python. The venv scheme has been added in Python 3.11 IIRC. Latest xcode is version 14.2 and it still ships Python 3.9. Some distributions path the venv scheme into older Python releases, but pip does not use the venv scheme for prefix installations (and thus isolated build environments) when it is available.

The key thing is to keep in sync the paths used for prefix installations and for setting up the isolated build environments (I already messed this up once, thinking that venv and prefix are equivalent).

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs rebase or merge PR has conflicts with current master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants