-
Notifications
You must be signed in to change notification settings - Fork 61
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
base: testing
Are you sure you want to change the base?
Conversation
Provide output right at the end if a removal fails due to a preremove script. That way, the user can double-check the output to see if there is something bad. For example, kernelctl failed to revert to the stock kernel.
I created a test package that will fail during preremove. |
Uninstall removes /opt, which is actually /home/root/.entware. you will have to install again with bootstrap. If you want to avoid doing so, you can copy the .entware folder before doing the uninstall, rename the copy to .entware after uninstalling and then re-enable. |
package/toltec-bootstrap/toltecctl
Outdated
@@ -806,6 +808,14 @@ uninstall() { | |||
} | |||
set -o functrace | |||
trap 'handle-error ${LINENO}' ERR | |||
handle-exit() { | |||
if ! $completed && $success && $clean; then |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
/opt
, not that entware supports that yet.