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/fish: generate autocompletions from man pages #56446

Merged
merged 1 commit into from Feb 28, 2019

Conversation

@hyperfekt
Copy link
Contributor

@hyperfekt hyperfekt commented Feb 27, 2019

Motivation for this change

#52464, but fixed.
Any colliding completions will override just as the system path does, but we prevent generating collision warnings for completions that are actually identical by not writing the source path into the completion file.
Also we have a small test now that should cover the effect of this PR.

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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@samueldr
Copy link
Member

@samueldr samueldr commented Feb 27, 2019

Here's the actual diff (excluding test), compared to the state before the revert:

(Made this by squashing the revert commit and a commit of these changes together.)

diff --git a/nixos/modules/programs/fish.nix b/nixos/modules/programs/fish.nix
index 03d6c26c8c8..1fe59407943 100644
--- a/nixos/modules/programs/fish.nix
+++ b/nixos/modules/programs/fish.nix
@@ -182,11 +182,38 @@ in
 
     environment.etc."fish/generated_completions".source =
       let
+        patchedGenerator = pkgs.stdenv.mkDerivation {
+          name = "fish_patched-completion-generator";
+          srcs = [
+            "${pkgs.fish}/share/fish/tools/create_manpage_completions.py"
+            "${pkgs.fish}/share/fish/tools/deroff.py"
+          ];
+          unpackCmd = "cp $curSrc $(basename $curSrc)";
+          sourceRoot = ".";
+          patches = [ (pkgs.writeText "fish_completion-generator_patch" ''
+            --- a/create_manpage_completions.py
+            +++ b/create_manpage_completions.py
+            @@ -776,8 +776,6 @@ def parse_manpage_at_path(manpage_path, output_directory):
+
+                         built_command_output.insert(0, "# " + CMDNAME)
+
+            -            # Output the magic word Autogenerated so we can tell if we can overwrite this
+            -            built_command_output.insert(1, "# Autogenerated from man page " + manpage_path)
+                         # built_command_output.insert(2, "# using " + parser.__class__.__name__) # XXX MISATTRIBUTES THE CULPABILE PARSER! Was really using Type2 but reporting TypeDeroffManParser
+
+                         for line in built_command_output:
+          '') ];
+          dontBuild = true;
+          installPhase = ''
+            mkdir -p $out
+            cp * $out/
+          '';
+        };
         generateCompletions = package: pkgs.runCommand
-          "${package.name}-fish-completions"
+          "${package.name}_fish-completions"
           (
             {
-              src = package;
+              inherit package;
               nativeBuildInputs = [ pkgs.python3 ];
               buildInputs = [ pkgs.fish ];
               preferLocalBuild = true;
@@ -196,13 +223,14 @@ in
           )
           ''
             mkdir -p $out
-            if [ -d $src/share/man ]; then
-              find $src/share/man -type f | xargs python ${pkgs.fish}/share/fish/tools/create_manpage_completions.py --directory $out >/dev/null
+            if [ -d $package/share/man ]; then
+              find $package/share/man -type f | xargs python ${patchedGenerator}/create_manpage_completions.py --directory $out >/dev/null
             fi
           '';
       in
         pkgs.buildEnv {
           name = "system-fish-completions";
+          ignoreCollisions = true;
           paths = map generateCompletions config.environment.systemPackages;
         };
 
diff --git a/nixos/tests/all-tests.nix b/nixos/tests/all-tests.nix
index 65227857a38..2ddb54bcc3d 100644
--- a/nixos/tests/all-tests.nix
+++ b/nixos/tests/all-tests.nix
@@ -74,6 +74,7 @@ in
   ferm = handleTest ./ferm.nix {};
   firefox = handleTest ./firefox.nix {};
   firewall = handleTest ./firewall.nix {};
+  fish = handleTest ./fish.nix {};
   flannel = handleTestOn ["x86_64-linux"] ./flannel.nix {};
   flatpak = handleTest ./flatpak.nix {};
   fsck = handleTest ./fsck.nix {};
@hyperfekt hyperfekt force-pushed the hyperfekt:fish_generate-completions branch from f051427 to 79c7e7f Feb 27, 2019
Copy link
Member

@dotlambda dotlambda left a comment

Thanks a lot for this!

nixos/modules/programs/fish.nix Outdated Show resolved Hide resolved
nixos/modules/programs/fish.nix Outdated Show resolved Hide resolved
nixos/modules/programs/fish.nix Show resolved Hide resolved
nixos/modules/programs/fish.nix Show resolved Hide resolved
nixos/modules/programs/fish.nix Outdated Show resolved Hide resolved
nixos/tests/fish.nix Outdated Show resolved Hide resolved
@dotlambda
Copy link
Member

@dotlambda dotlambda commented Feb 27, 2019

I can confirm that this PR produces no errors with my current system configuration. 🎉

@hyperfekt hyperfekt force-pushed the hyperfekt:fish_generate-completions branch from 79c7e7f to bc6db9c Feb 27, 2019
@dotlambda
Copy link
Member

@dotlambda dotlambda commented Feb 27, 2019

You forgot to git add the patch.

@hyperfekt hyperfekt force-pushed the hyperfekt:fish_generate-completions branch from bc6db9c to 407c24d Feb 27, 2019
@hyperfekt hyperfekt force-pushed the hyperfekt:fish_generate-completions branch from 407c24d to 3731835 Feb 27, 2019
@hyperfekt
Copy link
Contributor Author

@hyperfekt hyperfekt commented Feb 27, 2019

You forgot to git add the patch.

Argh. Fixed.

@gebner
Copy link
Member

@gebner gebner commented Feb 28, 2019

Also resolves the issue with the collisions for me.

@gebner gebner merged commit 80812af into NixOS:master Feb 28, 2019
9 checks passed
9 checks passed
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Apr 9, 2020

I was wondering if the generation could be implemented in a way such that the completion are in a single derivation. Currently I'm seeings hunders of _fish-completion derivations being built: it's not a big deal but it's quite noisy. Also there is no option to disable the generation, right?

@hyperfekt
Copy link
Contributor Author

@hyperfekt hyperfekt commented Apr 9, 2020

This is fundamentally a tradeoff between noise and performance on incremental package additions to the environment - creating completions for everything in Python takes a significant amount of time which I did not want to burden every new package installation with.
There is indeed no option to disable the generation because the generated completions shouldn't have a negative effect given they have a low priority.
If others find as well that the noise when initially building a system is too high to tolerate it could be worth the UI complexity of adding a toggle. If not, you might just (which reminds me we don't really have a good mechanism for that) want to patch the module to disable the generation.

@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Apr 10, 2020

creating completions for everything in Python takes a significant amount of time which I did not want to burden every new package installation with.

I didn't think of that, you are right: a single derivation would have to be rebuilt every time and it's definitely worse.

If others find as well that the noise when initially building a system is too high to tolerate it could be worth the UI complexity of adding a toggle.

Probably not, as I said it's not a big deal.

Thank you for replying and sorry for the trouble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.