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

Use --quiet --quiet by default for om ci and om show #324

Closed
wants to merge 6 commits into from

Conversation

shivaraj-bh
Copy link
Member

resolves #300

@shivaraj-bh shivaraj-bh marked this pull request as draft October 18, 2024 22:14
@shivaraj-bh shivaraj-bh marked this pull request as ready for review October 18, 2024 22:45
@shivaraj-bh
Copy link
Member Author

shivaraj-bh commented Oct 18, 2024

I just realised, om ci might not need it yet, because devour_flake.rs pipes both stdout and stderr

let mut output_fut = cmd.stdout(Stdio::piped()).stderr(Stdio::piped()).spawn()?;

Edit: It will be useful during nix flake check when this case is hit:

for (k, v) in &subflake.override_inputs {
args.extend(["--override-input", k, v]);
}

@srid
Copy link
Member

srid commented Oct 19, 2024

In [what] situations do we need this done for om show?

@shivaraj-bh
Copy link
Member Author

In situations do we need this done for om show?

When nix_eval is called trying to fetch flake schemas:

let v = nix_eval::<Self>(nix_cmd, &flake_opts, &inspect_flake).await?;

@shivaraj-bh
Copy link
Member Author

On a second thought, it would be more appropriate to localise modifying NixCmd with --quiet --quiet right before where it is needed.

@srid
Copy link
Member

srid commented Oct 19, 2024

In [what] situations do we need this done for om show?

When nix_eval is called trying to fetch flake schemas:

Not sure I understand. I don't see any override input relate logs when running, for example,
image

@shivaraj-bh
Copy link
Member Author

The logs from --override-input are not shown in om show because we add --quiet --quiet by default in nix_eval. This PR allows to append --quiet --quiet directly to the NixCmd, instead of being specific to nix_eval and thus expanding the scope to use it in NixCmd used during om ci.

@@ -119,6 +124,13 @@ impl NixCmd {
cmd
}

/// Suppress logs related to `override-input` usage by reducing `nix` command's verbosity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this (Suppress logs related to override-input usage) the only effect of using --quiet --quiet or does it suppress anything else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it does more than that, then function (mute_override_input_logs) wouldn't be accurate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logically, it reduces verbosity level of the logs. I could rename the function to depict that.

Copy link
Member

@srid srid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shivaraj-bh I tried out this PR locally and there appears to be a bug.

Basically it suppresses the entire output of nix build (I see no build logs):

Compare this branch:

image

vs main:

image

@@ -40,6 +40,10 @@ pub struct NixCmd {
#[cfg_attr(feature = "clap", arg(long))]
pub extra_access_tokens: Vec<String>,

/// Arguments to pass verbatim to the Nix command
#[cfg_attr(feature = "clap", arg(last = true))]
pub extra_args: Vec<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of extra_args, just add a quiet field looking like this. It is more self-documenting.

https://github.com/clap-rs/clap-verbosity-flag/blob/a7e305e8b0bde21be222ae658cc14813464ea3e6/src/lib.rs#L79-L88

@srid
Copy link
Member

srid commented Nov 8, 2024

Okay it doesn't suppress the entire output, but it certainly suppresses some of it, leaving just:

image

@srid
Copy link
Member

srid commented Nov 8, 2024

Closing per #300 (comment)

@srid srid closed this Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use --quiet --quiet wherever possible to Nix
2 participants