fix: --print-all-dependencies should handle unknown-deriver
#70
Conversation
src/nix/nix_store.rs
Outdated
| @@ -128,9 +128,11 @@ impl NixStoreCmd { | |||
| Ok(out) | |||
| } else { | |||
| let exit_code = out.status.code().unwrap_or(1); | |||
| let stderr_output = String::from_utf8(out.stderr)?; | |||
There was a problem hiding this comment.
Should this not be from_utf8_lossy?
There was a problem hiding this comment.
?;
Pretty much this causes the function to (unnecessarily) fail on decoding errors with stderr, which is something we don't care about. Lossy conversation is fine.
There was a problem hiding this comment.
As an aside: must we capture stderr? Why not let the parent process inherit and stream it directly to the user (still in stderr)?
srid
changed the title
--print-all-dependencies
rsrohitsingh682
force-pushed
the
add-stderr
branch
from
40f1299 to
3f0737b
Compare
|
src/nix/nix_store.rs
Outdated
| let stderr = String::from_utf8(out.stderr).ok(); | ||
| let exit_code = out.status.code(); | ||
| Err(CommandError::ProcessFailed { stderr, exit_code }.into()) |
There was a problem hiding this comment.
There's a bug there. If UTF8 decoding fails, this sets stderr to None thus creating a false impression that no stderr is missing. Just use lossy decoding.
src/nix/nix_store.rs
Outdated
| let stderr = String::from_utf8(out.stderr).ok(); | ||
| let exit_code = out.status.code(); | ||
| Err(CommandError::ProcessFailed { stderr, exit_code }.into()) |
src/nix/nix_store.rs
Outdated
|
|
||
| /// Errors when running and interpreting the output of a nix command | ||
| #[derive(Error, Debug)] | ||
| pub enum NixCmdError { |
There was a problem hiding this comment.
You shouldn't use duplicate names for types in order to avoid confusion. NixCmdError already exists: https://github.com/juspay/nix-rs/blob/2dbba678a9e67925d964dbde24fbd8951ecbd2de/src/command.rs#L187
| pub enum NixCmdError { | |
| pub enum NixStoreCmdError { |
src/nix/nix_store.rs
Outdated
| #[error("Command error: {0}")] | ||
| CmdError(#[from] CommandError), | ||
|
|
||
| #[error("Failed to decode command stdout (utf8 error): {0}")] | ||
| DecodeErrorUtf8(#[from] std::string::FromUtf8Error), | ||
|
|
||
| #[error("Failed to decode command stdout (from_str error): {0}")] | ||
| DecodeErrorFromStr(#[from] FromStrError), | ||
|
|
||
| #[error("Failed to decode command stdout (json error): {0}")] | ||
| DecodeErrorJson(#[from] serde_json::Error), |
There was a problem hiding this comment.
Why copy the fields rather than re-use NixCmdError directly here?
There was a problem hiding this comment.
src/nix/nix_store.rs
Outdated
| #[error("Failed to decode command stdout (json error): {0}")] | ||
| DecodeErrorJson(#[from] serde_json::Error), | ||
|
|
||
| #[error("Unknown deriver in drv_path")] |
There was a problem hiding this comment.
| #[error("Unknown deriver in drv_path")] | |
| #[error("Unknown deriver")] |
src/nix/nix_store.rs
Outdated
| #[derive(Debug)] | ||
| pub struct FromStrError(String); | ||
|
|
||
| impl Display for FromStrError { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| write!(f, "Failed to parse string: {}", self.0) | ||
| } | ||
| } | ||
|
|
||
| impl std::error::Error for FromStrError {} |
src/nix/nix_store.rs
Outdated
| #[derive(Error, Debug)] | ||
| pub enum CommandError { | ||
| #[error("Child process error: {0}")] | ||
| ChildProcessError(#[from] std::io::Error), | ||
| #[error( | ||
| "Process exited unsuccessfully. exit_code={:?}{}", | ||
| exit_code, | ||
| match stderr { | ||
| Some(s) => format!(" stderr={}", s), | ||
| None => "".to_string() | ||
| }, | ||
| )] | ||
| ProcessFailed { | ||
| stderr: Option<String>, | ||
| exit_code: Option<i32>, | ||
| }, | ||
| #[error("Failed to decode command stderr: {0}")] | ||
| Decode(#[from] std::string::FromUtf8Error), | ||
| } |
There was a problem hiding this comment.
Again; why copy this from nix-rs crate?
https://github.com/juspay/nix-rs/blob/2dbba678a9e67925d964dbde24fbd8951ecbd2de/src/command.rs#L213
This simplifies the call site making the code more readable
srid
changed the title
--print-all-dependencies--print-all-dependencies should handle unknown-deriver
When nixci build fails while querying nix-store then we don't get proper error. It displays error like
nix-store --query --requisites --include-outputs failed to run (exited: 1).
We can add stderr messages to have proper error message.