-
-
Notifications
You must be signed in to change notification settings - Fork 89
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
Another distro compatibility #64
base: master
Are you sure you want to change the base?
Conversation
Small but important compatibility modification for blackPanther OS and another distro in the future
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.
Thanks for your proposition! See my comments.
@@ -29,6 +29,8 @@ set -E | |||
version="4.2.0" | |||
scriptName=$(basename "$0") | |||
bashVersion=$(echo "$BASH_VERSION" | cut -d. -f1) | |||
# Requres for any other distribution, like blackPanther OS | |||
DISTRO=$(lsb_release -si | awk '{print $1}'| tr [:upper:] [:lower:]) |
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.
Woud rather have a name in camelCase, such as systemDistro
@@ -29,6 +29,8 @@ set -E | |||
version="4.2.0" | |||
scriptName=$(basename "$0") | |||
bashVersion=$(echo "$BASH_VERSION" | cut -d. -f1) | |||
# Requres for any other distribution, like blackPanther OS | |||
DISTRO=$(lsb_release -si | awk '{print $1}'| tr [:upper:] [:lower:]) |
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.
Is there a guarantee lsb_release
would be available in any Linux system? I would rather have a graceful handling for when this command is missing, otherwise the console would be polluted.
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.
Lots of applications and scripts depends on lsb_release command for compatibility issues. So that shouldn't be a problem.
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 bet you're right. However, since one day this tool might be used on MacOS, I'd rather you replace this shell command with
lsb_release -si 2> /dev/null | awk '{print $1}'| tr [:upper:] [:lower:]
There is likely good inspiration to have from https://metacpan.org/pod/distribution/Devel-CheckOS/lib/Devel/CheckOS/Families.pod#THE-Linux::Debian-FAMILY and other parts of that codebase |
...on the reliability of certain classifications and their various methods of probing, I mean |
Small but important compatibility modification for blackPanther OS and another distro in the future
name: Pull Request
about: 'Contribute to the project'
title: ''
assignees: jsamr