Fix some errors not being reported on console

[Marcono1234]

Fixes #934

It seems there were two issues:

• Conversion from CargoMSRVError to InstanceError
• InstanceError not being shown on console by default, but instead only being logged with tracing::error!
  (and usage of tracing::error! might also have been problematic because the InstanceError might be about setup of tracing having failed)

It looks like this can be solved by removing Context::output_format (since this information is also available from opts), and then performing the Context::try_from call later and handling errors like normal execution errors.

For now I have marked this as draft and added some notes as comments below. Please let me know if I overlooked something, and what you think.

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 75.17%. Comparing base (980e79a) to head (cf8347b).
Report is 77 commits behind head on main.

Files	Patch %	Lines
src/cli/shared_opts.rs	57.14%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #963      +/-   ##
==========================================
+ Coverage   73.46%   75.17%   +1.71%     
==========================================
  Files          80       80              
  Lines        5548     5564      +16     
==========================================
+ Hits         4076     4183     +107     
+ Misses       1472     1381      -91     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[foresterre]

Hey, Thanks a lot for the PR!

I think I gave you the wrong pointer previously in #934, because once the context is built, it is assumed we have a RustVersion:

https://github.com/foresterre/cargo-msrv/blob/main/src/context/verify.rs#L16

Looking in more detail at this PR, I think you're on the right track. An error is produced after all (courtesy of Context::try_from).

The first step which I feel is a good step in the right direction, is to change the tracing::error! to a println, here:

https://github.com/foresterre/cargo-msrv/pull/963/files/2bee052a328af82a191d8712edaa63a0bd5cb06e#diff-72a8660bff7d91b9c1e7985d434a773cbe58c206ff5f337bcef8fb3ee8bace75L24-R28

There is a small problem though 🥲!

This works for human output methods, e.g. running cargo msrv verify on a project with no rust-version gives us:

     Running `cargo-msrv.exe msrv verify --path ..\codecrafters-redis-rust`
Unable to find key 'package.rust-version' (or 'package.metadata.msrv') in '..\codecrafters-redis-rust\Cargo.toml'

However, we don't want this kind of output if the output-format is, for example json (or: not human), while it currently does:

     Running `cargo-msrv.exe msrv verify --path ..\codecrafters-redis-rust\ --output-format json`
Unable to find key 'package.rust-version' (or 'package.metadata.msrv') in '..\codecrafters-redis-rust\Cargo.toml'

The problem is that I've initialized the https://github.com/foresterre/cargo-msrv/blob/main/src/bin/cargo-msrv.rs#L56 before the reporter.

It makes to do so because the context also tells us, which output format ought to be used (i.e. which reporter handler to use) so it's a bit of a chicken and egg problem.

[foresterre]

One way to solve it would be to manually print json in the event that the output format is json. Using opts is perhaps not ideal over context, but we have to be pragmatic here.

The property is opts.shared_opts.user_opts. I believe you already accessed it too 😄.

I made a small proof of concept, similar to yours:

fn main() {
    let opts = parse_opts(std::env::args_os);
    let output_format = opts.shared_opts.user_output_opts.output_format;

    std::process::exit(
        match _main(opts) {
            Ok((_guard, exit_code)) => exit_code,
            Err(err) => {
                // Can't use tracing::error! here, because it may not have been initialized!
                report_init_err(err, output_format);

                ExitCode::Failure
            }
        }
        .into(),
    );
}

fn report_init_err(err: InstanceError, output_format: OutputFormat) {
    match output_format {
        OutputFormat::Human => eprintln!("{}", err),
        OutputFormat::Minimal => eprintln!("error\n{}", err),
        OutputFormat::Json => {
            eprintln!(
                "{}",
                serde_json::json!({
                    "type": "init",
                    "failure": format!("{}", err),
                })
            );
        }
        OutputFormat::None => {}
    }
}

fn parse_opts<I: IntoIterator<Item = OsString>, F: FnOnce() -> I + Clone>(
    args: F,
) -> CargoMsrvOpts {
    let matches = CargoCli::parse_args(args());
    let opts = matches.to_cargo_msrv_cli().to_opts();

    opts
}

And then fn _main can simply be:

fn _main(opts: CargoMsrvOpts) -> Result<(Option<TracingGuard>, ExitCode), InstanceError> {

Compared to yours I tried to keep as much intact (but as you can see it's very similar in concept), and did some manual effort to get the right output format.

With this concept, if I use the json output format:

Running `cargo-msrv.exe msrv verify --path ..\codecrafters-redis-rust\ --output-format json`
{"failure":"Unable to find key 'package.rust-version' (or 'package.metadata.msrv') in '..\\codecrafters-redis-rust\\Cargo.toml'","type":"init"}

and human:

Running `cargo-msrv.exe msrv verify --path ..\codecrafters-redis-rust\ --output-format human`
Unable to find key 'package.rust-version' (or 'package.metadata.msrv') in '..\codecrafters-redis-rust\Cargo.toml'

Which ever formatting we choose for the json, please also add it here: https://github.com/foresterre/cargo-msrv/blob/main/book/src/output-formats/json.md#list-of-events

I put it all on a branch. You may use whatever you want: 8331209 (https://github.com/foresterre/cargo-msrv/blob/concept/src/bin/cargo-msrv.rs#L19-L68)

P.S. If you want to rename the vague InstanceError to SetupError feel free to do so.

[foresterre]

We need to address the different output formats. Please see my other comments for details.

• use eprintln! over println!
• rename _main_1 etc. to more descriptive names

[Marcono1234]

Are you sure that the output-format handling is really a problem with my proposed changes here? Note that the println! I added is only as fallback in case something goes really wrong (I assume), something which should never happen.

Because I am calling Context::try_from a bit later now, and expected program errors such as the "Unable to find key ..." error are properly handled and respect the --output-format; they don't reach the println!.

In my opinion calling Context::try_from later, as done in this pull request, is the most important fix to this, since it makes sure then that all expected program errors are handled at the same place. The switch from tracing::error! to println! is nice to have (also for correctness), but not strictly needed to fix the original issue here.

[foresterre]

Thanks for the explanation! That will do it indeed.

Then I only have the request to rename the different _main_n functions to more descriptive names :)

[Marcono1234]

Then I only have the request to rename the different _main_n functions to more descriptive names :)

Do you have any suggestions? _main_n might not be that descriptive but at least it makes the order clear.
I guess _main_3 can be named run_program or similar. But for _main_1 and _main_2 I am not sure if a more descriptive name is more helpful (also since they are doing multiple things), and just based on the names it won't be obvious in which order they run. I can't think of very good names for them at the moment.

[foresterre]

Then I only have the request to rename the different _main_n functions to more descriptive names :)

Do you have any suggestions? _main_n might not be that descriptive but at least it makes the order clear. I guess _main_3 can be named run_program or similar. But for _main_1 and _main_2 I am not sure if a more descriptive name is more helpful (also since they are doing multiple things), and just based on the names it won't be obvious in which order they run. I can't think of very good names for them at the moment.

Just some ideas:

1. setup_opts_and_tracing
2. setup_reporter
3. setup_context

or

1. run_with_opts_and_tracing
2. run_with_reporter
3. run_with_context

An alternative could be to let setup_context (_main_3) return the context and call run_app from main.

[Marcono1234]

Thanks for the feedback and the suggestions! I have adjusted the code now, and force-pushed the changes. Please let me know if I should change anything else.

[foresterre]

Thank you!