Return error code when process exits with error

[marcospb19]

Current behavior

With the exception of this metadata error that usually occurs when you're not running inside a project:

cargo-sweep/src/main.rs

Lines 154 to 157 in 475ac9e

	let metadata = metadata(&path).context(format!(
	"Failed to gather metadata for {:?}",
	path.display()
	))?;

For every possible error in cargo-sweep, it reports the error and exits with a success exit code.

Expected behavior

The cargo-sweep process should exit with an error exit code.

[marcospb19]

Some technical details for someone who wants to pick this:

I suggest you move contents from main to a new run function, and execute it from main, then use the error! macro in case Result::Err is returned from run.

Fixing on main will break some integration tests that panic when they see an error exit code.

[jyn514]

I suggest you move contents from main to a new run function, and execute it from main, then use the error! macro in case Result::Err is returned from run.

Fixing on main will break some integration tests that panic when they see an error exit code.

I don't agree with this, I think we should continue to use anyhow::Error in main to report errors. To fix the tests we can just update them to look at stderr instead of stdout.

See also #88 (comment).

[marcospb19]

@jyn514 but we can't return anyhow::Error in main, because we are using error! to report our errors and not Debug.

Why call error! for each error instead of having the caller receive it with .context() and have just one place for reporting? It's pretty common to have functions return an error with all the details to be reported.

[jyn514]

@jyn514 but we can't return anyhow::Error in main, because we are using error! to report our errors and not Debug.

Why call error! for each error instead of having the caller receive it with .context() and have just one place for reporting? It's pretty common to have functions return an error with all the details to be reported.

I don't think we should be using error! though - if you look at the current code, we're inconsistent about what uses error!, e.g.

cargo-sweep/src/main.rs

Lines 184 to 186 in c4c5e19

	),
	Err(e) => error!(
	"{:?}",

and what returns an error from main, e.g.

cargo-sweep/src/main.rs

Lines 154 to 162 in c4c5e19

	} else {
	let metadata = metadata(&path).context(format!(
	"Failed to gather metadata for {:?}",
	path.display()
	))?;
	let out = Path::new(&metadata.target_directory).to_path_buf();
	if out.exists() {
	vec![out]
	} else {

I guess standardizing on error! instead of main is fine, but it does seem like a bit of extra work for little benefit.

[marcospb19]

I guess standardizing on error! instead of main is fine

By main you mean Err returning on main, right?

My suggestion of wrapping stuff around a run function is just if you choose to be consistent using error!(), but of course, it doesn't make sense for what you suggested (returning Result on main, but that changes how most errors are currently reported).

[Expurple]

Shouldn't this issue be closed by now? I can't reproduce exit code 0 on errors. Some of the erroring commands I've tried:

• cargo sweep -r -t 30 -i from Feature request: allow ORing between conditions #111
• cargo sweep --file from Panic when the --file file is not found #117
• cargo sweep --stamp name_here from Fails to write a --stamp file to the provided path #118

[marcospb19]

I found some

cargo-sweep/src/main.rs

Lines 187 to 190 in 5075756

	Err(err) => {
	error!("{:?}", err.context("Failed to load toolchains."));
	return Ok(());
	}