Skip to content

feat(appimage): more useful error message - #358

Merged
FabianLars-crabnebula merged 2 commits into
crabnebula-dev:mainfrom
mrcjkb:mj/push-wmzttsvnkvon
Aug 31, 2026
Merged

feat(appimage): more useful error message#358
FabianLars-crabnebula merged 2 commits into
crabnebula-dev:mainfrom
mrcjkb:mj/push-wmzttsvnkvon

Conversation

@mrcjkb

@mrcjkb mrcjkb commented Jul 22, 2025

Copy link
Copy Markdown
Contributor

I found the error message when executing the AppImage build script a bit confusing (also, the name of the script in the error message is wrong).

This PR adds the full path to the script.

@FabianLars-crabnebula FabianLars-crabnebula left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks for the pr :)

Comment thread crates/packager/src/error.rs Outdated
@mrcjkb
mrcjkb force-pushed the mj/push-wmzttsvnkvon branch 2 times, most recently from 679900f to 9ebd85d Compare July 23, 2025 14:30
@mrcjkb
mrcjkb force-pushed the mj/push-wmzttsvnkvon branch from 9ebd85d to 567f17b Compare September 26, 2025 08:50
26Emily
26Emily previously approved these changes Aug 13, 2026
@FabianLars-crabnebula

Copy link
Copy Markdown
Contributor

I wonder whether we should just keep it with the debug log https://github.com/mrcjkb/cargo-packager/blob/ac7495aab7eaee0fb10514a24cf56721b5a5940f/crates/packager/src/package/appimage/mod.rs#L193
I don't see much value in adding the path to the error directly if we log it anyway tbh.

Still a bit unhappy about a breaking change just for this tbh

@mrcjkb

mrcjkb commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Another alternative without introducing a breaking change could be to use io::Error::other:

.map_err(|err| io::Error::other(format!("Error running '{0}' script: {err}", sh_file.display())))
.map_err(crate::Error::AppImageScriptFailed)?;

@denjell-crabnebula

denjell-crabnebula commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Hey @FabianLars-crabnebula - why did this not get merged?

Ahhh. Because of the breaking change. Yeah, I agree. NO breaking changes for things like this.

@FabianLars-crabnebula

Copy link
Copy Markdown
Contributor

Another alternative without introducing a breaking change could be to use io::Error::other:

What would the error look like then on the dev/user side? At first glance I would have expected the io error to be lost in the conversion, is it not?

@mrcjkb

mrcjkb commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

Another alternative without introducing a breaking change could be to use io::Error::other:

What would the error look like then on the dev/user side? At first glance I would have expected the io error to be lost in the conversion, is it not?

If for some reason one wanted to pattern match on the underlying io::Error, then yes, it would be lost. The error is encoded via its Display instance.

@FabianLars-crabnebula

Copy link
Copy Markdown
Contributor

oops i missed the {err} on my phone. I think we can go with this approach until i migrate the rust rewrite i did in tauri-bundler over here.

@FabianLars-crabnebula

Copy link
Copy Markdown
Contributor

Ah, you'll need to sign your commits for me to merge this, sorry.

https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

@FabianLars-crabnebula

Copy link
Copy Markdown
Contributor

Actually we queued a few breaking dependency updates so we can break the error enum as well but tbh i kinda prefer this new way.

@mrcjkb
mrcjkb force-pushed the mj/push-wmzttsvnkvon branch from eb55ec1 to fd4c9d3 Compare August 31, 2026 16:31
@mrcjkb

mrcjkb commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Ah, you'll need to sign your commits for me to merge this, sorry.

docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

Hmm, it looks like the original commit was with an old e-mail address I no longer use and jj doesn't appear to be updating it.

Edit: Fixed.

@mrcjkb
mrcjkb force-pushed the mj/push-wmzttsvnkvon branch from fd4c9d3 to 5ffd7a1 Compare August 31, 2026 16:33
Signed-off-by: Marc Jakobi <marc@jakobi.dev>
@mrcjkb
mrcjkb force-pushed the mj/push-wmzttsvnkvon branch from 5ffd7a1 to 2f07afb Compare August 31, 2026 16:35
@FabianLars-crabnebula

Copy link
Copy Markdown
Contributor

thanks!

@FabianLars-crabnebula
FabianLars-crabnebula dismissed stale reviews from denjell-crabnebula and themself via 3b29f88 August 31, 2026 17:34
@FabianLars-crabnebula
FabianLars-crabnebula merged commit 06836b2 into crabnebula-dev:main Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants