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

util.get_host_platform: use sysconfig.get_platform #102

Merged
merged 1 commit into from
Jan 3, 2022

Conversation

naveen521kk
Copy link
Contributor

sysconfig module has a copy of this function in
sysconfig.get_platform and would be better to
use that directly. This should help distributors
to patch that function in a single place,
ie sysconfig.

Co-authored-by: Christoph Reiter [email protected]
Signed-off-by: Naveen M K [email protected]

sysconfig module has a copy of this function in
`sysconfig.get_platform` and would be better to
use that directly. This should help distributors
to patch that function in a single place,
ie sysconfig.

Co-authored-by:  Christoph Reiter <[email protected]>
Signed-off-by: Naveen M K <[email protected]>
@lazka
Copy link
Contributor

lazka commented Dec 31, 2021

For context: the mingw build defines it's own platform since the C extensions are not compatible with the official CPython ones, so we extend sysconfig.get_platform(). By distutils reusing it, things will just "work".

Here is the diff between stdlib version (current main) and distutils version:

--- stdlib.txt	2021-12-31 09:15:30.626663600 +0100
+++ distutils.txt	2021-12-31 09:15:32.808358800 +0100
@@ -1,10 +1,10 @@
-def get_platform():
-    """Return a string that identifies the current platform.
-    This is used mainly to distinguish platform-specific build directories and
-    platform-specific built distributions.  Typically includes the OS name and
-    version and the architecture (as supplied by 'os.uname()'), although the
-    exact information included depends on the OS; on Linux, the kernel version
-    isn't particularly important.
+def get_host_platform():
+    """Return a string that identifies the current platform.  This is used mainly to
+    distinguish platform-specific build directories and platform-specific built
+    distributions.  Typically includes the OS name and version and the
+    architecture (as supplied by 'os.uname()'), although the exact information
+    included depends on the OS; eg. on Linux, the kernel version isn't
+    particularly important.
     Examples of returned values:
        linux-i586
        linux-alpha (?)
@@ -23,16 +23,18 @@
             return 'win-arm64'
         return sys.platform
 
-    if os.name != "posix" or not hasattr(os, 'uname'):
-        # XXX what about the architecture? NT is Intel or Alpha
-        return sys.platform
-
     # Set for cross builds explicitly
     if "_PYTHON_HOST_PLATFORM" in os.environ:
         return os.environ["_PYTHON_HOST_PLATFORM"]
 
+    if os.name != "posix" or not hasattr(os, 'uname'):
+        # XXX what about the architecture? NT is Intel or Alpha,
+        # Mac OS is M68k or PPC, etc.
+        return sys.platform
+
     # Try to distinguish various flavours of Unix
-    osname, host, release, version, machine = os.uname()
+
+    (osname, host, release, version, machine) = os.uname()
 
     # Convert the OS name to lowercase, remove '/' characters, and translate
     # spaces (for "Power Macintosh")
@@ -44,31 +46,30 @@
         # At least on Linux/Intel, 'machine' is the processor --
         # i386, etc.
         # XXX what about Alpha, SPARC, etc?
-        return  f"{osname}-{machine}"
+        return  "%s-%s" % (osname, machine)
     elif osname[:5] == "sunos":
         if release[0] >= "5":           # SunOS 5 == Solaris 2
             osname = "solaris"
-            release = f"{int(release[0]) - 3}.{release[2:]}"
+            release = "%d.%s" % (int(release[0]) - 3, release[2:])
             # We can't use "platform.architecture()[0]" because a
             # bootstrap problem. We use a dict to get an error
             # if some suspicious happens.
             bitness = {2147483647:"32bit", 9223372036854775807:"64bit"}
-            machine += f".{bitness[sys.maxsize]}"
+            machine += ".%s" % bitness[sys.maxsize]
         # fall through to standard osname-release-machine representation
     elif osname[:3] == "aix":
-        from _aix_support import aix_platform
-        return aix_platform()
+        from .py38compat import aix_platform
+        return aix_platform(osname, version, release)
     elif osname[:6] == "cygwin":
         osname = "cygwin"
-        import re
-        rel_re = re.compile(r'[\d.]+')
+        rel_re = re.compile (r'[\d.]+', re.ASCII)
         m = rel_re.match(release)
         if m:
             release = m.group()
     elif osname[:6] == "darwin":
-        import _osx_support
+        import _osx_support, distutils.sysconfig
         osname, release, machine = _osx_support.get_platform_osx(
-                                            get_config_vars(),
-                                            osname, release, machine)
+                                        distutils.sysconfig.get_config_vars(),
+                                        osname, release, machine)
 
-    return f"{osname}-{release}-{machine}"
+    return "%s-%s-%s" % (osname, release, machine)

@jaraco
Copy link
Member

jaraco commented Jan 3, 2022

Nice. Thanks for the contrib.

@jaraco jaraco merged commit cf8c612 into pypa:main Jan 3, 2022
@jaraco
Copy link
Member

jaraco commented Jan 3, 2022

Ah, shoot. I probably should have thought through this more before merging.

If I paste the Python 3.7 sysconfig.get_platform over get_host_platform prior to the commit above, it shows this diff:

diff --git a/distutils/util.py b/distutils/util.py
index ac6d446d68..9446683920 100644
--- a/distutils/util.py
+++ b/distutils/util.py
@@ -17,13 +17,14 @@ from distutils.errors import DistutilsByteCompileError
 from .py35compat import _optim_args_from_interpreter_flags
 
 
-def get_host_platform():
-    """Return a string that identifies the current platform.  This is used mainly to
-    distinguish platform-specific build directories and platform-specific built
-    distributions.  Typically includes the OS name and version and the
-    architecture (as supplied by 'os.uname()'), although the exact information
-    included depends on the OS; eg. on Linux, the kernel version isn't
-    particularly important.
+def get_platform():
+    """Return a string that identifies the current platform.
+
+    This is used mainly to distinguish platform-specific build directories and
+    platform-specific built distributions.  Typically includes the OS name and
+    version and the architecture (as supplied by 'os.uname()'), although the
+    exact information included depends on the OS; on Linux, the kernel version
+    isn't particularly important.
 
     Examples of returned values:
        linux-i586
@@ -40,24 +41,18 @@ def get_host_platform():
     if os.name == 'nt':
         if 'amd64' in sys.version.lower():
             return 'win-amd64'
-        if '(arm)' in sys.version.lower():
-            return 'win-arm32'
-        if '(arm64)' in sys.version.lower():
-            return 'win-arm64'
+        return sys.platform
+
+    if os.name != "posix" or not hasattr(os, 'uname'):
+        # XXX what about the architecture? NT is Intel or Alpha
         return sys.platform
 
     # Set for cross builds explicitly
     if "_PYTHON_HOST_PLATFORM" in os.environ:
         return os.environ["_PYTHON_HOST_PLATFORM"]
 
-    if os.name != "posix" or not hasattr(os, 'uname'):
-        # XXX what about the architecture? NT is Intel or Alpha,
-        # Mac OS is M68k or PPC, etc.
-        return sys.platform
-
     # Try to distinguish various flavours of Unix
-
-    (osname, host, release, version, machine) = os.uname()
+    osname, host, release, version, machine = os.uname()
 
     # Convert the OS name to lowercase, remove '/' characters, and translate
     # spaces (for "Power Macintosh")
@@ -81,22 +76,23 @@ def get_host_platform():
             machine += ".%s" % bitness[sys.maxsize]
         # fall through to standard osname-release-machine representation
     elif osname[:3] == "aix":
-        from .py38compat import aix_platform
-        return aix_platform(osname, version, release)
+        return "%s-%s.%s" % (osname, version, release)
     elif osname[:6] == "cygwin":
         osname = "cygwin"
-        rel_re = re.compile (r'[\d.]+', re.ASCII)
+        import re
+        rel_re = re.compile(r'[\d.]+')
         m = rel_re.match(release)
         if m:
             release = m.group()
     elif osname[:6] == "darwin":
-        import _osx_support, distutils.sysconfig
+        import _osx_support
         osname, release, machine = _osx_support.get_platform_osx(
-                                        distutils.sysconfig.get_config_vars(),
-                                        osname, release, machine)
+                                            get_config_vars(),
+                                            osname, release, machine)
 
     return "%s-%s-%s" % (osname, release, machine)
 
+
 def get_platform():
     if os.name == 'nt':
         TARGET_TO_PLAT = {

@jaraco
Copy link
Member

jaraco commented Jan 3, 2022

Crucially, in that diff, I can see the loss of AIX support and ARM support. That means that applying this patch means loss of that functionality for those platforms/Python combinations. I'm going to back out the merge of the change.

@jaraco
Copy link
Member

jaraco commented Jan 3, 2022

In main, I've force-pushed the commit prior to this merge:

distutils main $ git push -f
Total 0 (delta 0), reused 0 (delta 0), pack-reused 0
To https://github.com/pypa/distutils
 + cf8c61254d...1560a1fe4e main -> main (forced update)

Please consider re-submitting the PR after addressing the concern in the prior comment.

@lazka
Copy link
Contributor

lazka commented Jan 3, 2022

Don't you need to diff it against the distutils function from 3.7 as well? That's the combination everyone was using until the setuptools switch and that is missing those aix/arm cases as well.

https://github.com/python/cpython/blob/3.7/Lib/distutils/util.py#L18

Or am I missing something?

I can look into adding back the support though, if wanted.

@jaraco
Copy link
Member

jaraco commented Jan 3, 2022

With the Setuptools adoption of this standalone distutils, it means that users even on Python 3.7 (and until very recently 3.6) would get the behavior from this repo. As a result, users on Python 3.7 might have come to expect behaviors from newer distutils to be present. In general, Setuptools w/ distutils attempts to provide a uniform feature set across all supported Python versions.

That said, it's conceivable that users of ARM and AIX are accustomed to those Pythons being unsupported, so may be unaffected in practice. If someone wants to make that assertion, we could accept the change as proposed. I'd feel slightly more comfortable with retaining the backported functionality, but I don't feel strongly about it unless a user from those platforms complains.

@lazka
Copy link
Contributor

lazka commented Jan 3, 2022

ok, fair enough, thanks for the explanation.

@naveen521kk naveen521kk deleted the get-platform-match-sysconfig branch January 4, 2022 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants