Package transactions: Always set error #2363
Open
+62
−125
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
leolost2605
force-pushed
the
leolost/always-set-error
branch
from
4ff3d62 to
abebc6f
Compare
tintou
reviewed
Oct 13, 2025
false is never returned because instead an error is thrown
leolost2605
force-pushed
the
leolost/always-set-error
branch
from
abebc6f to
d442f41
Compare
lenemter
reviewed
Oct 19, 2025
Comment on lines
-1628
to
1629
| job.result = Value (typeof (bool)); | ||
| job.result.set_boolean (false); | ||
| job.error = new IOError.FAILED (_("Component has no flatpak bundle")); | ||
| job.results_ready (); |
Member
There was a problem hiding this comment.
Shouldn't this produce a critical/warning in the console as well?
| if (!split_success) { | ||
| job.result = Value (typeof (bool)); | ||
| job.result.set_boolean (false); | ||
| job.error = new IOError.FAILED (_("Failed to split package key")); |
Comment on lines
498
to
502
| try { | ||
| return yield perform_operation (State.REMOVING, State.NOT_INSTALLED, state); | ||
| yield perform_operation (State.REMOVING); | ||
| } catch (Error e) { | ||
| throw e; | ||
| } |
Member
There was a problem hiding this comment.
Since this method throws an error anyway, you can remove try-catch block here
lenemter
reviewed
Oct 19, 2025
| if (bundle == null) { | ||
| job.result = Value (typeof (bool)); | ||
| job.result.set_boolean (false); | ||
| job.error = new IOError.FAILED (_("Component has no flatpak bundle")); |
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.
The first three commits make sure we always set an error instead of returning false for the three package transactions. This way we make sure we never fail silently for the user. Also it simplifies not having to deal with a success variable as well.
There was also bug where != was used instead of ==. The comment said we don't want to overwrite existing errors but the code checked if there was an existing error and if there WAS it overwrote it instead of the other way round.
The fourth commit ignores the return value from the package ops since it will now always return true or set an error so it assumes success if no error is set.
And the last commit is a little cleanup that changes the backend methods to
void.This also serves as preparation for fixing cancelling in a way that we get immediate feedback when cancelling instead of just a flicker that looks like it doesn't cancel.
Can use rebase to merge.