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

python tag issue with pip wheel / pip install / caching #7296

Closed
sbidoul opened this issue Nov 4, 2019 · 6 comments · Fixed by #7319
Closed

python tag issue with pip wheel / pip install / caching #7296

sbidoul opened this issue Nov 4, 2019 · 6 comments · Fixed by #7319
Labels
auto-locked Outdated issues that have been locked by automation C: cache Dealing with cache and files in it C: wheel The wheel format and 'pip wheel' command type: bug A confirmed bug or unintended behavior

Comments

@sbidoul
Copy link
Member

sbidoul commented Nov 4, 2019

A tricky issue I discovered while re-reading WheelBuilder.build.

I got to wonder why the python tag of the built wheel would depend on whether we were doing pip install (should_unpack=True) or pip wheel (should_unpack=False).

With pip 19.3.1, do this (the chosen distribution name is a pure python sdist that has no wheel PyPI):

  • pip install odoo-autodiscover (the wheel is built and cached)
  • pip wheel odoo-autodiscover (the wheel is obtained from cache: odoo_autodiscover-2.0.0-cp37-none-any.whl)

Notice the cp37 tag.

Now, empty the cache and do:

  • pip wheel odoo-autodiscover (the wheel is built and you get: odoo_autodiscover-2.0.0-py3-none-any.whl)

Notice the py3 tag.

So depending on whether the wheel was installed and cached before, the result of pip wheel is different.

This behaviour was apparently introduced in 0e240d7.

Now with systematic caching introduced in #7285, the behavior is subtly different: a py3 or cp37 wheel gets cached depending on whether pip install or pip wheel is executed first. Whereas before #7285, only cp37 could be cached, never py3.

I'm not too sure what to do with that yet, as I don't really understand the motivation of 0e240d7.

@triage-new-issues triage-new-issues bot added the S: needs triage Issues/PRs that need to be triaged label Nov 4, 2019
@sbidoul
Copy link
Member Author

sbidoul commented Nov 5, 2019

PR that introduced the behavior #3225

@sbidoul
Copy link
Member Author

sbidoul commented Nov 7, 2019

Reading #3025 (from 2015) it would seem this was made to workaround issues with some setup.py of pure python distributions that would adapt their install_requires section based on the python version they are running setup.py on. I think with modern requirements specifiers this would not be necessary today (but might still be relevant for old distributions that publish sdists only?)

So there are several possible approaches (note this applies to pure python distributions only, as binary wheels work correctly):

  1. abandon the workaround: cache and output wheels without implementation tag (so with py2 or py3 tag), on the ground that, in 2020, the issues raised in cache poisoning via automated wheels #3025 have faded away and/or are easy to fix in broken distributions, also knowning that the workaround does not really make sense for pep517 builds
  2. cache and output wheels with the implementation tag always, so pip wheel would always produce implementation specific wheel names for pure python wheels
  3. restore pre Make pip wheel cache what it built #7285 behaviour, which is subtly broken
  4. keep current master behaviour, which is subtly broken in a different way

Only 3. has no behavior change compared to pip 19.3.

It might not be relevant but I note that, wrt the pip code base:

  1. means a significant simplification (drop python_tag in a lot of places)
  2. is a minor simplification
  3. adds some complexity
  4. is status quo wrt current master

@sbidoul
Copy link
Member Author

sbidoul commented Nov 8, 2019

  1. is probably not a good idea.

If 1. is considered too risky, we could combine it with having a separate wheel cache root directory per python implementation.

sbidoul added a commit to sbidoul/pip that referenced this issue Nov 9, 2019
Make sure ``pip wheel`` never outputs pure python wheels with a
python implementation tag. Better fix/workaround for
`pypa#3025 <https://github.com/pypa/pip/issues/3025>`_ by
using a per-implementation wheel cache instead of caching pure python
wheels with an implementation tag in their name.

Fixes pypa#7296
@pradyunsg pradyunsg added C: cache Dealing with cache and files in it C: wheel The wheel format and 'pip wheel' command type: bug A confirmed bug or unintended behavior and removed S: needs triage Issues/PRs that need to be triaged labels Nov 10, 2019
@pradyunsg
Copy link
Member

Ah gee. Thanks for investigating and spotting this @sbidoul!

I'm inclined to say #3225 can basically be reverted now.

sbidoul added a commit to sbidoul/pip that referenced this issue Nov 10, 2019
Make sure ``pip wheel`` never outputs pure python wheels with a
python implementation tag. Better fix/workaround for
`pypa#3025 <https://github.com/pypa/pip/issues/3025>`_ by
using a per-implementation wheel cache instead of caching pure python
wheels with an implementation tag in their name.

Fixes pypa#7296
@chrahunt
Copy link
Member

I agree that projects should be using the existing facilities (e.g. python_version and implementation_name markers) to specify conditional dependencies.

I have a few concerns with removing the interpreter-specific wheel cache outright:

  1. the failures will happen at runtime and will be hard to connect to the wheel-building/caching because of conditional dependencies in setup.py and conditional imports in code. I can't think of a way other than "check the setup.py at the first sign of any issue" to help identify that a problem is caused by a non-conforming package.
  2. I can't see any good place to warn users about this. Building any legacy project into a wheel (or any project that uses a backend which produces metadata nondeterministically but doesn't update wheel tags) may produce a wheel that is not actually portable, but warning for all of them seems like it would be overkill.

@xavfernandez
Copy link
Member

xavfernandez commented Nov 11, 2019

If 1. is considered too risky, we could combine it with having a separate wheel cache root directory per python implementation.

seems the cleanest to me but would require some thinking on the cache layout transition.

Edited: and I've just seen that #7319 has been created 😫 Don't mind me 😖

sbidoul added a commit to sbidoul/pip that referenced this issue Nov 16, 2019
Make sure ``pip wheel`` never outputs pure python wheels with a
python implementation tag. Better fix/workaround for
`pypa#3025 <https://github.com/pypa/pip/issues/3025>`_ by
using a per-implementation wheel cache instead of caching pure python
wheels with an implementation tag in their name.

Fixes pypa#7296
sbidoul added a commit to sbidoul/pip that referenced this issue Dec 2, 2019
Make sure ``pip wheel`` never outputs pure python wheels with a
python implementation tag. Better fix/workaround for
`pypa#3025 <https://github.com/pypa/pip/issues/3025>`_ by
using a per-implementation wheel cache instead of caching pure python
wheels with an implementation tag in their name.

Fixes pypa#7296
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jan 11, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: cache Dealing with cache and files in it C: wheel The wheel format and 'pip wheel' command type: bug A confirmed bug or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants