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.

Sign up for GitHub

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

Jump to bottom

nix doctor: add more logging output to checks #3102

Merged
merged 1 commit into from Nov 5, 2019
Merged

nix doctor: add more logging output to checks #3102

merged 1 commit into from Nov 5, 2019

Conversation

bhipple
Copy link
Contributor

@bhipple bhipple commented Sep 21, 2019

When running nix doctor on a healthy system, it just prints the store URI and
nothing else. This makes it unclear whether the system is in a good state and
what check(s) it actually ran, since some of the checks are optional depending
on the store type.

This commit updates nix doctor to print an INFO log for every check that it
does, and to print a final message saying whether all checks passed or not. This
makes it clear to the user whether they've passed the checkup or not with the doctor.

Fixes #3084

@bhipple
Copy link
Contributor Author

bhipple commented Sep 21, 2019

New output looks like this:

λ brh nix →  ./inst/bin/nix doctor
INFO: Store uri = daemon
INFO: Checking that there is only one nix on PATH.
INFO: Checking that all profiles are gcroots.
INFO: Checking that client and daemon store protocol version match.
INFO: All checks passed!

And when in the nix-shell, with two nix binaries in my $PATH:

[nix-shell:~/src/nix]$ ./inst/bin/nix doctor
INFO: Store uri = daemon
INFO: Checking that there is only one nix on PATH.
WARNING: multiple versions of nix found in PATH.

  /home/bhipple/src/nix/inst/bin
  /nix/store/j18b1ryh2mkrp0k9b1vq1hyq3289qsgr-nix-2.2.2/bin

INFO: Checking that all profiles are gcroots.
INFO: Checking that client and daemon store protocol version match.
WARNING: Not all checks passed!

zimbatm
zimbatm approved these changes Sep 22, 2019
View reviewed changes
Copy link
Member

@zimbatm zimbatm left a comment

Choose a reason for hiding this comment

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

nice!

LnL7
LnL7 approved these changes Sep 23, 2019
View reviewed changes
src/nix/doctor.cc Outdated
}

bool checkNixInPath()
{
std::cout << "INFO: Checking that there is only one nix on PATH." << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

While this is useful information it might be better to only log the steps on debug level instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Logically this feels like a test suite, which generally print all the tests run with their status. Particularly with the colorized output now, there's something to be said for the warm fuzzy feeling of seeing a bunch of green PASSes scroll by :)

Since the tests that we run are conditional on the system config, as well, I think it's reasonable to print them all. Maybe if someday we have dozens of checks it might be worth reconsidering, but as of now we only have 3.

@edolstra
Copy link
Member

edolstra commented Oct 1, 2019

Maybe we can colorize the output? Like in the warn function:

void Logger::warn(const std::string & msg)
{
    log(lvlWarn, ANSI_RED "warning:" ANSI_NORMAL " " + msg);
}

@bhipple
nix doctor: add more logging output to checks
c5bd564 
When running nix doctor on a healthy system, it just prints the store URI and
nothing else. This makes it unclear whether the system is in a good state and
what check(s) it actually ran, since some of the checks are optional depending
on the store type.

This commit updates nix doctor to print an colored log message for every check
that it does, and explicitly state whether that check was a PASS or FAIL to make
it clear to the user whether the system passed its checkup with the doctor.

Fixes NixOS#3084
@bhipple
Copy link
Contributor Author

bhipple commented Oct 6, 2019

Thanks for the tip @edolstra! I've rewritten the output to use the standard Logger and emit consistent, colorized output like so:

out

In the first run I'm in the nix-shell, so I fail the check that I only have a single nix binary in $PATH; in the second run, I pass all checks.

Looks nice with the colors now!

@bhipple
Copy link
Contributor Author

bhipple commented Oct 20, 2019

@edolstra any thoughts on how it looks now? We could also use the systemd output format, which would be:

[  OK  ] Foo
[FAILED] Bar
[  OK  ] Baz

vs.

[PASS] Foo
[FAIL] Bar
[PASS] Baz

@edolstra edolstra merged commit c5bd564 into NixOS:master Nov 5, 2019
@edolstra
Copy link
Member

edolstra commented Nov 5, 2019

Thanks!

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-optimise-store-skipping-suspicious-writable-file/5394/2

@davidak davidak mentioned this pull request Mar 31, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimbatm zimbatm zimbatm approved these changes

@LnL7 LnL7 LnL7 approved these changes

@Ma27 Ma27 Ma27 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nix doctor: actually answer if there are problems or not
6 participants
@bhipple @edolstra @nixos-discourse @zimbatm @LnL7 @Ma27