Detect distro from arbitrary rootfs root_dir #161#247
Detect distro from arbitrary rootfs root_dir #161#247nir0s merged 6 commits intopython-distro:masterfrom
Conversation
This introduce a new optional root_dir argument to contruct a LinuxDistribution object. When provided, this is used as if it were the root of the filesystem when looking up for files. Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
|
The command line interface driven by |
|
@hartwork re
Would you like an option?
I could not agree more. Having a global created on import here does not make sense to me https://github.com/nir0s/distro/blob/cdfe85d15bd366820db6a1cfdc6cf9a0a5de7e37/distro.py#L1189 A lot of the module api could be made simpler IMHO. |
That would be plain awesome, yes 😄 |
Also fix the root_dir arg of LinuxDistribution() to be an absolute path to the root of a filesystem and not a path to /etc Reported-by: Sebastian Pipping <sebastian@pipping.org> @hartwork Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
pombredanne
force-pushed
the
161-root-dir-arg
branch
from
3d3f69e to
ab9d3c1
Compare
|
@hartwork there you go the latest push has a cli option now |
hartwork
left a comment
hartwork
left a comment
There was a problem hiding this comment.
I like where this is going! Two small comments below:
tests/test_distro.py
Outdated
| } | ||
| } | ||
| ''' | ||
| desired_output = json.loads(desired_output) |
There was a problem hiding this comment.
I noticed we're parsing hard-coded JSON here while then comparing high-level objects. How about removing the JSON indirection layer by going straight to Python nested dictionaries:
desired_output = {
'codename': '',
'id': 'fedora',
'like': '',
'version': '30',
'version_parts': {
'build_number': '',
'major': '30',
'minor': '',
},
}
As a side-effect, we can now re-introduce trailing commas for better diffs if those lines are touched in the future. What do you think?
There was a problem hiding this comment.
Sure, I was a tad lazy for this. !
tests/test_distro.py
Outdated
| root_dir = os.path.join(DISTROS_DIR, dist) | ||
| self.distro = distro.LinuxDistribution( | ||
| os_release_file='', | ||
| distro_release_file='non', |
There was a problem hiding this comment.
Why does it say 'non' here?
There was a problem hiding this comment.
That's something I carried over from the test class I subclass here https://github.com/nir0s/distro/blob/ab9d3c1fa7fcc71056700841b6a273b18af20b6d/tests/test_distro.py#L151
I scratched my head as this does not make sense. I think this is meant to be a non-existing release file, but I did not dig why you would want that. For the sake of simplicity I kept that as otherwise the tests are not behaving the same way as if you were passing no distro_release_file at all. ... which is really weird as this should not matter. @nir0s any clue?
There was a problem hiding this comment.
I dug a bit in the code as well just now: There is special handling for distro_release_file being "true" — if self.distro_release_file: — and the method _parse_distro_release_file swallows all file access errors in silence. So non seems more intentional to me now but I'd use a filename like no_such_file or so to better communicate that intention.
Using explicit named arguments when creating a LinuxDistribution makes the tests more readable than using positional unnamed arguments. Using 'path-to-non-existing-file' as a variable value for non-existing test file paths is easier to understand than a terse 'non' value. Reported-by: Sebastian Pipping <sebastian@pipping.org> Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
This is easier to read and maintain and more resilient to formatting changes. Reported-by: Sebastian Pipping <sebastian@pipping.org> Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
hartwork
left a comment
hartwork
left a comment
There was a problem hiding this comment.
Looks pretty good to me 👍
|
@nir0s ping? Do you mind to review this? Thanks! |
|
I've been away. I'll find time to review this soon. Thanks for this! |
|
ping ... this is now stale.. I will rebase |
|
@nir0s I updated to the latest master. |
|
@nir0s If you can tell if you may or may not merge that, it would be useful. If not I will have to maintain my own fork and push it as distro2 to PyPI. |
|
I see no reason not to get this in. I'll give it a look during the weekend. |
|
@nir0s Thank you ++! 🙇 |
3 participants
This introduce a new optional root_dir argument to contruct a
LinuxDistribution object. When provided, this is used as if it were
the root of the filesystem when looking up for files.
This also adds a --root-dir option to the command line.
This is a fix for #161
Signed-off-by: Philippe Ombredanne pombredanne@nexb.com