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

feature: add support for installing completions at package build time #779

Merged
merged 4 commits into from
Mar 8, 2024

Conversation

ghthor
Copy link
Contributor

@ghthor ghthor commented Feb 17, 2024

Enables installing the completions at build time (aka during package building) and also simplifies the management of completions in the long run as the only required modification to the shell rc file can be a single line.

I need some pointers on how to add this feature into the documentation, I wasn't able to figure it out but I also didn't give it 100%.

This feature makes the processing of installing the completions into the standard system locations at package build time line up with how the cobra framework does it as well as many other CLI frameworks.

For instance the following is from the nixpkg of a command that uses cobra.

https://github.com/nix-community/gomod2nix/blob/master/default.nix#L39

  postInstall = lib.optionalString (stdenv.buildPlatform == stdenv.targetPlatform) ''
    installShellCompletion --cmd gomod2nix \
      --bash <($out/bin/gomod2nix completion bash) \
      --fish <($out/bin/gomod2nix completion fish) \
      --zsh <($out/bin/gomod2nix completion zsh)
  '' + ''
    wrapProgram $out/bin/gomod2nix --prefix PATH : ${lib.makeBinPath [ go ]}
  '';

For comparison, the aws-sso-cli package would then do something like the following.

---
 pkgs/tools/admin/aws-sso-cli/default.nix | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/pkgs/tools/admin/aws-sso-cli/default.nix b/pkgs/tools/admin/aws-sso-cli/default.nix
index 998da596c06889..d9fc445c8bd929 100644
--- a/pkgs/tools/admin/aws-sso-cli/default.nix
+++ b/pkgs/tools/admin/aws-sso-cli/default.nix
@@ -23,7 +23,12 @@ buildGoModule rec {
     "-X main.Tag=nixpkgs"
   ];
 
-  postInstall = ''
+  postInstall = lib.optionalString (stdenv.buildPlatform == stdenv.targetPlatform) ''
+    installShellCompletion --cmd aws-sso \
+      --bash <($out/bin/aws-sso completions --source --shell=bash) \
+      --fish <($out/bin/aws-sso completions --source --shell=fish) \
+      --zsh <($out/bin/aws-sso completions --source --shell=zsh)
+  '' + ''
     wrapProgram $out/bin/aws-sso \
       --suffix PATH : ${lib.makeBinPath [ xdg-utils ]}
   '';

ghthor/nixpkgs@c7c2ea6

@synfinatic
Copy link
Owner

@ghthor , just so I'm clear, this change is specific for Nix?

@synfinatic
Copy link
Owner

@ghthor, you should be able to rebase & push and the builds should run properly now.

@ghthor
Copy link
Contributor Author

ghthor commented Feb 17, 2024

@ghthor , just so I'm clear, this change is specific for Nix?

It is not. It would apply to any packaging system, though it may need an additional feature to override the executable path. For instance if we wanted to add a PKGBUILD to the AUR I believe we'd need to override the executable path as it wouldn't be the same at build time versus runtime after the package was installed; while with Nix the executable path is the same at build time as runtime.

But this is a general pattern that enables packagers to incorporate the shell completions into package builds. I just used nix in my example as it's my current target to get support for.

Enables installing the completions at build time (aka during package building) and also simplifies the management of completions in the long run as the only required modification to the shell rc file can be a single line.
@ghthor
Copy link
Contributor Author

ghthor commented Feb 28, 2024

Bump @synfinatic anything I can do to help get this merged?

Copy link

codecov bot commented Mar 1, 2024

Codecov Report

Attention: Patch coverage is 56.41026% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 80.32%. Comparing base (1d47cef) to head (ae4abdb).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #779      +/-   ##
==========================================
- Coverage   82.49%   80.32%   -2.16%     
==========================================
  Files          35       37       +2     
  Lines        3266     3405     +139     
==========================================
+ Hits         2694     2735      +41     
- Misses        504      595      +91     
- Partials       68       75       +7     
Files Coverage Δ
internal/utils/fileedit.go 70.99% <45.45%> (-2.34%) ⬇️
internal/helper/helper.go 27.42% <60.71%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d47cef...ae4abdb. Read the comment docs.

@synfinatic
Copy link
Owner

Sorry... been busy with non-SSO related things. Anyways, the proposed code change looks good, but do you mind adding some unit tests and an appropriate comment in the CHANGELOG.md file?

@synfinatic
Copy link
Owner

@ghthor FYI, i've enabled the build job to automatically run without approval for you. shouldn't be any more delays.

@ghthor
Copy link
Contributor Author

ghthor commented Mar 6, 2024

I don't see any benefit to increasing the coverage of the tests for a bunch of toss it and return errors that have no handling behavior other than return. Do see any behavior remaining that would be worthwhile to add test cases for?

@ghthor
Copy link
Contributor Author

ghthor commented Mar 6, 2024

I'm happy to work on improving coverage else where as a followup to this PR

@synfinatic
Copy link
Owner

Thanks for the PR and your patience @ghthor .

@synfinatic synfinatic merged commit 300c63d into synfinatic:main Mar 8, 2024
5 of 7 checks passed
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.

2 participants