yet-another-bench-script icon indicating copy to clipboard operation
yet-another-bench-script copied to clipboard

Add check for commands like curl, wget etc. and fix some linter warnings

Open hugmouse opened this issue 1 year ago • 4 comments
trafficstars

Description

Just added a check for commands that are in the system, list them and show which do we have and not. If we don't have curl or wget - then don't run this script at all (maybe we should anyway?) Also fixed some warnings that https://www.shellcheck.net/ reports along the way.

Linter issues

Fixed some of them from https://www.shellcheck.net/ output:

  1. SC2015 (info): Lines 119, 120, 123, 126, 254 - Note that A && B || C is not if-then-else. C may run when A is true.
  2. SC2236 (style): Lines 129, 138, 168, 169, 170, 171, 172, 173, 174, 175, 176, 187, 189, 255, 261, 326, 371, 374, 376, 377, 627, 631, 659, 690, 705, 715, 791, 833, 844, 852, 887, 900, 902, 904, 927, 991, 1004, 1007, 1010, 1017 - Use -n instead of ! -z.
  3. SC2002 (style): Lines 271, 274 - Useless cat. Consider using cmd < file or cmd file instead.
  4. SC2155 (warning): Lines 328, 329, 330, 332, 339, 340, 341, 342, 343, 344, 345 - Declare and assign separately to avoid masking return values.
  5. SC2329 (info): Line 408 - The function is never invoked. Check usage (or ignored if invoked indirectly).
  6. SC2004 (style): Lines 556, 754, 757, 782, 785, 1016 - $/${} is unnecessary on arithmetic variables.
  7. SC2068 (error): Line 588 - Double quote array expansions to avoid re-splitting elements.
  8. SC1143 (error): Line 876 - This backslash is part of a comment and does not continue the line.
  9. SC2183 (warning): Lines 685, 699 - This format string has more variables than are passed as arguments.
  10. SC2046 (warning): Lines 667, 673, 967 - Quote this to prevent word splitting.
  11. SC2116 (style): Lines 797, 827, 1016 - Useless echo. Instead of cmd $(echo foo), just use cmd foo.
  12. SC2003 (style): Lines 691, 713 - expr is antiquated. Consider rewriting this using $((..)), ${} or [[ ]].

Other fixes

  • Added overlay type into TOTAL_DISK_RAW variable to calculate available space if we are running inside of Docker.

Examples

Example output of x64 Alpine Edge from Dockerhub:

Checking available commands
---------------------------------
locale               : ❌ not installed
uname                : ✔  installed
getconf              : ✔  installed
awk                  : ✔  installed
sed                  : ✔  installed
grep                 : ✔  installed
cut                  : ✔  installed
shuf                 : ✔  installed
timeout              : ✔  installed
date                 : ✔  installed
trap                 : ✔  installed
df                   : ✔  installed
free                 : ✔  installed
systemd-detect-virt  : ❌ not installed
wget                 : ✔  installed

x64 Ubuntu Focal from Dockerhub:

Checking available commands
---------------------------------
locale               : ✔  installed
uname                : ✔  installed
getconf              : ✔  installed
awk                  : ✔  installed
sed                  : ✔  installed
grep                 : ✔  installed
cut                  : ✔  installed
shuf                 : ✔  installed
timeout              : ✔  installed
date                 : ✔  installed
trap                 : ✔  installed
df                   : ✔  installed
free                 : ✔  installed
systemd-detect-virt  : ❌ not installed
curl/wget            : ❌ not installed

Error: Neither 'curl' nor 'wget' command found. Please install one of those to continue.

x64 Photon 5.0:

Checking available commands
---------------------------------
locale               : ✔  installed
uname                : ✔  installed
getconf              : ✔  installed
sed                  : ✔  installed
grep                 : ✔  installed
cut                  : ✔  installed
shuf                 : ❌ not installed
timeout              : ✔  installed
date                 : ✔  installed
trap                 : ✔  installed
df                   : ✔  installed
free                 : ✔  installed
systemd-detect-virt  : ❌ not installed
curl                 : ✔  installed
awk                  : ❌ not installed

Error: 'awk' command found. Please install one of those to continue, script heavily relies on it.

x64 Clearlinux latest, Mageia 9:

Checking available commands
---------------------------------
locale               : ✔  installed
uname                : ✔  installed
getconf              : ✔  installed
sed                  : ✔  installed
grep                 : ✔  installed
cut                  : ✔  installed
shuf                 : ✔  installed
timeout              : ✔  installed
date                 : ✔  installed
trap                 : ✔  installed
df                   : ✔  installed
free                 : ✔  installed
systemd-detect-virt  : ✔  installed
curl                 : ✔  installed
awk                  : ✔  installed

hugmouse avatar Jun 19 '24 16:06 hugmouse

Would be awesome if someone tested this revision in various environments. Tested on x64 Debian, Ubuntu and Alpine - works somewhat fine in there.

There's issues with default wget in Alpine though, it doesn't have -w option: wget: unrecognized option: w, so script breaks in that case. If you install wget from Alpine's repos though it works.

hugmouse avatar Jun 19 '24 16:06 hugmouse

This is fantastic, thank you so much for the PR!

I took a quick look and ran it to check the outputs. I'll take a deeper look soon, but for now I would suggest one change before we merge -- the command check output takes up a lot of real estate in the script output. I'd like to move all those checks to only print if the "help" flag (-h) is set. Otherwise, it should still do the checks but maybe only warn if there's a breaking issue (i.e. curl and wget both aren't installed, etc.).

masonr avatar Jun 21 '24 20:06 masonr

I will convert this PR into Draft for now and will add some more code later this week, feel free to suggest some more stuff or add code directly to this pr

Upd 1: actually will do a little bit later, got sick :mask:

hugmouse avatar Jun 23 '24 19:06 hugmouse

I'm alive again. Added -c flag for my stuff, any suggestions @masonr?

hugmouse avatar Jul 27 '24 14:07 hugmouse

Just wanted to do something similar. I've just ordered a couple of VPS servers to do some comparisons. The Geekbench test failed for me. But it was a very simple error: tar was simply not installed by default.

Precisely because the README says the following:

The script is designed to not require any external dependencies to be installed nor elevated privileges to run.

Nevertheless, all dependencies should be checked. Whether you then offer that they should be installed by the script itself or whether there is simply a note that you need this beforehand would be perfectly sufficient.

And before anyone asks: It's the AlmaLinux 9.5 distro that doesn't have it out of the box :D

sherlock7402 avatar Nov 19 '24 23:11 sherlock7402

@dxniel7402 done, can you check (just in case) on AlmaLinux again?

Using almalinux:latest from DockerHub:

Checking available commands
---------------------------------
locale               : ✔  installed
uname                : ✔  installed
getconf              : ✔  installed
sed                  : ✔  installed
grep                 : ✔  installed
cut                  : ✔  installed
shuf                 : ✔  installed
timeout              : ✔  installed
date                 : ✔  installed
trap                 : ✔  installed
df                   : ✔  installed
free                 : ❌ not installed
systemd-detect-virt  : ✔  installed
tar                  : ✔  installed
curl                 : ✔  installed
awk                  : ✔  installed
tar                  : ✔  installed

hugmouse avatar Nov 20 '24 13:11 hugmouse

@dxniel7402 done, can you check (just in case) on AlmaLinux again?

Using almalinux:latest from DockerHub:

Checking available commands
---------------------------------
locale               : ✔  installed
uname                : ✔  installed
getconf              : ✔  installed
sed                  : ✔  installed
grep                 : ✔  installed
cut                  : ✔  installed
shuf                 : ✔  installed
timeout              : ✔  installed
date                 : ✔  installed
trap                 : ✔  installed
df                   : ✔  installed
free                 : ❌ not installed
systemd-detect-virt  : ✔  installed
tar                  : ✔  installed
curl                 : ✔  installed
awk                  : ✔  installed
tar                  : ✔  installed

Looks like this now:

# curl -sL https://raw.githubusercontent.com/hugmouse/bench-bash/refs/heads/master/yabs.sh | bash
# ## ## ## ## ## ## ## ## ## ## ## ## ## ## ## ## ## #
#              Yet-Another-Bench-Script              #
#                     v2024-06-09                    #
# https://github.com/masonr/yet-another-bench-script #
# ## ## ## ## ## ## ## ## ## ## ## ## ## ## ## ## ## #
tar                  : ❌ not installed
tar                  : ❌ not installed

Error: 'tar' command not found. Please install it to continue, YABS needs it to be able to open Geekbench.tar.gz.

You are checking this twice:

  1. https://github.com/hugmouse/bench-bash/blob/97ff7b5b445c0a4ce0140906ff65dcc456285f40/yabs.sh#L55-L71
  2. https://github.com/hugmouse/bench-bash/blob/97ff7b5b445c0a4ce0140906ff65dcc456285f40/yabs.sh#L102-L112

Actually, you're checking the most twice. Let me have a bigger look at your code ^^

sherlock7402 avatar Nov 20 '24 21:11 sherlock7402

I need to completely refactor my changes I think lol, I've changed toooooooooooo much. I will probably come back to this with simpler changes in separate PR to not change so many things at the same time.

I actually removed a lot of code from the benchmark section, and I can't even remember why at this point - I think Geekbench changed the way how they print stuff to stdout, so some stuff just broke.

hugmouse avatar Nov 20 '24 21:11 hugmouse

@hugmouse

I reviewed the current implementation for checking dependencies, and I noticed several opportunities for optimization to make the script more efficient and maintainable. Here's my proposal:

Proposal: Organize Dependencies into Arrays

We can refactor the script to categorize dependencies into two arrays: NECESSARY_COMMANDS and OPTIONAL_COMMANDS. Each array will include both the command and a short description of its purpose. This approach ensures:

  1. Better readability and maintainability.
  2. Clearer feedback on missing dependencies.
  3. Elimination of redundancy in checks.

Refactored Code:

!!! Not tested, just refactored a bit with GH CoPilot !!!

# Array for necessary commands with descriptions
NECESSARY_COMMANDS=(
    "curl:Download external files (mandatory for script execution)"
    "tar:Extract compressed files (needed for Geekbench archive)"
    "awk:Text processing and data extraction"
    "locale:Ensure correct locale settings"
    "uname:Determine system architecture"
    "getconf:Get kernel bit-width (e.g., for ARM systems)"
    "sed:Text processing"
    "grep:Text searching"
    "cut:Text splitting"
    "df:Check available disk space"
    "date:Generate timestamps"
    "trap:Handle script interruptions and cleanup"
)

# Array for optional commands with descriptions
OPTIONAL_COMMANDS=(
    "shuf:Select random ports for network tests"
    "timeout:Limit execution time for commands"
    "free:Retrieve RAM and swap details"
    "systemd-detect-virt:Detect virtualization environment"
    "ping:Measure network latency"
    "fio:Run disk performance tests"
    "iperf3:Run network performance tests"
)

# Function to check command availability
check_command() {
    command -v "$1" &>/dev/null
}

# Check necessary commands
echo -e "\nChecking necessary commands:"
for entry in "${NECESSARY_COMMANDS[@]}"; do
    cmd="${entry%%:*}"       # Extract command
    desc="${entry#*:}"       # Extract description
    if check_command "$cmd"; then
        printf "%-20s : ✅  %s\n" "$cmd" "Available"
    else
        printf "%-20s : ❌  %s\n" "$cmd" "$desc"
        echo -e "Error: '$cmd' is missing. $desc."
        exit 1
    fi
done

# Check optional commands
echo -e "\nChecking optional commands:"
for entry in "${OPTIONAL_COMMANDS[@]}"; do
    cmd="${entry%%:*}"       # Extract command
    desc="${entry#*:}"       # Extract description
    if check_command "$cmd"; then
        printf "%-20s : ✅  %s\n" "$cmd" "Available"
    else
        printf "%-20s : ⚠️  %s\n" "$cmd" "$desc (Optional, but recommended)"
    fi
done

Benefits of This Approach:

  1. Structured Dependency Check: Dependencies are categorized and easy to expand or modify.
  2. Clear Feedback: The script provides concise feedback about missing dependencies and their purposes.
  3. Simpler Maintenance: The description directly in the array eliminates the need for scattered comments or additional documentation.
  4. Eliminates Redundancy: The previous logic for separate and redundant checks (e.g., tar or curl) is streamlined into a single structure.

What Can Be Removed:

  • Redundant separate checks (e.g., tar, curl, awk).
  • Variables like MISSING_CMDS, since the script exits immediately when a necessary dependency is missing.
  • The previous REQUIRED_COMMANDS logic, as all critical commands are now in NECESSARY_COMMANDS.

While writing this, I noticed that you already closed the PR. That’s totally fine — feel free to take this idea and use it however you like. If you’re interested, I’d love to collaborate on your fork. I could help with issues and contribute a bit. Since neither of us has permissions here, and I’d rather not start a parallel project, we could join forces on your fork and put our brains together! 🚀

sherlock7402 avatar Nov 20 '24 22:11 sherlock7402