Show the package name associated with shard errors #674
Conversation
miry
force-pushed
the
error-package-context
branch
3 times, most recently
from
b9107dc to
d72fdc7
Compare
miry
changed the title
miry
force-pushed
the
error-package-context
branch
3 times, most recently
from
5126925 to
43526fa
Compare
miry
force-pushed
the
error-package-context
branch
from
d598fb4 to
eaff116
Compare
miry
changed the title
miry
force-pushed
the
error-package-context
branch
from
6af2b40 to
ba3b6ce
Compare
|
This looks really nice. But I think the independent changes should be split into separate PRs. Adding the package name context to regular log messages is unrelated to showing the backtrace, for example. Could you please extract unrelated changes into separate PRs? |
I will do it. |
miry
force-pushed
the
error-package-context
branch
from
ba3b6ce to
e31bdba
Compare
In certain cases, it's unclear which package causes an error during shard installation. These changes improves error messages by adding explicit package context. When an error occurs, the log will now indicate which package triggered the failure.
miry
force-pushed
the
error-package-context
branch
from
e31bdba to
b26a668
Compare
src/cli.cr
Outdated
| exit 1 | ||
| rescue ex : Shards::ParseError | ||
| ex.to_s(STDERR) | ||
| exit 1 | ||
| rescue ex : Shards::Package::Error | ||
| package = ex.package | ||
| Shards::Log.error(exception: ex) { "Failed to install `#{package.name}`: #{ex.message}" } |
There was a problem hiding this comment.
issue: I don't get the connection between Shards::Package::Error and "failed to install". Is this error type only intended for installation errors? Maybe that should be better reflected in the name then.
And I suppose there should be more instances of errors that can happen while installing a package and those are not covered here.
The package being installed looks like contextual information to me (ref #676). It's not really a specific error type. So we should probably not treat it as that. The options are either adding a contextual package property to Shards::Error, or wrap the original error in a Shards::Package::Error to add context.
The former is probably easier.
With #676 we could pull the contextual information automatically from Log.context.
There was a problem hiding this comment.
It looks like I overcomplicated the solution. After checking similar errors, I noticed that simply including the package name directly in the error message was sufficient.
That said, I can still enrich Shards::Error with an optional package field and propagate it to the log context when handling the error in cli.cr.
miry
force-pushed
the
error-package-context
branch
4 times, most recently
from
3f515c3 to
c228dad
Compare
…, like in other similar logs messages
miry
force-pushed
the
error-package-context
branch
from
c228dad to
010b66b
Compare
| @@ -855,7 +855,7 @@ describe "install" do | |||
| with_shard(metadata) do | |||
| ex = expect_raises(FailedCommand) { run "shards install --no-color" } | |||
| ex.stdout.should contain <<-ERROR | |||
| E: Could not find executable "nonexistent" | |||
| E: Could not find executable "nonexistent" for "executable_missing" | |||
There was a problem hiding this comment.
I've found two ways of referring to a dependency in error messages:
- Just the name: e.g.,
... for "crystal-db" - With the keyword dependency: e.g.,
... for dependency "crystal-db"
To keep the error messages consistent, which format should we use?
There was a problem hiding this comment.
Let's use whatever here and address standardization of message in a follow-up. Possibly with a more structured approach (#674 (comment)).
|
Please avoid force-pushes. Just amend new fixup commits. That keeps the history readable and makes reviewing easier. 🙇 |
In certain cases, it's unclear which package causes an error during shard installation.
For example:
$ shards update marten Resolving dependencies Fetching https://github.com/miry/marten.git ... Fetching https://github.com/jeromegn/protobuf.cr.git ... Installing marten (0.5.6 at 469058f) ... Using protobuf (2.3.1) ... Postinstall of marten: scripts/precompile_marten_cli Could not find executable "protoc-gen-crystal"Although the error message points to a missing
protoc-gen-crystalexecutable (which is related toprotobuf),the failure occurs during a step that appears to be associated with
marten.This can be confusing and makes debugging more difficult.