replace pep425tags with packaging

[mattip]

Fixes gh-345

First steps

• add packaging to setup and testing for CI
• make sure tests are run with a modern setuptools and pip
• concentrate all the uses of pep425tags in one place

[mattip]

It would be nice if packaging exposed this method. I think the XXX is kind of impossible to remove.

[mattip]

Here is where I need some help from the packaging people: how can we replace the use of pep425tags: get_abbr_impl(), get_impl_ver(), get_abi_tag(), get_supported()

[brettcannon]

Looking at how pip transitioned is probably best as @pradyunsg pointed out, but a quick guess is:

• get_abbr_impl() -> implementation_name()
• get_impl_ver() -> implementation_version()
• get_abi_tag() -> No direct equivalent for just that
• get_supported() -> sys_tags() or proper combination of the other *_tags() functions depending on the situation

[mattip]

Thanks. It seems packaging might want to support get_python_tag, get_abi_tag and get_platform_tag, as well as get_platform (mentioned in a comment above).

[pradyunsg]

I suggest taking a look at pypa/pip#6908, which is the pip's tracking issue for porting to packaging.tags.

[mattip]

macOS version dectection via pep425tags.py uses any dynlib files to determine the base_version of macOS to support in the wheel (see calculate_macosx_platform_tag. packaging does not seem to have this ability.

[agronholm]

macOS version dectection via pep425tags.py uses any dynlib files to determine the base_version of macOS to support in the wheel (see calculate_macosx_platform_tag. packaging does not seem to have this ability.

And this was one of the biggest road blocks to switching to packaging.

[mattip]

OK, now that I understand the complexities, I will see if packaging will accept some patches to support this all.

[mattip]

There is a merge conflict. Does this project prefer merging from master or rebase/force-push to clear merge conflicts?

[agronholm]

Doesn't matter either way, as I will squash the commits in the end anyway.

[mattip]

Rebased and checks are passing. Is sys_tags as a replacement for supprted_tags up to the task on macOS? The old code did some strange parsing of dynlib files to find the supported macOS version, I think modern python distributions all use a proper sysconfig property instead.

[mattip]

Is this check important? The order of tags returned from sys_tags is different.

[agronholm]

Probably not, but I didn't write that code.

[agronholm]

Is sys_tags as a replacement for supprted_tags up to the task on macOS? The old code did some strange parsing of dynlib files to find the supported macOS version, I think modern python distributions all use a proper sysconfig property instead.

I think you should talk to @Czaki who wrote that detection code.

[Czaki]

@mattip this code is quite new. It was introduced in #314.
I look to python 3.7 and still this sysconfig returns deployment target against which python is build or current MACOSX_DEPLOYMENT_TARGET from environment. But it does not cover other build system like cmake. For example if you set c++ standards to 17 (with set (CMAKE_CXX_STANDARD 17) ) then libraries are compiled against macos 10.13 or 10.14 (I cannot check in this moment, most probably 10.14).

So sysconfig properties does not allow to determine proper minimal macos version.

[mattip]

@Czaki thanks. I think I can redo the checks in #314 in this PR. All the tests pass here which is a bit worrisome. I would think that there should be some failing test to show that sys_tags is a maximal set of tags that needs to be filtered for unsupported macOS versions.

[Czaki]

All the tests pass here which is a bit worrisome

You use own implementation of get_platform placed in src/wheel/bdist_wheel.py and test checking one from src/wheel/pep425tags.py
Yes, I do not add tests for class bdist_wheel(Command): for cover this. I try to rethink this and create such test, but I may need help with understand flow of Command execution.

I would think that there should be some failing test to show that sys_tags is a maximal set of tags that needs to be filtered for unsupported macOS versions.

Could you explain? How You would like to limit it?

[mattip]

I don't see how the condition on line 308 can be satisfied, since self.plat_name_supplied is set to self.plat_name is not None. It would seem that self.plat_name_supplied can be removed?

[Czaki]

It was in code before #314. But it somehow work, because I meet problem and need to create also #317. Maybe plat_name is set somehow in base class.

[mattip]

I removed the WIP label. I am curious what the project maintainers think of this. It no longer imports pep425tags, and either uses packaging.tags instead or adds the code directly to bdist_wheel.py. It does not go all the way since packaging.tags still does not support get_platform(directory) and get_abi_tag(), but I think using platform.sys_tags() is a step forward.

[gaborbernat]

Without wheel note pip builds are also a lot slower. So that freedome costs a lot of performance.

@pfmoore @hroncok for reasons why see pypa/virtualenv#1481 (comment) with benchmark provided by @asottile at pypa/virtualenv#1481 (comment)

$ bash t.sh 
+ : with wheel
+ clean_env
+ rm -rf /home/asottile/.cache/pip
+ rm -rf venv
+ virtualenv venv --no-download
+ run_test
+ venv/bin/pip install -qq pyyaml
+ venv/bin/pip uninstall -qqy pyyaml
+ venv/bin/pip install -qq pyyaml

real	0m0.900s
user	0m0.726s
sys	0m0.053s
+ : without wheel
+ clean_env
+ rm -rf /home/asottile/.cache/pip
+ rm -rf venv
+ virtualenv venv --no-download
+ venv/bin/pip uninstall -qqy wheel
+ run_test
+ venv/bin/pip install -qq pyyaml
+ venv/bin/pip uninstall -qqy pyyaml
+ venv/bin/pip install -qq pyyaml

real	0m9.808s
user	0m9.333s
sys	0m0.249s

without wheel installed, pip cannot build a wheel (and therefore can't cache the built output)

with PEP-517/518 its increasingly unnecessary to have this build dependency, as pip would put it into its build env anyway, its however potentially still creating problems for certain legacy packages, as the pip cant build a wheel directly (unchecked, need to check if it holds true)

For some reason I can't reply inline to this comment :-(

Yes, when using the legacy (non-PEP 517) code path, pip will check if wheel is importable, and only build a wheel if it is. In the absence of wheel, it will fall back to setup.py install, which has a number of differences in semantics, as well as not being cache-able.

[pfmoore]

@pfmoore @hroncok for reasons why see pypa/virtualenv#1481 (comment)

You may have misunderstood my comment (and the fact that @hroncok gave it a ❤️ suggests that he did too).

I am completely in favour of virtualenv installing wheel by default, I think that's a real usability benefit of virtualenv over venv. I was not at all saying that I think this should change, just noting that because virtualenv has seeded packages other than pip, we should expect them to have dependencies and therefore adding support for seeded packages with dependencies to virtualenv is a necessary step if we want to avoid having a "must vendor all its dependencies" policy proliferate through the packaging ecosystem - which IMO would be a very bad mistake, both in principle and in practice.

[gaborbernat]

The thing is this policy only affects three projects: pip, virtualenv and wheel. No other packaging projects need to vendor. And all of these three played along this rule until now. wheel was the first to break out of this, and I don't think either pip or setuptools has any plans to change their vendoring policies. Not Fedora maintainers already voiced their opinion on virtualenv tracker that they really would prefer wheel to not take on dependencies too... And this will impact similarly all other distribution maintainers. Then again if wheel goes ahead with taking dependencies virtualenv will have no choice but to add support for it.

[pfmoore]

I thought setuptools tried to de-vendor but had to revert because it caused issues? I could see them revisiting that. And personally, I view pip's vendoring as an unpleasant limitation of the current state of pip and the packaging ecosystem in general. I'd be more than happy if we found a way for pip to avoid vendoring some or all of its dependencies (not that I think this is likely 🙁).

And of course, wheel is not a fundamental package, any more than something like packaging is - the only reason virtualenv needs to seed wheel is to support pip's "legacy" (non-PEP517) install process. Maybe setuptools should be vendoring wheel instead? After all, we only need wheel because setuptools can't natively build wheels...

Anyway, I'll let the project maintainers decide. My opinion is purely around concerns of principle, and practical implications should beat that.

[gaborbernat]

To sum this up @agronholm are you still interested in going ahead with wheel having dependencies? (at pypa/virtualenv#1923 Fedora maintainers expressed their hope of not going down this path)

[agronholm]

That is my wish, because otherwise wheel will have to keep re-vendoring the tag code. However, if there is significant resistance and real arguments presented against it, I will stop pursuing the idea.

[carljm]

This PR broke the ability to set the plat_name attribute and have supported tags include that platform (previously supported by https://github.com/pypa/wheel/pull/346/files#diff-7af7e00575edbab17718ee16d91b8982L207 ). This breaks our usage, where we use a distutils config file to set a plat-name for bdist_wheel command, and is preventing us from upgrading wheel.

[agronholm]

@carljm Could you explain your case in detail?

[mattip]

Here is my analysis, but I did not actually write a test and check it.

The code to check for tag validity, which is called for non-pure wheels, now checks that plat-name is valid. A user who specifies their own, non-tag-compliant plat-name would fail this test. But that is disallowed on purpose, at least according to this comment

bdist sets self.plat_name if unset, we should only use it for purepy wheels if the user supplied it (emphasis added)

The older code seems to have let non-compliant plat-names through: whether by design or by mistake. So while the comment and behavior now are consistent, I inadvertently changed behavior in that PR.

What I don't understand, and maybe @carljm could clarify, is how pip will install such a wheel: it should reject it.

[mattip]

Hmm, but the test_plat_name_ext test should check that my analysis is mistaken ...

[pradyunsg]

And, in case we do end up finding a valid reason to have this, I suggest filing an issue on pypa/packaging instead of trying to change only-wheel to do the right thing here.

[carljm]

Could you explain your case in detail?

Sure. At work we have custom versioned internal Linux platforms each with its own specific set of C library versions and compiler versions etc. So we build a wide variety of shared libraries for these platforms, including many Python wheels that link against libraries in the platform. So we pass --plat-name to bdist-wheel when building wheels so they are marked with the appropriate platform.

What I don't understand, and maybe @carljm could clarify, is how pip will install such a wheel: it should reject it.

I don't know about this either way, because we don't use pip to install them. As far as I know wheel is a standardized Python packaging format, it is not owned by pip and should not necessarily serve only pip-supported use cases.

Hmm, but the test_plat_name_ext test should check that my analysis is mistaken ...

Yes, to me it seems quite intentional in the previous code that this plat-name override was allowed, given that it's tested. Also I wonder how test_plat_name_ext is still passing after this PR and why didn't it catch the removal of this support?

I suggest filing an issue on pypa/packaging instead of trying to change only-wheel to do the right thing here.

I'm not sure why. This was functionality entirely contained within the wheel library that previously existed and was tested but has now regressed. Seems straightforward to fix as a regression in wheel, it doesn't require changes in any other tool. If other tools (e.g. pip) want to make changes to support a similar use case, that seems fine, but it shouldn't be a prerequisite to fixing the regression.

If a PR to restore this functionality would be accepted, I'd be happy to supply one.

[carljm]

Oh, I see the description of #374 explains why the test failure was not noticed.

[agronholm]

@carljm Before I make a decision, I'd be interested in learning what you use the wheels for if you don't use pip to install them. Do you just unpack them directly or do something else with them?

That said, I am leaning towards restoring the old functionality.

[carljm]

@agronholm Yes, we have our own internal tooling that selects the right wheel from our wheel archive based on platform/version/abi tags and then unpacks them where we need them. Wheel is ultimately a very easy format to use, that's one of its attractions :)

[agronholm]

Are you using wheel unpack to extract them?

Wheel's internal structure could get a bit more complicated than just a simple zip file down the road. The wheel library's API and CLI will abstract that for the users, but if you're using unzip to extract the wheels, you may be in for an unpleasant surprise in the foreseeable future.

[carljm]

I'll have to defer to @jreese for that question, I haven't worked on that side of the system. Thanks for the heads-up!

[amyreese]

We do a combination. For the majority of use cases, we use Buck to build our applications, and Buck will extract the wheel contents, while also handling data directories. While building packages with dependencies that we have already built as wheels, we will also pip install these wheels into the chroot/virtualenv, using a custom ~/.pydistutils.cfg and pip.conf that it configured to specify our custom platform name.

Altogether, this allows us to better distinguish between wheels built for our internal platform/compiler/toolchain from upstream wheels from PyPI, so that we a) don't accidentally use upstream binaries for internal bulids, and b) don't use the internal platform wheels on engineers' laptops with standard virtualenvs.

That said, if the wheel standard changes, we'll change buck to adapt, but the need/desire for building wheels against custom platforms will remain.

[agronholm]

I would recommend changing Buck to depend on wheel's new public API (when that becomes available) to make it automatically adapt to any future changes in the wheel format.

[amyreese]

I would recommend changing Buck to depend on wheel's new public API (when that becomes available) to make it automatically adapt to any future changes in the wheel format.

Given that Buck isn't written in Python, it's unlikely that we could use the API directly, but if wheel provides a CLI equivalent, that would help.

[agronholm]

Given that Buck isn't written in Python, it's unlikely that we could use the API directly, but if wheel provides a CLI equivalent, that would help.

There is the wheel unpack command.

[gaborbernat]

@agronholm so where this land in the end, do you still plan to add dependencies for wheels package? If so we should come up with some battle plan to not break virtualenv.

[agronholm]

I would like to not have to vendor any dependencies, so yes, go ahead and make your plans :)

[aexvir]

@agronholm Hi there, apologies for the direct ping
We noticed that our automated wheel builds started failing, and we traced it back to this change.

So... now that wheel requires the tags to be included in the ones supplied by packaging.tags.sys_tags() I'm wondering how are we supposed to build wheels for custom platforms, like in our case, with alpine linux, as distutils.util.get_platform() returns linux_x86_64 for alpine.

We are running something like

python setup.py bdist_wheel --plat-name=linux-alpine3_12_x86_64

I honestly do see value on being able to specify custom platforms, it made our work so much easier so far, this change is kind of throwing out all the tooling we built around this... 😕

I'm actually now wondering a bit what's the utility of the --plat-name argument if it will always be validated against the output of sys_tags 🤔

If you don't think this should be solved on this project... should this issue be handled at a higher level? Like having sys_tags differenciate between musl and glibc based systems?
In that case... where do we draw the line of how granular should that distinction be?

[agronholm]

That was an unintended side effect of this PR and has already been rectified in master via PR #375. I will try to get a patch release out soon.

[aexvir]

Oh... for some reason It didn't seem obvious to me that I could check further commits... sorry for that

Thanks!