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

Latest wheel==0.32.0 breaks auditwheel/wheeltools.py #102

Closed
hugovk opened this issue Oct 1, 2018 · 31 comments
Closed

Latest wheel==0.32.0 breaks auditwheel/wheeltools.py #102

hugovk opened this issue Oct 1, 2018 · 31 comments

Comments

@hugovk
Copy link
Contributor

hugovk commented Oct 1, 2018

Wheel 0.32.0 was released two days ago and has refactored its (private) API, breaking auditwheel:

Traceback (most recent call last):
  File "/usr/local/bin/auditwheel", line 11, in <module>
    sys.exit(main())
  File "/opt/_internal/cpython-3.6.6/lib/python3.6/site-packages/auditwheel/main.py", line 49, in main
    rval = args.func(args, p)
  File "/opt/_internal/cpython-3.6.6/lib/python3.6/site-packages/auditwheel/main_repair.py", line 43, in execute
    from .repair import repair_wheel
  File "/opt/_internal/cpython-3.6.6/lib/python3.6/site-packages/auditwheel/repair.py", line 13, in <module>
    from .wheeltools import InWheelCtx, add_platforms
  File "/opt/_internal/cpython-3.6.6/lib/python3.6/site-packages/auditwheel/wheeltools.py", line 14, in <module>
    from wheel.util import urlsafe_b64encode, open_for_csv, native  # type: ignore
ImportError: cannot import name 'open_for_csv'

https://travis-ci.org/python-pillow/pillow-wheels/jobs/435427307#L4860

The same thing hit multibuild and delocate: pypa/wheel#255.

Note that wheel officially has no public API:

It should be noted that wheel is not intended to be used as a library, and as such there is no stable, public API.

pypa/wheel#255

