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

nixos/activation: Identifies the snippet that failed #44526

Merged

Conversation

samueldr
Copy link
Member

@samueldr samueldr commented Aug 5, 2018

This allows a developer to better identify in which snippet the
failure happened. Furthermore, users seeking help will have more
information available about the failure.

Motivation for this change

Talked about the idea briefly earlier today on #nixos

EDIT Removed a misleading request for comments, see later comment form myself

Things done
  • ✔️ Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • ✔️ NixOS
    • ⬜ macOS
    • ⬜ other Linux distributions
  • ⬜ Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • ⬜ Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • ⬜ Tested execution of all binary files (usually in ./result/bin/)
  • ⬜ Determined the impact on package closure size (by running nix path-info -S before and after)
  • ✔️ Fits CONTRIBUTING.md.

This allows a developer to better identify in which snippet the
failure happened. Furthermore, users seeking help will have more
information available about the failure.
@infinisil
Copy link
Member

Alternate suggestion for the output-to-snippet mapping: Prepend the snippet name to every message:

activating the configuration...
[etc] setting up /etc...
[foophase] Hello there
setting up tmpfiles

@samueldr
Copy link
Member Author

samueldr commented Aug 6, 2018

Prepend the snippet name to every message

Unless I'm mistaken, this is hard to do right within bash. One of the issue is the way stdin and stderr will be interleaved, without guarantee about there being a newline! Though I must say, it would look fine if it was implemented that way.

@rycee
Copy link
Member

rycee commented Aug 6, 2018

@samueldr Interesting approach! Home Manager prints a line for each entered activation script block and I find it a bit too chatty. I might adopt this method to only print the failing block unless verbose output is enabled 🙂

@samueldr
Copy link
Member Author

samueldr commented Aug 6, 2018

unless verbose output is enabled

Do you know any equivalent where we could activate the chatty version for the activation script?

@rycee
Copy link
Member

rycee commented Aug 6, 2018

@samueldr In Home Manager the "chatty" version is the default and there isn't really a non-chatty option. My idea would be to have the default be non-chatty (i.e. only print the failing block) and have the chatty one when using the -v option.

So, currently I get the output

Starting home manager activation
Activating checkFilesChanged
Activating checkLinkTargets
Activating writeBoundary
Activating uninstallHomeManager
Activating installPackages
replacing old 'home-manager-path'
installing 'home-manager-path'
Activating createHomeInfoDir
Activating createMaildir
Activating gnomeTerminal
Activating linkGeneration
Cleaning up orphan links from /home/rycee
No change so reusing latest profile generation 1306
Creating home file links in /home/rycee
Activating onFilesChange
Activating reloadSystemD
Starting: random-background.service setxkbmap.service
Activating useGtkThemeInQt4

by default but would instead get

Starting home manager activation
replacing old 'home-manager-path'
installing 'home-manager-path'
Cleaning up orphan links from /home/rycee
No change so reusing latest profile generation 1306
Creating home file links in /home/rycee
Starting: random-background.service setxkbmap.service

@samueldr
Copy link
Member Author

So uh, any reviews, updates?

@edolstra, any reasons this shouldn't be merged? (Singling you out since I'd like someone with stage-1/activation background to agree or disagree) Others fitting the bill, feel free to chime in.

@edolstra
Copy link
Member

This looks good to me, but the proposal to spam the console with dozens of activation script names is definitely not a good idea, under the principle that programs should be quiet unless they have something interesting to say.

@samueldr
Copy link
Member Author

Yes, the proposal was conditional to there being a way to opt-in to a verbose mode; which there isn't.

@samueldr samueldr added this to the 18.09 milestone Aug 28, 2018
@samueldr
Copy link
Member Author

samueldr commented Sep 2, 2018

I have amended the description...

What wat previously in the description was a small request for comments:


This, though feels a bit barren. It would be nice if there was some other mechanisms allowing to match the start of the output to the specific snippet, and not only the failure state.

For that, I had something like what follows in mind, though it causes a very verbose output (which, too, follows).

diff --git a/nixos/modules/system/activation/activation-script.nix b/nixos/modules/system/activation/activation-script.nix
index 1058b212b7a..d632b20e930 100644
--- a/nixos/modules/system/activation/activation-script.nix
+++ b/nixos/modules/system/activation/activation-script.nix
@@ -8,6 +8,7 @@ let
   addAttributeName = mapAttrs (a: v: v // {
     text = ''
       #### Activation script snippet ${a}:
+      printf " -> '%s'\n" "${a}"
       _localstatus=0
       ${v.text}
@santaslittlehelper .../nixos/nixpkgs $ time sudo nixos-rebuild switch --build-host samuel@duffman.local
building Nix...
warning: you did not specify '--add-root'; the result might be removed by the garbage collector
building the system configuration...
warning: you did not specify '--add-root'; the result might be removed by the garbage collector
warning: you did not specify '--add-root'; the result might be removed by the garbage collector
updating GRUB 2 menu...
activating the configuration...
 -> 'stdio'
 -> 'binsh'
 -> 'domain'
 -> 'users'
 -> 'groups'
 -> 'etc'
setting up /etc...
 -> 'hostname'
 -> 'specialfs'
 -> 'modprobe'
 -> 'nix'
 -> 'polkit'
 -> 'var'
 -> 'resolvconf'
 -> 'setup-opengl'
 -> 'systemd'
 -> 'udevd'
 -> 'udisks2'
 -> 'upower'
 -> 'usrbinenv'
 -> 'wrappers'
setting up tmpfiles

Suggestions welcome.

@grahamc grahamc merged commit f14b6cb into NixOS:master Sep 2, 2018
@grahamc
Copy link
Member

grahamc commented Sep 2, 2018

I like the more verbose output, but maybe it'd be tolerable if it was one line:

activating: resolvconf, setup-opegnl, 

which was appended to until done.

@samueldr samueldr deleted the feature/actiavation-failure-identification branch September 2, 2018 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants