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

Pip allows cpxx-none-any tags (and maybe it shouldn't) #10923

Closed
1 task done
tom-bowles opened this issue Feb 25, 2022 · 14 comments
Closed
1 task done

Pip allows cpxx-none-any tags (and maybe it shouldn't) #10923

tom-bowles opened this issue Feb 25, 2022 · 14 comments
Labels
resolution: no action When the resolution is to not do anything type: bug A confirmed bug or unintended behavior

Comments

@tom-bowles
Copy link

Description

Continuing discussion from: pypa/packaging#511

In summary, pip's TargetPython.get_tags() for cpython includes eg cp39-none-any as one of the compatible flags. This is because it always passes the interpreter in when it calls packaging.tags.compatible_tags(). This is inconsistent with packaging.tags.sys_tags(), which doesn't pass the interepeter in unless it's pypy. packaging.tags.sys_tags() therefore doesn't include cp39-none-any.

One practical implication of this is that the pyxll distribution, which has only cpxx-none-any wheels, installs with pip but doesn't with poetry, which uses packaging.tags.sys_tags().

Discussion of this on the packaging issue above suggests this should be considered an issue with pip, and that it shouldn't include these tags. However, please note that this behaviour wasn't introduced recently and it is relied on by, at least, pyxll. Pip versions as old as 20.2 are able to install the pyxll wheels.

Expected behavior

I would expect pip and packaging.tags to agree on which tags are compatible with a given python installation.

pip version

22.0.3

Python version

3.9

OS

Windows

How to Reproduce

  1. pip install pyxll
  2. It works, and arguably shouldn't.

Output

No response

Code of Conduct

@tom-bowles tom-bowles added S: needs triage Issues/PRs that need to be triaged type: bug A confirmed bug or unintended behavior labels Feb 25, 2022
@tom-bowles
Copy link
Author

As requsted by @pradyunsg on pypa/packaging#511, pip debug -v output for pip 20.2 is at https://gist.github.com/tom-bowles/c84442db511c3eb7d73f4b1f775ea67f. It indicates that cp39-none-any was included by that old pip version.

@pfmoore
Copy link
Member

pfmoore commented Feb 25, 2022

I can see no reason why cpyxll needs that tag. It uses ctypes to access various Windows functionality, but I see nothing that would indicate that it is CPython-specific, or even Python version specific.

From my admittedly brief skim of the code in the wheel, I see no reason why it shouldn't be py3-non-any, or maybe py3-none-win32.win_amd64.

As it's a paid for product, and apparently closed source (at least in the sense that there's no public access to the source repository, the Python code is visible in the wheel, obviously), I'd suggest asking on their support channel for an explanation of why they choose to use such an unconventional tag. If they have valid reasons for needing it, those reasons might help clarify why (and how) to add support to pip/packaging/etc.

@tom-bowles
Copy link
Author

tom-bowles commented Feb 25, 2022

As I mentioned on pypa/packaging#511, I suspect the reason for "any" is to do with the fact that they have one wheel that supports both 32 and 64 bit Windows. The reason for "cp" rather than "py" is probably that their Excel addin (which is a separate, non-python installer) embeds the cpython interpreter in Excel so while their wheel may not contain cpython-specific code the mechanism as a whole probably only works with cpython. This is surmising, though.

I'll raise a question with pyxll support if we have the ability to do so, but there's a limit to the extent to which I can or should act as an intermediary between them and the pip devs. I raised the issue not because I need it fixing (I have a workaround) but because I noticed an inconsistency between pypa/pip and pypa/packaging in the treatment of compatibility tags, which seems wrong in principle.

@tom-bowles
Copy link
Author

I didn't know about the dot separated multiple platforms thing, btw. cp39-none-win32.win_amd64 may be the best option for them. It certainly doesn't work on non-Windows platforms. I'll mention that if I raise a support message with them.

@uranusjr
Copy link
Member

uranusjr commented Mar 1, 2022

Coming back to the pip issue, why does pip’s tag list contain cpXX-none-any but packaging does not? I know the lists are built differently, but IIRC packaging also calls the same functions under the hood (otherwise why are those functions there). Does packaging explicitly remove cpXX-none-any when joining the lists? I guess I could just read the source, but I’d happily skip it if anyone can answer these off the top of the head…

@pfmoore
Copy link
Member

pfmoore commented Mar 1, 2022

I think it was mentioned somewhere on one of the threads, but I believe it's because we supply an explicit platform. I've not dug into why this makes a difference, though (and I suspect it shouldn't 🙁)

@pradyunsg
Copy link
Member

@pfmoore
Copy link
Member

pfmoore commented Mar 1, 2022

Thanks, that was the one I remember. So I guess the question I have is whether we should just change pip to get behaviour consistent with packaging.tags, or whether there's a more fundamental question, about whether packaging.tags.sys_tags() should behave the same as when you call the lower-level API but passing the current environment's interpreter/abi/etc. The latter question is non-trivial, because it's not immediately obvious (to me, at least) how to replicate the way sys_tags builds its results out of the lower-level APIs.

I'm +1 on consistency, and +0 on changing pip in the short term, but I think the longer-term solution should probably come from packaging (maybe sys_tags should gain arguments that let you override the platform/interpreter/abi).

It's worth pointing out, though, that changing pip would mean that pyxll would no longer be installable with pip - which is probably not the outcome pyxll users would desire...

@uranusjr
Copy link
Member

uranusjr commented Mar 1, 2022

In terms of this issue specifically, personally I don’t think pip should disallow this particular tag, mostly with a “why not” reasoning. It’s not inconceivable to come up with scenarios (albeit contrived) that the tag may make sense to someone.

@pfmoore
Copy link
Member

pfmoore commented Mar 1, 2022

Agreed, I should have said that the first thing to decide here is what is the "correct" behaviour. As you say, there's no clear reason why this specific tag should be disallowed. From what I recall the set of tags packaging uses was mostly decided by surveying what was in general use and taking a "this looks like a reasonable set" approach (practicality beats purity - the theoretically correct tag set is pretty huge...)

In hindsight I wish tags had been designed so that consumers didn't have to explicitly list every tag they accepted, but it's too late for such a major change now.

@pradyunsg
Copy link
Member

Well, if we wanna keep this, then let's get packaging to add it in as well.

/cc @brettcannon

@brettcannon
Copy link
Member

Does packaging explicitly remove cpXX-none-any when joining the lists?

Nope, packaging never generates the tag.

It’s not inconceivable to come up with scenarios (albeit contrived) that the tag may make sense to someone.

Yep, although when I did the original research on tags out there I believe there was less than 10 in all of PyPI that had this sort of format.

As you say, there's no clear reason why this specific tag should be disallowed.

See pypa/packaging#311 for the complete discussion as to why it was left out, but it's mostly a question of:

  1. What is this sort of wheel even meant to represent?
  2. Where does it go into the tag list?

In hindsight I wish tags had been designed so that consumers didn't have to explicitly list every tag they accepted, but it's too late for such a major change now.

Oh, to dream...

Although I do have an idea of coming up with a JSON schema that records what a system needs to know in order to install wheels as appropriate (hence why I asked https://discuss.python.org/t/what-information-is-needed-to-choose-the-right-dependency-file-for-a-platform/13447 ).

Well, if we wanna keep this, then let's get packaging to add it in as well.

As I said in pypa/packaging#311 (comment) , at this point I'm willing to review a PR to add such a tag (don't feel motivated to author it), but do read through that entire issue to understand the design considerations that the person proposing it will need to go through.

joonis added a commit to joonis/packaging that referenced this issue May 2, 2022
brettcannon added a commit to pypa/packaging that referenced this issue Aug 19, 2022
Closes #511

See pypa/pip#10923 for motivation.

Co-authored-by: Brett Cannon <brett@python.org>
@uranusjr
Copy link
Member

I think we have resolved that it’s fine for pip to keep this tag allowed.

@uranusjr uranusjr closed this as not planned Won't fix, can't repro, duplicate, stale Aug 20, 2022
@uranusjr uranusjr added resolution: no action When the resolution is to not do anything and removed S: needs triage Issues/PRs that need to be triaged labels Aug 20, 2022
@brettcannon
Copy link
Member

The next release of packaging will include the tag: pypa/packaging#511 .

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
resolution: no action When the resolution is to not do anything type: bug A confirmed bug or unintended behavior
Projects
None yet
Development

No branches or pull requests

5 participants