(We're working around the issue by pinning to the previous wheel==0.31.1.)

@agronholm
Copy link

I had no idea that so many tools relied on the private API of wheel. This is a bummer.

@hugovk
Copy link
Contributor Author

hugovk commented Oct 1, 2018

Yep. A lot of duplicates, but 32k results for this one:

https://github.com/search?l=Python&q="from+wheel.util+import"+open_for_csv&type=Code

People in general don't read docs. Perhaps using the _underscore convention would be more clear (but would apply to everything, and break all third-party use.)

@matthew-brett
Copy link
Contributor

Dammit - it is unfortunate that Robert copied the delocate wheel code without the tests. That makes it much harder to know if the fixes are going to work.

I've just fixed this in the latest delocate release. Maybe auditwheel can import from delocate?

@rmcgibbo
Copy link
Member

rmcgibbo commented Oct 1, 2018

Sorry :/

@Mizux
Copy link

Mizux commented Oct 2, 2018

My 2 cents, in Manylinux docker image auditwheel program didn't work anymore.
Since auditwheel use an "hidden" python located at /opt/_internal/cpython-3.6.6/bin/python
So I use:

/opt/_internal/cpython-3.6.6/bin/python -m pip install wheel=0.31.1

@matthew-brett
Copy link
Contributor

@Mizux - the Manylinux docker image auditwheel should work now, with a version of the workaround you're using - does it work for you?

@Mizux
Copy link

Mizux commented Oct 2, 2018

@matthew-brett I use this workaround yesterday (Paris Time ~ 15h ago) to generate our python ortools packages....
I should try again today without this patch and using your last docker image...

ps: here the patch google/or-tools@c907c57
and here the dockerfile use: https://github.com/google/or-tools/blob/master/tools/docker/manylinux1.Dockerfile

tensorflow-copybara pushed a commit to tensorflow/tensorflow that referenced this issue Oct 3, 2018
tatatodd added a commit to tensorflow/tensorflow that referenced this issue Oct 3, 2018
1.12-rc0 cherry-pick request: Pin wheel=0.31.1 to work around issue pypa/auditwheel#102
tensorflow-copybara pushed a commit to tensorflow/tensorflow that referenced this issue Oct 4, 2018
benjamintanweihao pushed a commit to benjamintanweihao/tensorflow that referenced this issue Oct 12, 2018
@ehashman
Copy link
Member

Anyone have objections to me pinning to 0.31.1 in requirements.txt for now until we can actually fix this?

@matthew-brett
Copy link
Contributor

Sounds reasonable to me.

@agronholm
Copy link

Also, make sure that you let me know in pypa/wheel#262 how auditwheel would like to use the wheel library (if @matthew-brett has missed something). This will help me design a proper public API which will not be broken on a whim.

@ehashman
Copy link
Member

We have all these wacky helpers defined for being able to treat a wheel as a context. Unclear if that's the sort of thing that should go into a public API but @agronholm I will take a deep look and update that issue when I'm done.

@matthew-brett
Copy link
Contributor

@ehashman - these helpers are all copied from Delocate, I think, but with some modifications. If there was a way to merge the changes back into delocate, then we could maintain this stuff in one place. And there would be tests!

@ehashman
Copy link
Member

Ah, great. Emphatic +1.

There's a PyPA sprint next weekend, I'd be very interested in looking at this then.

@anthrotype
Copy link

is anybody working on this? I'm at the PyPA sprint in London and could work on updating auditwheel to work with wheel 0.32

@matthew-brett
Copy link
Contributor

@ehashman - are you at the sprint? @anthrotype - I'm sure you saw the stuff above - if it were me, I'd check that the delocate code does what's needed, and depend on that, because it has tests, and it would mean only one place to maintain the code (Robert C copied it from delocate originally).

@matthew-brett
Copy link
Contributor

Or even break that part of the code out into its own tiny module. Maybe that would make it easier for Wheel to know what they need to keep stable in the API.

@ehashman
Copy link
Member

Is there a chat platform folks are using to communicate between London and NYC for the sprint?

Since @matthew-brett noted that this stuff was copied from delocate, I wonder if we could also grab their unit tests in order to verify the behaviour is correct.

@anthrotype I took a quick look at your repo---I'm pretty sure we shouldn't add a multibuild dependency to auditwheel. I think the "right" way to fix this would be to either modify our code to work with the new wheel 0.32.0 UI or work on a public UI in wheel.

@matthew-brett
Copy link
Contributor

I'm on Google chat if y'all need me - matthew.brett@gmail.com .

Yes, definitely add the unit tests.

I think the multibuild submodule doesn't need to be in the new repo, but the plan is to make a new pypi-installable package to do the wheel stuff. Then delocate and auditwheel would import that.. For delocate I have some test wheels with extension code in there, but we could make small pure python test wheels for the tests, or use the one I'm building in delocate.

@anthrotype
Copy link

We can remove multibuild, sure. I only added it to reuse the same Travis setup and ensure the existing delocate tests are still working (they do now).
I’m off for today. I think I added both @matthew-brett and @ehashman as collaborators.
You can take over from here.

@anthrotype
Copy link

I have removed multibuild as well as the functions in wheeltools/tools.py that are only required by delocate and hence are macOS-specific (e.g. those that use otool and install_name_tool commands).

What we need to do now is:

  1. check whether the wheeltools module that is vendored in auditwheel has diverged from the upstream delocate/wheeltools, and if so integrate any changes into the standalone wheeltools package.
  2. publish wheeltools to PyPI (the travis setup is configured to automatically upload on tagged commits, using my PyPI encrypted credentials).
  3. update auditwheel and delocate to import from the new wheeltools module.

@anthrotype
Copy link

I have put up the scaffolding of a stand-alone wheeltools module, by extracting the wheeltools.py and tools.py modules (and respective unit tests) from the current delocate master branch.
I've also verified that rest of the delocate test suite passes when importing from the standalone module (matthew-brett/delocate#46).

I then tried to take this wheeltools module extracted from delocate and import it inside auditwheel, however I stumbled on a few issues and am not sure how (or whether) to proceed.

The wheeltools.add_platforms function in particular has significantly diverged between delocate and auditwheel:

  1. In delocate, it takes the wheel filename as input, whereas in the latter it takes an already open InWheelCtx object.
  2. the auditwheel add_platforms has an additional remove_platforms keyword argument, which the delocate version lacks (in fact, the function name is add_platforms, which suggests that it only extends the list of platforms).
  3. delocate raises an error "Cannot add platforms to pure wheel" when the "Root-Is-Purelib" whereas (see the recent Support non-extension wheels with binary dependencies #110), auditwheel also checks for binaries inside a purelib root, or ignores the pure wheel without raising an error.
  4. auditwheel has some informative print() statements, whereas the delocate version does not (Replace print statements with logger #109)

There may be more differences which I'm not aware of yet. I have to say, I'm not familiar enough with the Wheel specification; also am busy with other things, and am not sure I can promise to continue working on this in the coming weeks.
I guess I underestimated the amount of work needed (I should have checked with you guys before embarking on it). Anyway, I hope @ehashman and @matthew-brett will continue from where I left and that this effort of mine was not completely wasted. :)

benjamintanweihao pushed a commit to benjamintanweihao/tensorflow that referenced this issue Dec 5, 2018
jcfr added a commit to scikit-build/ninja-python-distributions that referenced this issue Feb 24, 2019
jcfr added a commit to kjerstadius/ninja-python-distributions that referenced this issue Feb 24, 2019
phdru added a commit to CheetahTemplate3/cheetah3 that referenced this issue Mar 19, 2019
dimitern added a commit to dimitern/xmlstarlet that referenced this issue Oct 29, 2019
Longbowman pushed a commit to bowman-lab/enspara that referenced this issue Dec 28, 2019
@lkollar
Copy link
Contributor

lkollar commented Mar 2, 2020

Closing this as work on wheeltools seems to have stopped and wheel has been upgraded to 0.34 in #223, so the original issue is fixed.

@anthrotype feel free to open a new issue if/when you think wheeltools can replace the vendored version.

@lkollar lkollar closed this as completed Mar 2, 2020
jfolz added a commit to jfolz/simplejpeg that referenced this issue Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants