Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Running biome --version exits with failure #312

Closed
1 task done
nhedger opened this issue Sep 17, 2023 · 11 comments · Fixed by #373
Closed
1 task done

🐛 Running biome --version exits with failure #312

nhedger opened this issue Sep 17, 2023 · 11 comments · Fixed by #373
Assignees
Labels
A-CLI Area: CLI S-Bug-confirmed Status: report has been confirmed as a valid bug

Comments

@nhedger
Copy link
Member

nhedger commented Sep 17, 2023

Environment information

CLI:
  Version:                      1.2.0
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           macos

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           unset
  JS_RUNTIME_NAME:              unset
  NODE_PACKAGE_MANAGER:         unset

Biome Configuration:
  Status:                       unset

Workspace:
  Open Documents:               0

Discovering running Biome servers...

What happened?

  1. Run biome --version
  2. See that the version is in the output
  3. Check the exit code by running echo $?
  4. See that the printed exit code is 1 (failure)

Expected result

Running biome --version should exit with code 0.

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@nhedger
Copy link
Member Author

nhedger commented Sep 17, 2023

To keep in mind:

When this is fixed, we should update the test block of biome's homebrew formula to check for exit code 0 instead of exit code 1.

@b4s36t4
Copy link
Contributor

b4s36t4 commented Sep 17, 2023

I tried to debug it, it only happens with root command when I pass options like (--version, --help). The result of command of type Err which I'm thinking coming from bpaf.

let command = biome_command().run_inner(Args::current_args());

@nhedger
Copy link
Member Author

nhedger commented Sep 17, 2023

@ematipico
Copy link
Member

@nhedger should we file an issue upstream?

@nhedger
Copy link
Member Author

nhedger commented Sep 19, 2023

Turns out we might be doing something wrong. I'll explain in details later today.

@ematipico ematipico added S-Bug-confirmed Status: report has been confirmed as a valid bug A-CLI Area: CLI labels Sep 20, 2023
@nhedger
Copy link
Member Author

nhedger commented Sep 20, 2023

This will feel counter-intuitive, but I think the problem lies in the following code.

match command {
Ok(command) => {
let color_mode = to_color_mode(command.get_color());
console.set_color(color_mode);
let is_verbose = command.is_verbose();
let result = run_workspace(&mut console, command);
match result {
Err(termination) => {
console.error(markup! {
{if is_verbose { PrintDiagnostic::verbose(&termination) } else { PrintDiagnostic::simple(&termination) }}
});
termination.report()
}
Ok(_) => ExitCode::SUCCESS,
}
}
Err(failure) => {
return if let ParseFailure::Stdout(help, _) = &failure {
console.log(markup! {{help.to_string()}});
ExitCode::FAILURE
} else {
let diagnostic = CliDiagnostic::parse_error_bpaf(failure);
console.error(markup! { {PrintDiagnostic::simple(&diagnostic)}});
ExitCode::FAILURE
}
}
}

Specifically, the part where we're handling the failures.

Err(failure) => {
return if let ParseFailure::Stdout(help, _) = &failure {
console.log(markup! {{help.to_string()}});
ExitCode::FAILURE
} else {
let diagnostic = CliDiagnostic::parse_error_bpaf(failure);
console.error(markup! { {PrintDiagnostic::simple(&diagnostic)}});
ExitCode::FAILURE
}
}

At line 52, we return Exit::FAILURE if the failure is of the Stdout variant of the ParseFailure enumeration.

According to the documentation of that enum, the Stdout variant means

Print this to stdout and exit with success code

I believe returning Exit::SUCCESS would be correct and would solve the issue.

@ematipico
Copy link
Member

Good find, that should be easy to fix then!

@nhedger
Copy link
Member Author

nhedger commented Sep 21, 2023

Yep, I'll take care of it later today.

@b4s36t4
Copy link
Contributor

b4s36t4 commented Sep 21, 2023

@nhedger But why does only --version commands do throw Err while other commands are giving Ok types? My point is if the return type is Err that's actually err right? it doesn't matter how are we handling it? Maybe an upstream or version update(if any) could solve this.

@pacak
Copy link

pacak commented Sep 21, 2023

But why does only --version commands do throw Err while other commands are giving Ok types?

bpaf uses Ok when it produces a value parser is designed to produce - Options, otherwise it produces Err with one of 3 variants - "print this to stderr and exit with failure" and two different "print this to stdout and exit with success", one supports formatting and one doesn't. I'll see if I can improve the docs to make it clearer...

@nhedger nhedger self-assigned this Sep 21, 2023
@nhedger
Copy link
Member Author

nhedger commented Sep 21, 2023

Thanks for clarifying @pacak, much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI S-Bug-confirmed Status: report has been confirmed as a valid bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants