Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blackPantherOS
Copy link

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


Small but important compatibility modification for blackPanther OS and another distro in the future
Copy link
Owner

@jsamr jsamr left a 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:])
Copy link
Owner

@jsamr jsamr Feb 13, 2021

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:])
Copy link
Owner

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.

Copy link
Contributor

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.

Copy link
Owner

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:]

@jonassmedegaard
Copy link

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

@jonassmedegaard
Copy link

...on the reliability of certain classifications and their various methods of probing, I mean

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants