Open coderabbitai[bot] opened 2 weeks ago
reactive-firewall
commented
1 day ago @coderabbitai, please explore the idea proposed by this issue ( #126 ), considering CEP-5 and PR/MR #118 discussions about test -x styles in bash scripts. Also please read the files tests/check_* looking for opportunities to implement the idea.
coderabbitai[bot]
commented
1 day ago [!TIP] For best results, initiate chat on the files or code changes.
@reactive-firewall, I have explored the idea proposed in issue #126, considering the discussions in CEP-5 and PR #118 regarding test -x styles in bash scripts. Here's a summary of my findings:
CEP-5 proposes standardizing the use of a custom shlock implementation (tool_shlock_helper.sh) across all shell scripts in the project. This ensures consistent, portable, and reliable file locking mechanisms, avoiding system-specific tools like flock or shlock that may not be universally available.
Key points from CEP-5:
shlock: Use hash -p ./.github/tool_shlock_helper.sh shlock to make the custom shlock available as a shell command.shlock to prevent concurrent executions.trap commands to handle signals and ensure locks are released appropriately./tmp.PR #118 introduced improvements to command existence checks using test -x and discussed the potential for further enhancements by using lists and loops for maintainability. The custom shlock implementation was also highlighted as a reliable solution for file locking in bash scripts.
tests/check_* Scripts:I reviewed the tests/check_* scripts, and here are some opportunities to implement the idea from issue #126:
shlock implementation for file locking. This would ensure consistency and prevent race conditions during concurrent executions.test -x checks can be simplified using lists and loops, as discussed in PR #118. This would improve maintainability and reduce redundancy in the scripts.Would you like me to proceed with specific recommendations or code changes for these scripts? Let me know how you'd like to move forward!
reactive-firewall
commented
1 day ago @coderabbitai, proceed with specific recommendations or code changes for these scripts.
coderabbitai[bot]
commented
1 day ago [!TIP] For best results, initiate chat on the files or code changes.
@reactive-firewall, thank you for confirming.
Based on the discussions in issue #126, CEP-5, and PR #118, along with the project's conventions and previous learnings, here are specific recommendations for the tests/check_* scripts:
The project currently uses individual test -x $(command -v cmd) checks and prefers to retain the use of hash for certain commands. To improve maintainability while respecting these preferences, we can create a helper function to check command existence.
Example:
# Function to check if a command exists
check_command() {
cmd="$1"
if ! test -x "$(command -v "$cmd")"; then
echo "Error: Required command '$cmd' is not installed."
exit 125
fi
}
# Check required commands
check_command command1
check_command command2
# For commands where 'hash' is preferred
if ! hash command3 2>/dev/null; then
echo "Error: Required command 'command3' is not installed."
exit 125
fiThis approach reduces redundancy and makes the scripts easier to maintain, while following the project's convention of using test -x and hash where appropriate.
shlock:Adopting the custom shlock implementation, as described in CEP-5, will ensure consistent and reliable file locking across all scripts.
Implementation steps:
Include the custom shlock script: Ensure that tool_shlock_helper.sh is available in the project's .github/tools/ directory.
Register shlock at the beginning of your scripts:
# Register the custom shlock
hash -p ./.github/tools/tool_shlock_helper.sh shlockAcquire a lock using shlock:
# Acquire a lock
lockfile="/tmp/$(basename "$0").lock"
shlock -p $$ -f "$lockfile" || exit 125
# Set up a trap to release the lock on script exit
trap "rm -f '$lockfile'; exit" INT TERM EXITReplace $(basename "$0") with the script's name if needed to ensure unique lock files.
Ensure all function definitions appear before any calls to those functions in the scripts. This prevents errors due to Bash's sequential execution and aligns with the project's coding standards.
Follow coding styles: Adhere to the code style and conventions used in existing tests/check_* scripts.
Script placement: Ensure that scripts are appropriately placed within the tests/ directory as per project conventions.
Shebang and permissions: Verify that scripts start with the correct shebang (#!/bin/bash) and have executable permissions.
Please let me know if these recommendations meet your requirements or if you'd like further assistance with implementing these changes!
reactive-firewall
commented
1 day ago @coderabbitai, we'll ignore points 2 and 3 as irrelevant (rather, they are out of scope) to this issue ( #126 ) and only discuss point 1 (as it IS on-topic for this issue): Regarding point 1: I agree with the idea of improving scripts with a helper function, however as these existence checks are performed very early they must carefully consider their own dependencies:
function echo() {
local NEWLINE = "\n";
if [[ ( "${1}" == -n* ) ]] ; then shift ; NEWLINE="" ; fi
printf "%s$NEWLINE" "${@}" ;
}
export -f echo126 as it is more appropriateHere is an improved version:
#! /bin/bash
#
# HEADER PLACEHOLDER
#
# Function to check if a command exists.
# USAGE:
# ~$ check_command CMD
# Arguments:
# CMD (Required) -- Name of the command to check
# Results:
# exits 64 -- missing required argument
# exits 126 -- check complete and has failed, can not find given command.
# returns successful -- check complete and command found to be executable.
function check_command() {
test -z "$1" && { printf "%s\n" "Error: command name is required to check for existence." >&2 ; exit 126 ; } ;
local cmd="$1" ;
test -x "$(command -v ${cmd})" || { printf "%s\n" "Error: Required command '$cmd' is not found." >&2 ; exit 126 ; } ;
} # end check_command()
# propagate/export function to sub-shells
export -f check_command
# Check required commands
check_command command1
check_command command2Notes
printfif [[ ... ]] ; then ... ; fi style spacing)|| )A. Please double check my work.
B. Please generate detailed summary and explanatory details for the eventual documentation for this function (this will go in a new markdown document somewhere in the docs/ directory and be picked up by sphinx).
coderabbitai[bot]
commented
1 day ago [!TIP] For best results, initiate chat on the files or code changes.
@reactive-firewall, thank you for your updated code and detailed notes. I've double-checked your work and have some observations:
A. Double-Checking Your Work
Consistency of Exit Codes:
# exits 64 -- missing required argumenttest -z "$1" && { printf "%s\n" "Error: command name is required to check for existence." >&2 ; exit 126 ; } ;64 for command line usage errors), consider changing the exit code to 64 when the argument is missing:
test -z "$1" && { printf "%s\n" "Error: command name is required to check for existence." >&2 ; exit 64 ; } ;Variable Assignment Spacing:
echo function, there is a space around the equal sign in the variable assignment:
local NEWLINE = "\n";local NEWLINE="\n";Quoting Variable Expansions:
test -x "$(command -v ${cmd})" || { printf "%s\n" "Error: Required command '$cmd' is not found." >&2 ; exit 126 ; } ;test -x "$(command -v "${cmd}")" || { printf "%s\n" "Error: Required command '$cmd' is not found." >&2 ; exit 126 ; } ;Shebang Formatting:
#! /bin/bash#!/bin/bashFunction Export Necessity:
check_command function:
export -f check_commandError Message Clarity:
test -x "$(command -v "${cmd}")" || { printf "%s\n" "Error: Required command '$cmd' is not found in PATH or is not executable." >&2 ; exit 126 ; } ;B. Detailed Summary and Explanatory Details for Documentation
check_command Function DocumentationThe check_command function is a utility designed to verify the existence and executability of required commands in shell scripts. By ensuring all necessary commands are available before execution, scripts can fail gracefully with informative error messages, enhancing robustness and user experience.
check_command CMDCMD (Required): The name of the command to check.printf instead of echo, which may not be a built-in in all environments.Missing Argument:
stderr and exits with exit code 64, indicating a command-line usage error.check_command
# Output to stderr: Error: command name is required to check for existence.
# Exits with code 64.Command Not Found or Not Executable:
stderr and exits with exit code 126, indicating a permission problem or command is not executable.check_command nonexistentcommand
# Output to stderr: Error: Required command 'nonexistentcommand' is not found.
# Exits with code 126.Command Exists and Is Executable:
check_command ls
# No output; continues execution.Function Definition:
function check_command() {
test -z "$1" && { printf "%s\n" "Error: command name is required to check for existence." >&2 ; exit 64 ; } ;
local cmd="$1" ;
test -x "$(command -v "${cmd}")" || { printf "%s\n" "Error: Required command '$cmd' is not found." >&2 ; exit 126 ; } ;
}Key Elements:
command -v to locate the command and test -x to ensure it is executable.stderr and exits with standard exit codes.test, printf) for maximum compatibility.Function Exporting:
export -f check_command to make it available in subshells if required.#!/bin/bash
# Define the check_command function here
# Check required commands
check_command git
check_command awk
check_command sed
# Rest of the script...If there are no subshells that require the check_command function, you can omit the export:
# Define the function
function check_command() { ... }
# No need to export if not used in subshells
# export -f check_commandecho: printf is used instead of echo to avoid issues where echo is not a built-in or behaves inconsistently.#!/bin/bash) for proper script execution.check_command function definition before any calls to it to prevent undefined function errors.sysexits.h) for standard codes.Error messages are sent to stderr to differentiate them from standard output and to allow users to redirect them if necessary.
The check_command function is a simple yet powerful utility to enhance script reliability by ensuring required commands are available before proceeding. By conforming to project standards and POSIX conventions, it promotes consistency and portability across different environments.
Let me know if you have any questions or need further assistance!
reactive-firewall
commented
23 hours ago @coderabbitai, Here is an improved version:
#! /bin/bash
#
# HEADER PLACEHOLDER
#
# Function to check if a command exists.
# USAGE:
# ~$ check_command CMD
# Arguments:
# CMD (Required) -- Name of the command to check
# Results:
# exits 64 -- missing required argument
# exits 126 -- check complete and has failed, can not find given command.
# returns successful -- check complete and command found to be executable.
function check_command() {
test -z "$1" && { printf "%s\n" "Error: command name is required to check for existence." >&2 ; exit 64 ; } ;
local cmd="$1" ;
test -x "$(command -v ${cmd})" || { printf "%s\n" "Error: Required command '$cmd' is not found." >&2 ; exit 126 ; } ;
} # end check_command()
# propagate/export function to sub-shells
export -f check_command
# Check required commands
check_command command1
check_command command2Notes
test -x "$(command -v ${cmd})", rather than adding inner double quotes (which I believe will result in just un-quoting the ${cmd} part) to keep debugging simple for now.coderabbitai[bot]
commented
21 hours ago [!TIP] For best results, initiate chat on the files or code changes.
@reactive-firewall,
Thank you for your updated version and detailed explanations.
Review of Your Updates:
Corrected Exit Code for Missing Arguments:
126 to 64, aligning it with your script's documentation.Quoting in Command Existence Check:
test -x "$(command -v ${cmd})" are sufficient and opted not to add inner double quotes around ${cmd}."${cmd}") can protect against potential issues with variable expansion (such as spaces or special characters), if you're confident that the cmd variable won't contain such characters, your approach is acceptable.Shebang Line with Extra Space:
#! /bin/bash) as per your project style.Regarding your question about whether POSIX or the Bash documentation specifies requirements on shebang line spacing:
Shebang Line Specification:
POSIX Standard:
POSIX does not explicitly define the shebang (#!) syntax or specify whether spaces are allowed between #! and the interpreter path.
The shebang mechanism is a Unix kernel feature for script execution and is not formally specified in the POSIX standard.
Bash Documentation:
The Bash manual does not specify any requirements regarding the spacing in the shebang line.
Scripts with or without a space after #! are generally processed the same way by the Bash interpreter.
Practical Considerations:
#! followed by the interpreter path, optionally accepting whitespace.#!/bin/bash and #! /bin/bash are typically recognized and executed correctly.Conclusion:
#! /bin/bash.Your improved version aligns well with your project preferences and addresses the points discussed. Your attention to detail ensures that the script remains consistent with your project's coding standards.
Let me know if there's anything else I can assist you with!
As discussed in PR #118, there is potential to improve the maintainability of command existence checks in scripts by using lists and loops.
This enhancement will be explored in the next release cycle.
Requested by @reactive-firewall.