-
Notifications
You must be signed in to change notification settings - Fork 65
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
Detect distro from arbitrary rootfs root_dir #161 #247
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>
3d3f69e
to
ab9d3c1
Compare
@hartwork there you go the latest push has a cli option now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it say 'non'
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ++! 🙇 |
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