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

Provide better output on toltecctl uninstall #692

Open
wants to merge 37 commits into
base: testing
Choose a base branch
from
Open
Changes from 1 commit
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
f0c8013
Provide better output on toltecctl uninstall
Eeems May 10, 2023
71c5462
Update version
Eeems May 10, 2023
46ab86b
Remove extra whitespace left by the github editor
Eeems May 10, 2023
99fb2f6
Make variable local
Eeems May 10, 2023
c3dba52
No longer use a subshell
Eeems May 10, 2023
1649a09
Merge branch 'testing' into Eeems-patch-10
Eeems May 10, 2023
f10380a
Merge branch 'testing' into Eeems-patch-10
Eeems Aug 2, 2023
25372bd
Merge branch 'testing' into Eeems-patch-10
Eeems Aug 2, 2023
f45e08b
Merge branch 'testing' into Eeems-patch-10
Eeems Aug 8, 2023
03d090e
Merge branch 'testing' into Eeems-patch-10
Eeems Aug 15, 2023
90d6117
Merge branch 'testing' into Eeems-patch-10
Eeems Oct 3, 2023
ecee049
Merge branch 'testing' into Eeems-patch-10
Eeems Nov 27, 2023
325cd4d
Fix toltec-bootstrap version
Eeems Nov 27, 2023
8937f96
Merge branch 'testing' into Eeems-patch-10
Eeems Dec 6, 2023
038d359
Merge branch 'testing' into Eeems-patch-10
Eeems May 8, 2024
b7e1753
Merge branch 'testing' into Eeems-patch-10
Eeems May 31, 2024
44228c5
Merge branch 'testing' into Eeems-patch-10
Eeems May 31, 2024
0e024aa
Merge branch 'testing' into Eeems-patch-10
Eeems Jun 2, 2024
94af3c3
Add missing dependency to launcherctl
Eeems Jun 2, 2024
08c582d
Merge branch 'testing' into Eeems-patch-10
Eeems Jun 2, 2024
ea59852
Bump toltec-bootstrap version
Eeems Jun 2, 2024
fcedcd4
Force launcherctl to uninstall before xochitl
Eeems Jun 2, 2024
2645de9
Make uninstall way more fault tolerant, and report better errors
Eeems Jun 2, 2024
a11076d
Fix error line output
Eeems Jun 2, 2024
9c24c05
Better handle failed uninstalls
Eeems Jun 2, 2024
c96df00
Add extra log line
Eeems Jun 2, 2024
30663b7
Fix entware-rc depends
Eeems Jun 2, 2024
dcf6d73
fix final exit
Eeems Jun 2, 2024
52b100e
Merge branch 'testing' into Eeems-patch-10
Eeems Jun 3, 2024
96720a3
Merge branch 'testing' into Eeems-patch-10
Eeems Jun 6, 2024
8deff1c
Update toltecctl
Eeems Sep 7, 2024
7604217
Merge branch 'testing' into Eeems-patch-10
Eeems Sep 7, 2024
54d5dcb
Update toltecctl
Eeems Sep 7, 2024
3cd792e
Merge branch 'testing' into Eeems-patch-10
Eeems Sep 12, 2024
3e5c0a1
Merge branch 'testing' into Eeems-patch-10
Eeems Sep 18, 2024
118c9a1
Merge branch 'testing' into Eeems-patch-10
Eeems Oct 12, 2024
bf0683f
Merge branch 'testing' into Eeems-patch-10
Eeems Dec 17, 2024
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
18 changes: 16 additions & 2 deletions package/toltec-bootstrap/toltecctl
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,8 @@ remove-bind-mount() {

log INFO "Removing mount over '$1'"
systemctl disable "$unit_name"
systemctl stop "$unit_name"
# Don't error, as umount -l will handle the failure
systemctl stop "$unit_name" || true
if mountpoint -q "$1"; then
umount -l "$1"
fi
Expand Down Expand Up @@ -790,6 +791,7 @@ uninstall() {
fi
local clean=true
local success=true
local complete=false

broken-state() {
echo " Your system may be in a broken state. Please review the logs"
Expand All @@ -806,6 +808,14 @@ uninstall() {
}
set -o functrace
trap 'handle-error ${LINENO}' ERR
handle-exit() {
if ! $completed && $success && $clean; then
Copy link

Choose a reason for hiding this comment

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

completed vs complete, wrong variable names

Copy link
Member Author

Choose a reason for hiding this comment

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

You seem to be reviewing an outdated version, this is $complete for me when I look

Copy link

Choose a reason for hiding this comment

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

Ok, I'm reviewing patches in order, not a full diff. I'm more used to a workflow where a PR is force updated instead of incremental patches and a squash afterwards.

Copy link

@Havner Havner Sep 7, 2024

Choose a reason for hiding this comment

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

Also I think there is operator order issue here:

$ export VAR1=true
$ export VAR2=true
$ export VAR3=true
$ if ! $VAR1 && $VAR2 && $VAR3; then echo failed; fi
$ export VAR2=false
$ if ! $VAR1 && $VAR2 && $VAR3; then echo failed; fi
$ if ! ($VAR1 && $VAR2 && $VAR3); then echo failed; fi
failed

Copy link
Member Author

Choose a reason for hiding this comment

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

Since they are declared as local it could be that they aren't working as expected. Although when I tested this originally it was working as I expected.

Copy link

Choose a reason for hiding this comment

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

I don't think local makes any difference here, ! has higher priority than && hence it applies only to VAR1 and from the look of it you wanted to apply it to the whole expression (V1 && V2 && V3)

Copy link
Member Author

Choose a reason for hiding this comment

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

I would have wrapped it in ( and ) if I was trying to apply it to all of them, I'm checking if not complete, and success and clean. success/clean are true by default and get changed to false when something else handles reporting an error.

Copy link

Choose a reason for hiding this comment

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

Ah, ok then. From reading the code it looked to me like you are looking for a failure scenario (e.g. at least one of those vars is false).

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, I'm only wanting to handle the error if the error handler is triggered and we did not complete the uninstall, and nothing set success or clean to false due to their own error handling.

It is less than ideal, thus the extra comments on the variable declarations. I couldn't really come up with a cleaner way to handle this with the limitations bash has on error handling.

Copy link

Choose a reason for hiding this comment

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

I would have to analyze this fully to check the logic completely. I'm too tired now.

What I did find though is that you make a dep between clean and success so checking both in this line might be at least redundant.

https://github.com/toltec-dev/toltec/blob/Eeems-patch-10/package/toltec-bootstrap/toltecctl#L885

But I'm not able to check the logic fully now.

log ERROR "Uninstall did not complete properly"
broken-state
exit 1
fi
}
trap 'handle-exit' EXIT

if [[ "$toltecctl_path" != *_backup ]]; then
log INFO "Creating backup of toltecctl at ${toltecctl_path}_backup"
Expand All @@ -820,6 +830,7 @@ uninstall() {
fi
log ERROR "Current install seems to be partially removed, unable to automatically removed."
broken-state
complete=true
exit 1
fi

Expand All @@ -839,8 +850,9 @@ EOF
fi
rm -f "$opkg_path"

# Remove mount point
set +e # Don't exit on error, next check will validate if it worked
remove-bind-mount "$toltec_dest"
set -e

case "$(install-state)" in
no) ;;
Expand Down Expand Up @@ -876,12 +888,14 @@ EOF
if ! $success; then
log ERROR "Uninstall failed"
broken-state
complete=true
exit 1
fi
if [[ "$toltecctl_path" != *_backup ]]; then
rm -f "$toltecctl_path"
fi
rm -f "${toltecctl_path}_backup"
complete=true
}

# The current toltec install state
Expand Down
Loading