Display system info before the stack trace #2347
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideAdds a reusable system information collector and indentation helper to improve fatal error stack traces, updates default DockSTARTer installation paths, and refreshes GitHub Actions workflows and docs accordingly. Sequence diagram for fatal error handling with system infosequenceDiagram
participant Script as main_sh
participant Fatal as fatal
participant SysInfo as get_system_info
participant Indent as indent_text
participant FatalNoTrace as fatal_notrace
participant Logger as log
Script->>Fatal: fatal(message...)
activate Fatal
Fatal->>SysInfo: get_system_info()
SysInfo-->>Fatal: system_info_lines[]
Fatal->>Indent: indent_text(2, system_info_and_stack[])
Indent-->>Fatal: indented_text
Fatal->>FatalNoTrace: fatal_notrace(header, indented_text, footer, message...)
deactivate Fatal
activate FatalNoTrace
FatalNoTrace->>Logger: log(true, timestamped_log("[FATAL]", ...))
Logger-->>FatalNoTrace: write_to_log_and_stderr
FatalNoTrace-->>Script: exit 1
deactivate FatalNoTrace
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
bot
added
ci
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- The new
indent_texthelper is now defined in bothmain.shand.includes/misc_functions.sh; consider centralizing it in one place and reusing it to avoid duplication and divergence. - The hard-coded
fatal "Super Fatal Error"call at the start ofmenu_mainlooks like a leftover debug statement and will prevent the menu from ever displaying; it should probably be removed or guarded. - After adding
arm64as a supported architecture incheck_arch, the error message still only listsaarch64andx86_64; update the message so it matches the actual set of supported architectures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `indent_text` helper is now defined in both `main.sh` and `.includes/misc_functions.sh`; consider centralizing it in one place and reusing it to avoid duplication and divergence.
- The hard-coded `fatal "Super Fatal Error"` call at the start of `menu_main` looks like a leftover debug statement and will prevent the menu from ever displaying; it should probably be removed or guarded.
- After adding `arm64` as a supported architecture in `check_arch`, the error message still only lists `aarch64` and `x86_64`; update the message so it matches the actual set of supported architectures.
## Individual Comments
### Comment 1
<location> `main.sh:306` </location>
<code_context>
+
local -a Stack=()
+
+ get_system_info | readarray -t Stack
+ Stack+=("")
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `readarray` in a pipeline will populate `Stack` in a subshell, leaving `Stack` empty in the current shell.
In bash, unless `lastpipe` is enabled, this means the `Stack` array in the current shell never gets updated, so the system info you want to prepend is lost. Prefer process substitution to keep `readarray` in the current shell:
```bash
readarray -t Stack < <(get_system_info)
Stack+=("")
```
This ensures `Stack` is correctly populated with the system info.
</issue_to_address>
### Comment 2
<location> `.scripts/menu_main.sh:10` </location>
<code_context>
return
fi
+ fatal "Super Fatal Error"
local Title="Main Menu"
local Option_Configure="Configuration"
</code_context>
<issue_to_address>
**issue (bug_risk):** Calling `fatal` at the top of `menu_main` will abort every invocation of the main menu.
This unconditional `fatal` causes every `menu_main` call to exit immediately before running any menu logic. If it’s only for debugging, remove it or guard it behind a condition so normal menu execution can proceed.
</issue_to_address>
### Comment 3
<location> `main.sh:397-399` </location>
<code_context>
# Check for supported CPU architecture
check_arch() {
- if [[ ${ARCH} != "aarch64" ]] && [[ ${ARCH} != "x86_64" ]]; then
+ if [[ ${ARCH} != "arm64" ]] && [[ ${ARCH} != "aarch64" ]] && [[ ${ARCH} != "x86_64" ]]; then
fatal_notrace \
"Unsupported architeture." \
</code_context>
<issue_to_address>
**suggestion:** The error message still lists only `aarch64` and `x86_64` as supported even though `arm64` is now accepted.
Consider updating the fatal message to mention `arm64` alongside `aarch64` and `x86_64` so it matches the actual supported architectures, and optionally fix the `architeture` typo.
```suggestion
fatal_notrace \
"Unsupported architecture." \
"Supported architectures are 'arm64', 'aarch64' or 'x86_64', running architecture is '${ARCH}'."
```
</issue_to_address>
### Comment 4
<location> `.includes/misc_functions.sh:78-83` </location>
<code_context>
printf '%s\n' "${Result}"
}
+indent_text() {
+ local -i IndentSize=${1}
+ shift
+ pr -e -t -o ${IndentSize} <<< "$(
+ printf "%s\n" "$@"
+ )"
+}
+
</code_context>
<issue_to_address>
**suggestion:** `indent_text` is now defined in both `main.sh` and `.includes/misc_functions.sh`, which introduces duplication and possible confusion about which implementation is used.
There are now two identical `indent_text` definitions (here and in `main.sh`). If both files are sourced, this can cause confusion about which one is actually used and risk divergence if only one is updated. Please keep a single shared definition (preferably in a common helper file) and have other scripts call that, or remove this copy if `main.sh` is the intended source of truth.
Suggested implementation:
```
}
is_true() {
```
1. Ensure that `main.sh` (or whichever file now owns `indent_text`) is sourced before any script that relies on `indent_text`.
2. If other scripts were previously sourcing `.includes/misc_functions.sh` specifically to get `indent_text`, update them to source the new canonical location instead.
</issue_to_address>
### Comment 5
<location> `docs/basics/command-line-usage.md:79` </location>
<code_context>
This is the best place to change the app's external default ports.
-There will also be an application specific variable file created at `~/.docker/compose/.env.app.<appname>`. This may or not be populated with variables. The variables included, if any, will entierly depend on the application installed. If you need to edit the values of the variables, or include any new variables for the app, it is entirely safe to do so.
+There will also be an application specific variable file created at `~/.dockstarter/compose/.env.app.<appname>`. This may or not be populated with variables. The variables included, if any, will entierly depend on the application installed. If you need to edit the values of the variables, or include any new variables for the app, it is entirely safe to do so.
#### Removing Apps
</code_context>
<issue_to_address>
**issue (typo):** Fix grammar and spelling in the description of the application-specific variable file.
Consider rephrasing to: "This may or may not be populated with variables. The variables included, if any, will entirely depend on the application installed."
```suggestion
There will also be an application specific variable file created at `~/.dockstarter/compose/.env.app.<appname>`. This may or may not be populated with variables. The variables included, if any, will entirely depend on the application installed. If you need to edit the values of the variables, or include any new variables for the app, it is entirely safe to do so.
```
</issue_to_address>
### Comment 6
<location> `docs/basics/faq.md:12` </location>
<code_context>
+For new installs the default `DOCKER_VOLUME_CONFIG` is `~/.config/appdata`. Users who ran DockSTARTer before this location became the default may have `~/.dockstarter/config`, and we advise relocating.
If you'd like to move your existing config to a new location you can do the following:
Edit `~/. docker/compose/.env` (in any text editor) and set
</code_context>
<issue_to_address>
**suggestion (typo):** Remove the stray space in the path and align it with the updated `~/.dockstarter` directory.
The path is currently written as `~/. docker/compose/.env`, which has an extra space after `~/.`. To match the rest of the docs and the new default, this should be `~/.dockstarter/compose/.env`.
```suggestion
Edit `~/.dockstarter/compose/.env` (in any text editor) and set
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
fatal(), the[[ Frame -ge StartFrame ]]checks never expandFrameand will be treated as a literal string, which can cause runtime errors underset -euo pipefail; use arithmetic context (e.g.(( Frame >= StartFrame ))) or[[ $Frame -ge $StartFrame ]]instead. - The
check_arch()error message still lists onlyaarch64andx86_64as supported, but the function now acceptsarm64as well; consider updating the message to includearm64to avoid confusing users. - In
get_system_info(), calls likereadlink /proc/$$/exeassume/procexists and can emit noisy errors on platforms without it; you may want to guard them (e.g. with2>/dev/nulland a fallback) to keep fatal output clean and robust across environments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `fatal()`, the `[[ Frame -ge StartFrame ]]` checks never expand `Frame` and will be treated as a literal string, which can cause runtime errors under `set -euo pipefail`; use arithmetic context (e.g. `(( Frame >= StartFrame ))`) or `[[ $Frame -ge $StartFrame ]]` instead.
- The `check_arch()` error message still lists only `aarch64` and `x86_64` as supported, but the function now accepts `arm64` as well; consider updating the message to include `arm64` to avoid confusing users.
- In `get_system_info()`, calls like `readlink /proc/$$/exe` assume `/proc` exists and can emit noisy errors on platforms without it; you may want to guard them (e.g. with `2>/dev/null` and a fallback) to keep fatal output clean and robust across environments.
## Individual Comments
### Comment 1
<location> `main.sh:326` </location>
<code_context>
- local StackLineFormat=" ${C["TraceFrameNumber"]}%${FrameNumberLength}d${NC}:${indent}${FramePrefix}${C["TraceSourceFile"]}%s${NC}:${C["TraceLineNumber"]}%d${NC} (${C["TraceFunction"]}%s${NC})"
+ local StackLineFormat="${C["TraceFrameNumber"]}%${FrameNumberLength}d${NC}:${indent}${FramePrefix}${C["TraceSourceFile"]}%s${NC}:${C["TraceLineNumber"]}%d${NC} (${C["TraceFunction"]}%s${NC})"
+
+ if [[ Frame -ge StartFrame ]]; then
+ # shellcheck disable=SC2059 # Don't use variables in the printf format string.
+ Stack+=(
</code_context>
<issue_to_address>
**issue (bug_risk):** Frame/StartFrame comparison is missing `$` expansions and will fail as an integer comparison at runtime.
In the `fatal` loop, `[[ Frame -ge StartFrame ]]` (and the later `if [[ Frame -ge StartFrame ]]; then`) compare the literal strings `Frame`/`StartFrame`, so `[[` treats them as arithmetic and fails with `integer expression expected`. This can cause unexpected recursion into `fatal` or terminate the script. Use variable expansion and a numeric comparison, e.g. `if (( Frame >= StartFrame )); then` or `if [[ $Frame -ge $StartFrame ]]; then`, and update the other occurrence accordingly.
</issue_to_address>
### Comment 2
<location> `main.sh:396-399` </location>
<code_context>
# Check for supported CPU architecture
check_arch() {
- if [[ ${ARCH} != "aarch64" ]] && [[ ${ARCH} != "x86_64" ]]; then
+ if [[ ${ARCH} != "arm64" ]] && [[ ${ARCH} != "aarch64" ]] && [[ ${ARCH} != "x86_64" ]]; then
fatal_notrace \
- "Unsupported architeture." \
- "Supported architetures are 'aarch64' or 'x86_64', running architeture is '${ARCH}'."
+ "Unsupported architecture." \
+ "Supported architectures are 'aarch64' or 'x86_64', running architecture is '${ARCH}'."
fi
}
</code_context>
<issue_to_address>
**issue:** The supported architectures message is out of sync with the newly-allowed `arm64` architecture.
The condition now allows `arm64`, but the error message still lists only `'aarch64'` and `'x86_64'`. This could confuse `arm64` users who hit this path. Please update the message to include `arm64` (or derive the supported list from a single source to avoid future drift).
</issue_to_address>
### Comment 3
<location> `docs/basics/command-line-usage.md:50` </location>
<code_context>
## Setup Your Environment
-If you do not yet have a `~/.docker/compose/.env` and/or `~/.docker/compose.env.app.<appname>`files:
+If you do not yet have a `~/.dockstarter/compose/.env` and/or `~/.dockstarter/compose.env.app.<appname>`files:
```bash
</code_context>
<issue_to_address>
**suggestion (typo):** Fix minor spacing and path issues in the application env file sentence.
Please add a space before "files" and update the second path to include the missing `/` after `compose`: `~/.dockstarter/compose/.env.app.<appname>` for consistency with the rest of the docs.
```suggestion
If you do not yet have a `~/.dockstarter/compose/.env` and/or `~/.dockstarter/compose/.env.app.<appname>` files:
```
</issue_to_address>
### Comment 4
<location> `docs/basics/command-line-usage.md:79` </location>
<code_context>
This is the best place to change the app's external default ports.
-There will also be an application specific variable file created at `~/.docker/compose/.env.app.<appname>`. This may or not be populated with variables. The variables included, if any, will entierly depend on the application installed. If you need to edit the values of the variables, or include any new variables for the app, it is entirely safe to do so.
+There will also be an application specific variable file created at `~/.dockstarter/compose/.env.app.<appname>`. This may or not be populated with variables. The variables included, if any, will entierly depend on the application installed. If you need to edit the values of the variables, or include any new variables for the app, it is entirely safe to do so.
#### Removing Apps
</code_context>
<issue_to_address>
**suggestion (typo):** Correct minor grammar and spelling in the description of the application-specific variable file.
Please update the sentence to "This may or may not be populated with variables." and correct "entierly" to "entirely" for clarity.
```suggestion
There will also be an application specific variable file created at `~/.dockstarter/compose/.env.app.<appname>`. This may or may not be populated with variables. The variables included, if any, will entirely depend on the application installed. If you need to edit the values of the variables, or include any new variables for the app, it is entirely safe to do so.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add detailed system information collection and formatting to fatal error handling, and align default installation paths and documentation with the .dockstarter directory while updating CI dependencies.
Enhancements:
Build:
Documentation: