Improve error message when app needs to be built #884

etehtsea · 2022-11-07T10:21:27Z

Context

Replaces anyhow with thiserror inside spin_publish library. Tried to preserve the original behavior.
Improves UX when app needs to be built. Please see the first example in the comparison list.

Comparison

Project needs to be built

Before:

spin/examples/http-rust $ /tmp/spin bindle prepare --staging-dir /tmp/http-rust
Error: Failed to expand 'spin.toml' to a bindle

Caused by:
    0: Failed to convert components to Bindle format
    1: Failed to get parcel id for '<redacted>spin/examples/http-rust/target/wasm32-wasi/release/http_rust.wasm'
    2: No such file or directory (os error 2)

After (spin bundle prepare):

spin/examples/http-rust 859-publish $ ../../target/release/spin bindle prepare --staging-dir /tmp/http-rust
Error: Missing build artifact: '<redacted>/spin/examples/http-rust/target/wasm32-wasi/release/http_rust.wasm'

Please try to run `spin build` first

After (spin deploy):

 ../../target/release/spin deploy
Error: Missing build artifact: '<redacted>/spin/examples/http-rust/target/wasm32-wasi/release/http_rust.wasm'

Please try to run `spin build` first

Learn more at https://developer.fermyon.com/cloud/faq

After (spin bindle push):

 ../../target/release/spin bindle push --bindle-server http://localhost:8080
Error: Missing build artifact: '<redacted>/spin/examples/http-rust/target/wasm32-wasi/release/http_rust.wasm'

Please try to run `spin build` first

Bindle server is unavailable

Before:

/tmp/spin bindle push --bindle-server http://localhost:8000/v1
Error: Failed to push bindle to server

