Skip to content

Add kit directory validation during installation #352

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.

Sign up for GitHub

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

Closed
sayalibhavsar wants to merge 1 commit into from
Closed

Add kit directory validation during installation #352

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions bin/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,76 @@ write_installation_record() {
# Write installation record
write_installation_record

# Validate kit directories specified in templates
echo ""
echo "Validating kit directories..."

missing_kits=()
config_dir=$(readlink -f "config")

for template_file in config/*_template.yml; do
# Skip if no files match the pattern
Comment on lines +216 to +220
Copy link

@qodo-code-review qodo-code-review bot Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

4. Config path assumption 🐞 Bug ⛯ Reliability

The validation uses readlink -f "config" and globs config/*_template.yml relative to the current
working directory. Under set -e, running the installer from a different directory can cause
readlink to fail and abort installation after packages were already installed.
Agent Prompt 
### Issue description
The validation assumes it is run from repo root (`config/...`). With `set -e`, `readlink -f config` will abort the install if invoked from another directory, potentially leaving a partial install.

### Issue Context
The installer uses relative paths for config templates but does not `cd` to the repo root.

### Fix Focus Areas
- bin/install.sh[20-21]
- bin/install.sh[216-221]
- bin/install.sh[219-256]

### Suggested approach
- Compute repo root from script location:
  - `REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd -P)"`
  - `CONFIG_DIR="$REPO_ROOT/config"`
- Use `CONFIG_DIR` everywhere:
  - `for template_file in "$CONFIG_DIR"/*_template.yml; do ...`
- Avoid a failing `readlink -f` under `set -e`:
  - Either drop it and use `$CONFIG_DIR` in messaging,
  - or guard: `config_dir=$(readlink -f "$CONFIG_DIR" 2>/dev/null || echo "$CONFIG_DIR")`
- Optionally skip validation with a clear warning if `$CONFIG_DIR` doesn’t exist.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[ -e "$template_file" ] || continue

template_name=$(basename "$template_file")

# Extract upload_extra value using yq
upload_extra=$(yq -r '.upload_extra // "none"' "$template_file" 2>/dev/null || echo "none")
Comment on lines +225 to +226
Copy link

@qodo-code-review qodo-code-review bot Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Yq not on path 🐞 Bug ⛯ Reliability

The validation calls yq immediately after installing it via pip3 --user, but the script never
adds the user bin dir to PATH. If yq is not already on PATH, the validation silently treats
all templates as having no upload_extra, defeating the PR’s goal.
Agent Prompt 
### Issue description
The new validation invokes `yq` but `yq` is installed via `pip3 --user` and may not be on `PATH` during the same script run. Because stderr is suppressed and a fallback value is used, the validation can silently do nothing.

### Issue Context
`pip --user` installs executables under the Python user base (commonly `~/.local/bin`). The script itself later tells the user to ensure `~/.local/bin` is on their PATH, but validation happens before that guidance.

### Fix Focus Areas
- bin/install.sh[127-138]
- bin/install.sh[212-256]

### Suggested approach
- After the pip installs, add something like:
  - `USER_BASE_BIN="$(python3 -m site --user-base)/bin"; export PATH="$USER_BASE_BIN:$PATH"`
- Before running validation, do:
  - `command -v yq >/dev/null || { echo "Skipping kit validation: yq not found on PATH"; skip block; }`
- Avoid silently treating missing `yq` as `upload_extra=none`; emit a clear warning when skipping validation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


# Skip if upload_extra is 'none', empty, or null
if [[ "$upload_extra" == "none" ]] || [[ -z "$upload_extra" ]] || [[ "$upload_extra" == "null" ]]; then
continue
fi

# Check if upload_extra is an array or a string
is_array=$(yq -r '.upload_extra | type' "$template_file" 2>/dev/null || echo "null")

if [[ "$is_array" == "array" ]]; then
# It's an array, process each element
while IFS= read -r path; do
if [[ "$path" != "none" ]] && [[ -n "$path" ]]; then
if [[ ! -e "$path" ]]; then
missing_kits+=("$template_name:$path")
fi
fi
done < <(yq -r '.upload_extra[]' "$template_file" 2>/dev/null)
else
# It's a string, split by spaces
IFS=' ' read -ra paths <<< "$upload_extra"
for path in "${paths[@]}"; do
Comment on lines +246 to +248
Copy link

@qodo-code-review qodo-code-review bot Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

3. Comma parsing mismatch 🐞 Bug ✓ Correctness

For string-valued upload_extra, the validation splits only on spaces. This conflicts with existing
docs and runtime parsing which treat upload_extra as a comma-separated list; comma-separated
strings will be treated as a single path and incorrectly reported missing.
Agent Prompt 
### Issue description
The validation’s string parsing only splits on spaces, but `upload_extra` is documented and used elsewhere as comma-separated. This causes false positives (e.g., `/a.tar,/b.tar` checked as a single non-existent path).

### Issue Context
Existing logic in `bin/burden` replaces commas with spaces before iterating. Docs also specify comma-separated.

### Fix Focus Areas
- bin/install.sh[245-255]
- docs/test_config_files.md[124-127]
- bin/burden[823-829]

### Suggested approach
- In the string branch, normalize commas:
  - `upload_extra_norm="${upload_extra//,/ }"`
  - then split on default IFS whitespace: `read -r -a paths <<< "$upload_extra_norm"`
- Consider applying the same normalization to array elements too (defensive), if an element itself contains commas.
- Add a small comment noting supported delimiters: comma and/or whitespace.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

if [[ "$path" != "none" ]] && [[ -n "$path" ]]; then
if [[ ! -e "$path" ]]; then
missing_kits+=("$template_name:$path")
fi
fi
done
fi
done

# Display warning if there are missing kits
if [ ${#missing_kits[@]} -gt 0 ]; then
echo ""
echo "======================================================================"
echo "WARNING: Missing Kit Directories"
echo "======================================================================"
echo ""
echo "The following kit files/directories specified in templates do not exist:"
echo ""
for entry in "${missing_kits[@]}"; do
template=$(echo "$entry" | cut -d':' -f1)
path=$(echo "$entry" | cut -d':' -f2-)
echo " - $template: $path"
done
echo ""
echo "These kits are required for certain benchmarks to run properly."
echo "Wrappers depending on these kits may fail until they are provided."
echo ""
echo "Please ensure the required kits are placed at the specified locations"
echo "or update the template files in $config_dir to reflect the correct paths."
echo "======================================================================"
echo ""
fi
Comment on lines +212 to +280
Copy link

@qodo-code-review qodo-code-review bot Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. /home/uploads check missing 📎 Requirement gap ⛯ Reliability

The installer changes add kit-path validation but do not check whether /home/uploads exists or
emit the required non-blocking warning message. This fails the required installer visibility
behavior when /home/uploads is missing.
Agent Prompt 
## Issue description
The installer must warn (non-blocking) when `/home/uploads` is missing, but the current changes only validate kit paths from templates and do not implement the required `/home/uploads` warning.

## Issue Context
Compliance requires the installer to check for `/home/uploads` and print the message: `Warning: /home/uploads directory not found. Some wrappers may fail until it is created.` (or functionally equivalent) without aborting installation.

## Fix Focus Areas
- bin/install.sh[209-285]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


echo "Before you can run Zathras:"
echo "****Ensure ~/.local/bin is in your path"
echo "****Set up a scenario file"
Expand Down
Loading