Remove distutils.util usage from packaging.tags #349

uranusjr · 2020-11-07T07:33:55Z

I basically copied distutils.util.get_host_platform(), run Black on it, and make the code slightly more modern.

There are several possible refactoring available, e.g. move the Windows and Linux logic out of the function to short-circuit some logic, and use function arguments to avoid os.uname() being called repeatedly. But each of these adds chance for things to break, and I feel it’s better to first include an implementation that guarentees no backward incompatibility before we try to improve it.

Fix #327.

packaging/tags.py

mattip · 2020-12-11T07:29:21Z

This is in case PEP 632 to deprecate distutils goes through?

uranusjr · 2020-12-11T07:56:46Z

The main reason is because distutils is a poor source to get this information, its internals being undocumented and may break without warning (it already did for some people). See #327 (comment).

puetzk · 2020-12-11T19:02:39Z

My interest started because distutils has already made a breaking change, and in Python >= 3.8 pip no longer works in the vcvarsall "Developer Prompt for Visual Studio XXX" environment - it tries to install wheels matching the MSVC target architecture, rather than the python.exe into which it's installing things. See https://bugs.python.org/issue38989 for my report of this end-symptom, and pypa/pip#8649 for someone else's.

@zooba has indicated, both on python.org and on github that he considers distutils.util.get_host_platform() a private function that should not be used for this purpose. Of course, he's also the author of PEP 632, so it's probably not unrelated to propsing deprecation and removal in general.

zooba · 2021-01-04T16:11:09Z

@zooba has indicated, both on python.org and on github that he considers distutils.util.get_host_platform() a private function that should not be used for this purpose. Of course, he's also the author of PEP 632, so it's probably not unrelated to propsing deprecation and removal in general.

Yeah, the recommendation to not use the function is because it's going to be deprecated soon. Distutils has poor platform detection, so anything else will be an improvement, and honestly I like packaging.tags as a location for it, since then other tools can ask "which platform tag should I build for" (since e.g. manylinux has different kinds of rules from win32). We're progressively moving packaging stuff out of the stdlib anyway, so that it can be divorced from the language version.

zooba · 2021-01-28T20:57:24Z

@uranusjr How can we help this along? People keep being bitten by pip looking at the cross-compile target rather than the current platform.

uranusjr · 2021-01-28T22:48:20Z

I've done all I could, someone needs to review and merge the PR.

zooba · 2021-01-29T01:13:15Z

packaging/tags.py

+        import sysconfig
+        import _osx_support
+
+        osname, release, machine = _osx_support.get_platform_osx(


My only query is whether this should rely on _osx_support (which I assume is a core CPython module?) or reproduce whatever it's doing.

Correct, it’s a helper module in stdlib. The function is quite complicated and I’d very much keep referencing the stdlib if possible. Not sure who to ask about whether this is a good idea.

@ronaldoussoren @ned-deily

Looks like the equivalent public API for this is sysconfig.get_platform(), so it should be an easy swap. It might even make a suitable fallback for all cases after you've gotten past the initial tests. I suspect you'll still want your own version of it for backporting fixes/changes, but if the stdlib functionality is fine for this bit then you may as well use as much as you can/like.

That said, I only compared to the version of sysconfig in master... it might be that there are already improvements that aren't in 3.9, and so you'll still need the logic you've got here. But the _osx_support reference can't really be any worse in earlier versions.

git blame the everything that changed in that function during the recent 5 years are about Windows (ARM support), AIX improvements, and dropping BSD/OS support, so it’s probably safe. I’ll switch that API.

Actually, the same also applies to _aix_support (prior to 3.9 it’s just {osname}-{release}-{machine}, and was expanded to support PEP 425), so I replaced that as well.

_osx_support.get_platform_osx() only uses three configs: * MACOSX_DEPLOYMENT_TARGET * CFLAGS * _OSX_SUPPORT_INITIAL_CFLAGS All of these should be identical from both sources.

Black-ify, and replace %-formatting with str.format()

uranusjr · 2021-02-04T02:32:48Z

On further inspection, _get_host_platform is almost identical to sysconfig.get_platform (and the two have been updated simultaneously for a long while), except an ever so slight difference:

sysconfig: https://github.com/python/cpython/blob/master/Lib/sysconfig.py#L656-L671
distutils: https://github.com/python/cpython/blob/master/Lib/distutils/util.py#L38-L54

In distutils, the _PYTHON_HOST_PLATFORM override happens before if os.name != "posix" or not hasattr(os, 'uname'), but sysconfig puts it after the condition. git blame tells me this has been the case since the environ was first introduced in 2012: python/cpython@1abe1c5. Is there a reason to this? Does it cause functional differences? If not, the entire function can simply be replaced.

brettcannon · 2021-02-04T21:11:00Z

If the override isn't applied first, then applying it differently on various platforms seems odd, so I would go with the sysconfig approach for consistency.

puetzk · 2021-02-05T02:57:02Z

I see nothing in https://bugs.python.org/issue14330 indicating was intentional either. It seems to have been originally intended as an internal flag for cpython's own build system? But now seems to have a bit broader scope (e.g. dh-python's pybuild sets it, and debian documents it as part of cross-building extensions: https://wiki.debian.org/Python/MultiArch, and google found NumPy build scripts for iOS using it).

It is potentially a functional difference: sysconfig only honors _PYTHON_HOST_PLATFORM for posix systems with os.uname, distutils honors it for everything except nt. At least for CPython though, there don't actually seem to be any other options besides nt and posix. So it seems this should make little difference in practice - if it's os.name is nt both ignore it, if it's posix they both honor it, unless there really is a still-supported posix system where HAVE_UNAME is not defined, and thus os.uname is missing.

Which seems unlikely; uname dates all the way back to SVr4 and was standardized by POSIX.1-1988, though BSD didn't get it until 4.4BSD in 1994. The autoconf check and posixmodule.c here do in fact seem to date all the way back to pre-alpha-1.0.0: python/cpython@c39de5f. Back in 1992 it might have made sense to care about 4.3BSD :-) But I don't think there are any early-90s unices still supported by CPython 3.x, so should be moot.

The documentation suggests java is a valid 3rd option, presumably intended for the maybe-happening-someday jython3? All the other historical options seem to have been dropped in between python 2 and 3. But if it's java cross-compiling is kind of nonsensical. And the unfinished 2016 attempt at jython3 had its own sysconfig.py that deleted _PYTHON_HOST_PLATFORM anyway. Same would presumably go for any other non-CPython implementation, their sysconfig.py would be tweaked to their needs anyway.

puetzk · 2021-02-05T03:07:02Z

Epistemic status: reasonably comfortable with unix history, but no personal knowledge history here, just did doing some software archaeology to see if I could find anything interesting. @doko42 (the original author of _PYTHON_HOST_PLATFORM) seems to still be active in with an occasional cpython/setuptools review, so linking just in case he happens to have have made the sysconfig and distutils implementations different on purpose and (even more unlikely) still remembers why :-)

uranusjr · 2021-02-05T04:55:09Z

I’ve filed #396 so we have something to merge if we decide the two implementations are indeed equivalent.

ronaldoussoren · 2021-02-05T08:43:36Z

I haven't fully read up on this PR yet, but I do wonder if it wouldn't be better to move this functionality into the stdlib module sysconfig (with a clear and documented interface).

The function is closely related to other functionality in sysconfig (such as sysconfig.get_platform). This would also solve the issue you're having with relying on _osx_support.

BTW. _osx_support was introduced to centralise some functions that are used by both distutils and sysconfig.

mattip · 2021-02-05T09:18:11Z

Pinging @fangerer for a GraalPython perspective: I don't know if they change os.uname ?

uranusjr · 2021-02-05T09:27:56Z

@ronaldoussoren There is already a sysconfig.get_platform, but the behaviour is (very slightly) different. The main point to decide here is whether we should