Caused by:
    0: Failed to push bindle from '/tmp/.tmpaDtdJm' to server at 'http://localhost:8000/v1'
    1: Error creating request
    2: error sending request for url (http://localhost:8000/v1/_i): error trying to connect: tcp connect error: Connection refused (os error 111)
    3: error trying to connect: tcp connect error: Connection refused (os error 111)
    4: tcp connect error: Connection refused (os error 111)
    5: Connection refused (os error 111)

After:

../../target/release/spin bindle push --bindle-server http://localhost:8000/v1 
Error: Failed to push bindle from '/tmp/.tmpxJ6f3N' to the server at 'http://localhost:8000/v1'

Caused by:
    0: Error creating bindle client
    1: Error creating request
    2: error sending request for url (http://localhost:8000/v1/_i): error trying to connect: tcp connect error: Connection refused (os error 111)
    3: error trying to connect: tcp connect error: Connection refused (os error 111)
    4: tcp connect error: Connection refused (os error 111)
    5: Connection refused (os error 111)

Bindle already exists

Before:

$ /tmp/spin bindle push --bindle-server http://localhost:8080/v1
Error: Failed to push bindle to server

Caused by:
    Bindle spin-hello-world/1.0.0 already exists on the server

 ../../target/release/spin bindle push --bindle-server http://localhost:8080/v1
Error: Failed to push bindle from '/tmp/.tmpwe3qBS' to the server at 'http://localhost:8080/v1'

Caused by:
    Bindle spin-hello-world/1.0.0 already exists on the server

Unable to write into the staging dir

Before:

/tmp/spin bindle prepare --staging-dir /tmp/foo
Error: Failed to expand 'spin.toml' to a bindle

Caused by:
    0: Failed to save app manifest to temporary file
    1: Permission denied (os error 13)

After:

../../target/release/spin bindle prepare --staging-dir /tmp/pd 
Error: Failed to create directory: '/tmp/pd/manifests'

Caused by:
    Permission denied (os error 13)

Network error

Before:

/tmp/spin deploy                               
Error: error sending request for url (https://non-existing-domain-asldkhasdfkljhsdflijsdf.com/healthz): error trying to connect: dns error: failed to lookup address information: Name or service not known

Caused by:
    0: error trying to connect: dns error: failed to lookup address information: Name or service not known
    1: dns error: failed to lookup address information: Name or service not known
    2: failed to lookup address information: Name or service not known

After:

 ../../target/release/spin deploy
Error: error sending request for url (https://non-existing-domain-asldkhasdfkljhsdflijsdf.com/healthz): error trying to connect: dns error: failed to lookup address information: Name or service not known

Caused by:
    0: error trying to connect: dns error: failed to lookup address information: Name or service not known
    1: dns error: failed to lookup address information: Name or service not known
    2: failed to lookup address information: Name or service not known

Fixes #859.

crates/publish/src/lib.rs

itowlson

I've left a bunch of nitty comments but I really like the structure of this - we really should be making much more use of this style of error handling. Good stuff despite the niggles - thank you!

itowlson · 2022-11-07T20:12:48Z

src/lib.rs

+    )
+}
+
+pub(crate) async fn prepare_bindle(


Having two prepare_bindle functions is confusing to me - which do I use where? I'd suggest having just one, the one in spin_publish, and here having a to_printable_error function (badly named, sorry). Callers of this would then look like spin_publish::prepare_bindle(...).await.map_err(to_printable_error) which, although more verbose, makes the additional behaviour easier to see at the call site.

itowlson · 2022-11-07T20:16:47Z

src/lib.rs

+        Err(spin_publish::Error::BindleComponentManifest {
+            source: source_err,
+            path,
+        }) if source_err.kind() == std::io::ErrorKind::NotFound => {


Could we detect this in spin_publish::prepare_bindle and make the error more specific? E.g. Err(PublishError::MissingArtifact(path)) => .... That would simplify the pattern match - currently the multiple lines make the flow hard for me to follow (though this is partly down to rustfmt!)

itowlson · 2022-11-07T20:18:34Z

crates/publish/src/error.rs

+
+/// Describes various errors that can be returned during publishing
+#[derive(Debug, thiserror::Error)]
+pub enum Error {


Consider a name like PublishError or BindleError so it doesn't need to be qualified to distinguish it from anyhow::Error (was creating noise in the match expression).

I tried to make it consistent with the rest of the spin's codebase:

spin_config::Error

spin_redis::Error

spin_plugins::error::Error

Also, <crate>::Error looks consistent with the Rust's ecosystem:

std::io::Error

anyhow::Error

std::error::Error

toml::ser::Error

reqwest::Error

On the other hand, I named specialized Result as PublishResult but in this case I relied on https://github.com/fermyon/spin/blob/main/crates/plugins/src/error.rs#L1.

All in all, I'm not against any of the variants and can rename it to whatever is considered "the default" for the project.

itowlson · 2022-11-07T20:21:23Z

crates/publish/src/error.rs

+    BindlePushingNotImplemented,
+    /// IO errors from interacting with the file system
+    #[error("Error while performing IO operation")]
+    Io(#[from] std::io::Error),


If this relates specifically to the file system then please include a path, the raw Rust io::Error is sadly unhelpful about this!

done. description field now includes the path. Not added it as a separate field because otherwise it gets tricky when there are more then one paths (like copy) or none

itowlson · 2022-11-07T20:22:00Z

crates/publish/src/error.rs

+    /// Publishing of components whose sources are already bindles is not supported
+    #[error("This version of Spin can't publish components whose sources are already bindles")]
+    BindlePushingNotImplemented,
+    /// IO errors from interacting with the file system


How is this distinct from BindleComponentManifest which is also an I/O error?

Dropped BindleComponentManifest

itowlson · 2022-11-07T20:24:23Z

crates/publish/src/error.rs

+    /// Malformed bindle id
+    #[error("App name and version '{0}' do not form a bindle ID")]
+    BindleId(String),
+    /// IO error happened during building bindle component manifest


Is the "bindle component manifest" the munged version of the spin.toml file?

itowlson · 2022-11-07T20:26:56Z

crates/publish/src/error.rs

+    #[error("Failed to build bindle component manifest: {path}")]
+    BindleComponentManifest {
+        /// Represents path on a filesystem
+        path: String,


Given your purpose in this PR, I wonder if it's worth distinguishing "I couldn't find the component.source file" (Wasm file not built) from "something went wrong getting asset file details" (possibly a build issue, but possibly more generic I/O problem)?

itowlson · 2022-11-07T20:27:54Z

crates/publish/src/error.rs

+    #[error("Error while performing IO operation")]
+    Io(#[from] std::io::Error),
+    /// Invalid TOML serialization that can occur when serializing an object to a request
+    #[error("Failed to write app manifest to TOML")]


Is the "app manifest" the same as the "bindle component manifest"?

I think this description isn't correct. Will update to something more generic like "Failed to serialize to TOML" as it's also used to write invoice.toml.

Sounds good. If there's any way we can say what we failed to serialise (the invoice, the app manifest, anything else?) that would be super helpful for diagnostics.

itowlson · 2022-11-07T20:30:47Z

crates/publish/src/bindle_writer.rs

-        tokio::fs::copy(&source_file, &dest_file)
-            .await
-            .with_context(|| copy_parcel_failed_msg(&source_file, &dest_file))?;
+        tokio::fs::copy(&source_file, &dest_file).await?;


I worry about the loss of context here. It looks like if a copy fails, we will no longer get a "parcel copy failed" message, but instead an Io(io::Error) which will not tell us what was happening when we failed or what file we failed on. Am I missing something?

Is there a use-case when copying only one of the parcels will fail?

I was trying to find a balance between wrapping on every level and not giving enough info for debug. Because to my taste, wrapping on every level might be a bit excessive, for ex.:

/tmp/spin bindle push --bindle-server http://localhost:8000/v1 Error: Failed to push bindle to server Caused by: 0: Failed to push bindle from '/tmp/.tmpaDtdJm' to server at 'http://localhost:8000/v1' 1: Error creating request 2: error sending request for url (http://localhost:8000/v1/_i): error trying to connect: tcp connect error: Connection refused (os error 111) 3: error trying to connect: tcp connect error: Connection refused (os error 111) 4: tcp connect error: Connection refused (os error 111) 5: Connection refused (os error 111)

Here bindle client wraps the error p. 1-5
Anyway, I'll try to figure it out!

No, but there is value in telling the user which parcel failed. For example, if a file is missing or permission denied or something, I need to know which file so I can go in and fix it; I don't want to scrobble through every file in the project.

I agree we sometimes go overboard on error wrapping, and I'm happy to see less of that in the main output (e.g. we could consign the full context to logs, though we'd want a nicer experience for displaying them in this case, a la Fisher's -vvvvvv). But I don't want to lose information that is critical to diagnosing and remedying a failure. We got burned by "file does not exist (os error 2)" and NOT KNOWING WHICH FILE far too often in the early days of Spin...!

Hope that makes sense!

itowlson · 2022-11-07T20:57:09Z

Also thanks heaps for posting the before and after output - this is super useful.

It all looks good except "Unable to write into the staging dir" which I have mixed feelings about - I like that it removes the focus on the specific file, but the "error performing I/O operation" drew my attention away from the "permission denied" - could we have something like "failed to save/copy bindle data to temporary directory" maybe?

etehtsea · 2022-11-13T07:51:05Z

crates/publish/src/expander.rs

-            file_sha256_string(&full_path)
-                .with_context(|| format!("Failed to get parcel id for '{}'", full_path.display()))?
+
+            if let Ok(false) = Path::try_exists(&full_path) {


Do you have the min Rust version compiler requirement?
try_exists is marked stable from 1.63.0 (https://doc.rust-lang.org/stable/std/path/struct.Path.html#method.try_exists)

1.63 shouldn't be a problem. I think CI tracks latest stable.

etehtsea · 2022-11-13T08:13:04Z

Update

All I/O operations now wrapped into PublishError::Io which is now includes description to ease-up debugging. Please see Unable to write into the staging dir example in PR's description. Special-cased variants like BindleComponentsManifest were dropped.
TomlSerialization variant is now provides description
MissingBuildArtifact variant is added
spin_publish::Error was renamed into spin_publish::PublishError

Examples in the PR descriptions were updated to match new behavior

itowlson

Looks good - thanks!

itowlson · 2022-11-17T01:45:36Z

Oh heck, looks like you need to sign/re-sign some commits. Sorry, not sure why GH can't check that at check time instead of 'ready to merge' time.

Refs: fermyon#859. Signed-off-by: Konstantin Shabanov <mail@etehtsea.me>

Fail deploy with missing build artifact error when app has to be built first. Fixes fermyon#859. Signed-off-by: Konstantin Shabanov <mail@etehtsea.me>

etehtsea · 2022-11-17T04:24:17Z

Oh heck, looks like you need to sign/re-sign some commits. Sorry, not sure why GH can't check that at check time instead of 'ready to merge' time.

I'm forgetting to re-sign commits after rebase. Will check what I can do to avoid this in the future.
Sorry about that!

itowlson · 2022-11-17T06:25:05Z

There is a Git config setting you can turn on to GPG sign things (I think VS Code also has some integration for it). If you don't want to always GPG sign, or don't want to use your main GPG key for Spin, it might be possible to set it at a repo level rather than globally, not sure.

etehtsea · 2022-11-17T15:28:30Z

Yeah, previously I temporarily turned it off because I had some issues with gpg-agent and it wasn't convenient to type the passphrase over and over again. Now it seems to work just fine.

P.S. Also, TIL: https://github.blog/changelog/2022-08-23-ssh-commit-verification-now-supported/

etehtsea force-pushed the 859-publish branch from e2346cc to edf1d7e Compare November 7, 2022 13:09

etehtsea commented Nov 7, 2022

View reviewed changes

crates/publish/src/lib.rs Outdated Show resolved Hide resolved

etehtsea force-pushed the 859-publish branch from edf1d7e to 514d6f0 Compare November 7, 2022 13:15

etehtsea marked this pull request as ready for review November 7, 2022 15:03

etehtsea force-pushed the 859-publish branch from 514d6f0 to 9878661 Compare November 7, 2022 15:10

itowlson reviewed Nov 7, 2022

View reviewed changes

etehtsea mentioned this pull request Nov 8, 2022

Introduce spin_publish::prepare_bindle #887

Merged

etehtsea marked this pull request as draft November 8, 2022 15:09

This was referenced Nov 9, 2022

Get Wasm modules from Web URLs #890

Merged

chore: Reuse sha256 digest from spin_loader #893

Merged

etehtsea force-pushed the 859-publish branch from 9878661 to 5710d0b Compare November 13, 2022 07:15

etehtsea commented Nov 13, 2022

View reviewed changes

etehtsea marked this pull request as ready for review November 13, 2022 08:13

etehtsea force-pushed the 859-publish branch 3 times, most recently from 0d2d6eb to 5c119ec Compare November 15, 2022 07:18

itowlson approved these changes Nov 17, 2022

View reviewed changes

etehtsea added 2 commits November 17, 2022 10:17

Replace anyhow with thiserror in spin_publish

8111301

Refs: fermyon#859. Signed-off-by: Konstantin Shabanov <mail@etehtsea.me>

Improve error message when app needs to be built

81913bc

Fail deploy with missing build artifact error when app has to be built first. Fixes fermyon#859. Signed-off-by: Konstantin Shabanov <mail@etehtsea.me>

etehtsea force-pushed the 859-publish branch from 5c119ec to 81913bc Compare November 17, 2022 04:18

itowlson merged commit a2f4c79 into fermyon:main Nov 17, 2022

etehtsea deleted the 859-publish branch November 17, 2022 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error message when app needs to be built #884

Improve error message when app needs to be built #884

etehtsea commented Nov 7, 2022 •

edited

Loading

itowlson left a comment

itowlson Nov 7, 2022

etehtsea Nov 13, 2022

itowlson Nov 7, 2022

etehtsea Nov 13, 2022

itowlson Nov 7, 2022

etehtsea Nov 8, 2022 •

edited

Loading

etehtsea Nov 13, 2022

itowlson Nov 7, 2022

etehtsea Nov 13, 2022 •

edited

Loading

itowlson Nov 7, 2022

etehtsea Nov 13, 2022 •

edited

Loading

itowlson Nov 7, 2022

itowlson Nov 7, 2022

itowlson Nov 7, 2022

etehtsea Nov 10, 2022

itowlson Nov 10, 2022

etehtsea Nov 13, 2022

itowlson Nov 7, 2022

etehtsea Nov 10, 2022 •

edited

Loading

itowlson Nov 10, 2022

etehtsea Nov 13, 2022 •

edited

Loading

itowlson commented Nov 7, 2022

etehtsea Nov 13, 2022 •

edited

Loading

itowlson Nov 13, 2022

etehtsea commented Nov 13, 2022

itowlson left a comment

itowlson commented Nov 17, 2022

etehtsea commented Nov 17, 2022 •

edited

Loading

itowlson commented Nov 17, 2022

etehtsea commented Nov 17, 2022 •

edited

Loading

Improve error message when app needs to be built #884

Improve error message when app needs to be built #884

Conversation

etehtsea commented Nov 7, 2022 • edited Loading

Context

Comparison

Project needs to be built

Bindle server is unavailable

Bindle already exists

Unable to write into the staging dir

Network error

itowlson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etehtsea Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etehtsea Nov 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etehtsea Nov 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etehtsea Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etehtsea Nov 13, 2022 • edited Loading

Choose a reason for hiding this comment

itowlson commented Nov 7, 2022

etehtsea Nov 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etehtsea commented Nov 13, 2022

Update

itowlson left a comment

Choose a reason for hiding this comment

itowlson commented Nov 17, 2022

etehtsea commented Nov 17, 2022 • edited Loading

itowlson commented Nov 17, 2022

etehtsea commented Nov 17, 2022 • edited Loading

etehtsea commented Nov 7, 2022 •

edited

Loading

etehtsea Nov 8, 2022 •

edited

Loading

etehtsea Nov 13, 2022 •

edited

Loading

etehtsea Nov 13, 2022 •

edited

Loading

etehtsea Nov 10, 2022 •

edited

Loading

etehtsea Nov 13, 2022 •

edited

Loading

etehtsea Nov 13, 2022 •

edited

Loading

etehtsea commented Nov 17, 2022 •

edited

Loading

etehtsea commented Nov 17, 2022 •

edited

Loading