bpo-28468: Add platform.freedesktop_osrelease #23492
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Christian Heimes <christian@python.org>
|
issue28468 is marked as won't fix. plus we have the distro module https://github.com/nir0s/distro for that reason. what exactly did change that we need to re-introduce that functionality? |
|
I have re-opened my issue along with this PR. Amongst others we need the feature to work around broken tests on specific platforms. |
|
it's almost always wrong to work around broken tests by checking a particular release. I'm happy to see that the distro specific tests in the ssl module are gone. why introduce new ones? |
doko42
requested changes
Nov 24, 2020
There was a problem hiding this comment.
It took a while to remove the platform information (platform.linux_distribution). I don't think that re-adding this in another form is rectified.
Relying on a distro name and/or release name is a bad style, which doesn't cover in it's proposed form all targes. Usually these checks can be done, and should be based on feature tests done by the configure step.
Also exposing this function as a Python standard API seems to be wrong, as there is a fine third party module for that purpose: https://pypi.org/project/distro/
|
When you're done making the requested changes, leave the comment: |
vstinner
reviewed
Nov 24, 2020
Lib/test/test_platform.py
Outdated
| platform.freedesktop_osrelease() | ||
|
|
||
| def test_parse_osrelease(self): | ||
| info = platform._parse_osrelease(FEDORA_OSRELEASE.split("\n")) |
There was a problem hiding this comment.
Remark: For the purpose of the unit test, .splitlines() would also work, and it's less strict, no?
Lib/test/test_platform.py
Outdated
| @@ -8,12 +8,63 @@ | |||
| from test import support | |||
| from test.support import os_helper | |||
|
|
|||
| FEDORA_OSRELEASE = """\ | |||
There was a problem hiding this comment.
The trailing \ is surprising. Is it needed? Maybe write """...""".lstrip()?
|
@doko42: For practical reasons, would you mind to keep the PR to discuss the actual implementation, and discuss the rationale and need of the feature at https://bugs.python.org/issue28468 ? Thanks ;-) It's just that I hate have a discussion going at two different places and the issue has a longer history. |
vstinner
reviewed
Nov 24, 2020
* change name to freedesktop_os_release * add default PRETTY_NAME * exception handling without refleak * more tests * mention Android has no os-release
vstinner
reviewed
Nov 24, 2020
Co-authored-by: Victor Stinner <vstinner@python.org>
vstinner
reviewed
Nov 24, 2020
Lib/platform.py
Outdated
| if 'ID_LIKE' in info: | ||
| info['ID_LIKE'] = tuple( | ||
| id_like for id_like in info['ID_LIKE'].split(' ') if id_like | ||
| ) |
There was a problem hiding this comment.
I suggest to remove this code.
I'm not sure if it's worth it to have a special case only for the ID_LIKE variable. The platform module is mostly based on strings. You introduce a special case where a single variable is a tuple. Since it's well specified that it's space separated, the caller can split the string by themselves, no?
Splitting at spaces it trivial in Python:
>>> "debian ubuntu".split()
['debian', 'ubuntu']
There was a problem hiding this comment.
This is the only special case that a lot of users will get wrong and that won't get detected by CI. For almost all distros the ID_LIKE is either not present or it is a single entry, often debian or fedora. For example Ubuntu has ID_LIKE=debian. Only very few distros have an ID_LIKE with multiple entries.
Users always have to split the field and always check for "name in info["ID_LIKE"].split()". The explicit handling of ID_LIKE get rids of a foot gun.
There was a problem hiding this comment.
The problem is that if tomorrow a new field is added with a value separated by spaces, the caller might expect Python to magically split it as a tuple. But now, we will have to update the function whenever a new field is added for add a special case.
If there is no special case and it's a dummy parser, the code is more likely to stay for the same for the next 10 years.
I would prefer to not reintroduce the same problem that we fixed by removing linux_distribution(): having to update the stdlib when the filename changes or file content evolves.
If you want a more advanced view of the os-release, I suggest to write a higher level API on top of it and put it on PyPI, so it can be easily updated (similar to the distro module which contains many special cases).
vstinner
reviewed
Nov 24, 2020
| } | ||
|
|
||
| for line in lines: | ||
| mo = _os_release_line.match(line) |
There was a problem hiding this comment.
The distro module uses shlex.shlex() to parse os-release.
lexer = shlex.shlex(lines, posix=True)
lexer.whitespace_split = True
The freedesktop specification says:
Shell special characters ("$", quotes, backslash, backtick) must be escaped with backslashes, following shell style.
It seems like your code and the distro module parse \" and \' differently for example.
There was a problem hiding this comment.
Something like:
NAME='Fedora 19 (Schrödinger\'s Cat)'
Note: I wrote the example manually, it doesn't come from a concrete os-release file.
vstinner
reviewed
Nov 25, 2020
| DOUBLE_QUOTE="double" | ||
| EMPTY_DOUBLE="" | ||
| QUOTES="double\'s" | ||
| SPECIALS="\$\`\\\'\"" |
There was a problem hiding this comment.
Would you mind to add a comment explaining that the format requires these 5 characters to be escaped with a blackslash? Maybe rename SPECIALS to ESCAPED.
| "SINGLE_QUOTE": "single", | ||
| "EMPTY_SINGLE": "", | ||
| "QUOTES": "double's", | ||
| "SPECIALS": "$`\\'\"", |
There was a problem hiding this comment.
Maybe write this specific test to make it more readable:
Suggested change
| "SPECIALS": "$`\\'\"", | |
| "SPECIALS": ''.join(['$', '`', '\\', "'", '"']), |
So you can remove self.assertEqual(len(info["SPECIALS"]), 5).
Documentation now shows how to use ID_LIKE correctly.
vstinner
approved these changes
Nov 25, 2020
|
| # unescape five special characters mentioned in the standard | ||
| _os_release_unescape = re.compile(r"\\([\\\$\"\'`])") | ||
| # /etc takes precedence over /usr/lib | ||
| _os_release_candidates = ("/etc/os-release", "/usr/lib/os-relesase") |
There was a problem hiding this comment.
Suggested change
| _os_release_candidates = ("/etc/os-release", "/usr/lib/os-relesase") | |
| _os_release_candidates = ("/etc/os-release", "/usr/lib/os-release") |
There was a problem hiding this comment.
Ooops, nicely spotted :-) I created PR #23913 to fix it.
adorilson
pushed a commit
to adorilson/cpython
that referenced
this pull request
Mar 13, 2021
Add platform.freedesktop_os_release() function to parse freedesktop.org os-release files. Signed-off-by: Christian Heimes <christian@python.org> Co-authored-by: Victor Stinner <vstinner@python.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Christian Heimes christian@python.org
https://bugs.python.org/issue28468