Have our own implementation that exactly replicates distutils.util.get_host_platform.
Use sysconfig.get_platform as-is.
Change sysconfig.get_platform to match the behaviour of distutils.util.get_host_platform.

ronaldoussoren · 2021-02-05T09:33:33Z

The 4th option is to add sysconfig.get_host_platform if the difference in behaviour is important.

The sysconfig module was extracted from distutils and as such has a similar problem in that its specification is not very clear. I'd be careful with changing the behaviour of sysconfig.get_platform, there's a clear risk that this breaks some other project.

puetzk · 2021-02-05T14:28:22Z

graalpython appears to identify itself as os.name = 'posix', by implementing a builtin module named posix, including an implementation of os.uname.

So it would seem to be another one where the sysconfig and distutils implementation difference would not matter.

doko42 · 2021-02-05T15:37:26Z

Relying on the os module would break cross-building extensions. I didn't look into cross build recently, so I don't know if it's currently working.

The idea is to run the build python (running on your build machine) with the sysconfig module from your host/target machine, assuming that the sysconfig/sysconfig_data modules provide all information for the host/target. That cannot work, if people start relying on the builtin os module.

puetzk · 2021-02-05T15:38:57Z

@uranusjr had some discussions bout these different sources (before this PR was opened) at https://discuss.python.org/t/pep-425-platform-tag/5157/11

Just to link some more (probably stale) context, which many of the folks commenting have presumably seen, but I just ran across.

brettcannon · 2021-02-05T21:41:18Z

This might be one of those situations where we won't know the ramifications until we ask people widely to test or we do a release and wait to see what the complaints are.

pradyunsg · 2021-02-06T08:56:07Z

we do a release and wait to see what the complaints are.

I'm thinking let's do this but with #396. The behavior change feels like it has a low risk of breakage, with most people affected knowing their nuances better than us -- but I have no data/reasoning behind "why" beyond intuition TBH.

Relying on the os module would break cross-building extensions.

No? This is only used when the user doesn't provide a specific platform that they're generating these tags for. See the stack "down" from https://github.com/uranusjr/packaging/blob/no-distutils/packaging/tags.py#L275.

uranusjr · 2021-02-06T09:53:54Z

Note that the current implementation in the main branch already does not work particularly well in cross-building scenarios, since it relies on sysconfig values obtained internally. The experience can probably be made better if those values can ba specified manually by the user, but that’s a completely different feature request and should not be discussed with the topic at hand here (replacing distutils).

brettcannon · 2021-02-06T18:19:57Z

@pradyunsg your feeling is my feeling as well. No one has any good feeling as to why anything is the way it is, so I say go with the simpler solution. 😄

uranusjr · 2021-04-24T05:53:10Z

This has been replaced by #396.

uranusjr force-pushed the no-distutils branch from 154a55c to b5965a0 Compare November 7, 2020 07:45

puetzk mentioned this pull request Nov 13, 2020

[codebase] Python 3.6 required conan-io/tribe#3

Merged

puetzk reviewed Nov 13, 2020

View reviewed changes

packaging/tags.py Show resolved Hide resolved

brettcannon added the packaging.tags label Dec 11, 2020

Base automatically changed from master to main January 21, 2021 19:20

zooba reviewed Jan 29, 2021

View reviewed changes

uranusjr force-pushed the no-distutils branch from 870e9ee to 13364f7 Compare January 29, 2021 03:43

uranusjr added 4 commits February 4, 2021 10:12

Pull in distutils.util.get_host_platform()

00d3e53

Replace distutils.sysconfig call with sysconfig

06b4583

_osx_support.get_platform_osx() only uses three configs: * MACOSX_DEPLOYMENT_TARGET * CFLAGS * _OSX_SUPPORT_INITIAL_CFLAGS All of these should be identical from both sources.

Modernize _get_host_platform()

6ec5a26

Black-ify, and replace %-formatting with str.format()

Fix re.ASCII usage on Python 2

02faa59

uranusjr force-pushed the no-distutils branch from 1b7e957 to b7245d6 Compare February 4, 2021 02:15

Use sysconfig for _aix_support and _osx_support

2f3c6f6

uranusjr force-pushed the no-distutils branch from b7245d6 to 2f3c6f6 Compare February 4, 2021 02:19

uranusjr mentioned this pull request Feb 5, 2021

Replace distutils usage with sysconfig #396

Merged

uranusjr closed this Apr 24, 2021

uranusjr deleted the no-distutils branch December 10, 2021 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove distutils.util usage from packaging.tags #349

Remove distutils.util usage from packaging.tags #349

uranusjr commented Nov 7, 2020

mattip commented Dec 11, 2020

uranusjr commented Dec 11, 2020

puetzk commented Dec 11, 2020 •

edited by brettcannon

Loading

zooba commented Jan 4, 2021

zooba commented Jan 28, 2021

uranusjr commented Jan 28, 2021

zooba Jan 29, 2021

uranusjr Jan 29, 2021

brettcannon Jan 29, 2021

zooba Feb 3, 2021

zooba Feb 3, 2021

uranusjr Feb 4, 2021

uranusjr Feb 4, 2021

uranusjr commented Feb 4, 2021

brettcannon commented Feb 4, 2021

puetzk commented Feb 5, 2021

puetzk commented Feb 5, 2021 •

edited

Loading

uranusjr commented Feb 5, 2021

ronaldoussoren commented Feb 5, 2021

mattip commented Feb 5, 2021

uranusjr commented Feb 5, 2021 •

edited

Loading

ronaldoussoren commented Feb 5, 2021

puetzk commented Feb 5, 2021 •

edited

Loading

doko42 commented Feb 5, 2021

puetzk commented Feb 5, 2021

brettcannon commented Feb 5, 2021

pradyunsg commented Feb 6, 2021

uranusjr commented Feb 6, 2021

brettcannon commented Feb 6, 2021

uranusjr commented Apr 24, 2021

Remove distutils.util usage from packaging.tags #349

Remove distutils.util usage from packaging.tags #349

Conversation

uranusjr commented Nov 7, 2020

mattip commented Dec 11, 2020

uranusjr commented Dec 11, 2020

puetzk commented Dec 11, 2020 • edited by brettcannon Loading

zooba commented Jan 4, 2021

zooba commented Jan 28, 2021

uranusjr commented Jan 28, 2021

zooba Jan 29, 2021

Choose a reason for hiding this comment

uranusjr Jan 29, 2021

Choose a reason for hiding this comment

brettcannon Jan 29, 2021

Choose a reason for hiding this comment

zooba Feb 3, 2021

Choose a reason for hiding this comment

zooba Feb 3, 2021

Choose a reason for hiding this comment

uranusjr Feb 4, 2021

Choose a reason for hiding this comment

uranusjr Feb 4, 2021

Choose a reason for hiding this comment

uranusjr commented Feb 4, 2021

brettcannon commented Feb 4, 2021

puetzk commented Feb 5, 2021

puetzk commented Feb 5, 2021 • edited Loading

uranusjr commented Feb 5, 2021

ronaldoussoren commented Feb 5, 2021

mattip commented Feb 5, 2021

uranusjr commented Feb 5, 2021 • edited Loading

ronaldoussoren commented Feb 5, 2021

puetzk commented Feb 5, 2021 • edited Loading

doko42 commented Feb 5, 2021

puetzk commented Feb 5, 2021

brettcannon commented Feb 5, 2021

pradyunsg commented Feb 6, 2021

uranusjr commented Feb 6, 2021

brettcannon commented Feb 6, 2021

uranusjr commented Apr 24, 2021

puetzk commented Dec 11, 2020 •

edited by brettcannon

Loading

puetzk commented Feb 5, 2021 •

edited

Loading

uranusjr commented Feb 5, 2021 •

edited

Loading

puetzk commented Feb 5, 2021 •

edited

Loading