bpo-33435: Fixed incorrect version detection of linux in some distrib… #6719
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. When your account is ready, please add a comment in this pull request Thanks again to your contribution and we look forward to looking at it! |
mrandybu
force-pushed
the
fix-issue-33435
branch
2 times, most recently
from
8e314db to
03266cb
Compare
mrandybu
force-pushed
the
fix-issue-33435
branch
6 times, most recently
from
7bb1e9d to
aee6d6c
Compare
Lib/platform.py
Outdated
| @@ -134,7 +134,11 @@ | |||
|
|
|||
| # Directory to search for configuration information on Unix. | |||
| # Constant used by test_platform to test linux_distribution(). | |||
| _UNIXCONFDIR = '/etc' | |||
| try: | |||
| _UNIXCONFDIR = '/etc/os-release' | |||
There was a problem hiding this comment.
Is /etc/os-release a directory in some distributives?
There was a problem hiding this comment.
/etc has os-release file in some linux distributions
There was a problem hiding this comment.
Naming a path of the regular file _UNIXCONFDIR looks confusing.
Lib/platform.py
Outdated
| _UNIXCONFDIR = '/etc' | ||
| try: | ||
| _UNIXCONFDIR = '/etc/os-release' | ||
| open(_UNIXCONFDIR) |
There was a problem hiding this comment.
ResourceWarning will be emitted here.
In general, performing filesystem operations at import time can be considered a bad style.
There was a problem hiding this comment.
here i define _UNIXCONFDIR because test_platform import it variable, and if os-release directory will be defined in function, test_platform finished with error.
There was a problem hiding this comment.
Wouldn't be better to search the 'os-release' file in the _UNIXCONFDIR directory similarly to searching other config files?
There was a problem hiding this comment.
If search os-release file using an existing method, it is necessary to make a lot of changes to it, since the os-release file after sorting is in the last places and therefore it is not using in the future. I think that can find, if not, then look for the rest of the files. Now I have remade the commit in accordance with your remarks.
Lib/platform.py
Outdated
| etc = os.listdir(_UNIXCONFDIR) | ||
| except OSError: | ||
| # Probably not a Unix system | ||
| _os_release = open(_UNIXCONFDIR) |
There was a problem hiding this comment.
ResourceWarning will be emitted here.
Why you use underscored names for local variables?
Lib/platform.py
Outdated
| except OSError: | ||
| # Probably not a Unix system | ||
| _os_release = open(_UNIXCONFDIR) | ||
| _os_release_content = _os_release.read() |
There was a problem hiding this comment.
Can fail if the encoding of the file doesn't match the locale encoding.
Lib/platform.py
Outdated
| # Probably not a Unix system | ||
| _os_release = open(_UNIXCONFDIR) | ||
| _os_release_content = _os_release.read() | ||
| get_name = re.findall(r'NAME=.*', _os_release_content)[0] |
There was a problem hiding this comment.
Why not just re.search() if you use only the first found match? What if the patter will be not found in the file?
The pattern r'NAME=.*' gives false positive for e.g. VERSION_CODENAME=.
Lib/platform.py
Outdated
| break | ||
| else: | ||
| return _dist_try_harder(distname, version, id) | ||
| except IOError: |
There was a problem hiding this comment.
IOError is a deprecated alias to OSError.
…utions (platform.py)
|
what replaced linux_distribution? |
In some linux distributions, the information about the distribution is incorrectly determined when the linux_distribution() method is called from the platform class. Since the information file os-release becomes a certain standard, I propose first of all to check its availability and if it is in the system, then parse the information from it. I apply the patch. It will work well with version 2.7.
https://bugs.python.org/issue33435