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

Generate auto-completion in package build #65

Merged
merged 5 commits into from
Nov 8, 2024

Conversation

ede1998
Copy link

@ede1998 ede1998 commented Nov 6, 2024

Thanks for maintaining this fork.

I noticed that original rip does not offer completions and found your fork.

I modified the package build a bit so that to completions are now generated at package build time and nix will take care that they are available in the standard location that the shell expects. This means that after installation the completions should work out of the box without additional setup/

@MilesCranmer
Copy link
Owner

Thanks!

Is there somewhere I can read about the installShellCompletion command? Also are there other projects that do something like this/is this the standard way?

@ede1998
Copy link
Author

ede1998 commented Nov 6, 2024

I also only found out about it today by looking at zellij packaging:
https://github.com/NixOS/nixpkgs/blob/d063c1dd113c91ab27959ba540c0d9753409edf3/pkgs/tools/misc/zellij/default.nix#L53-L56

The hook is documented here:
https://nixos.org/manual/nixpkgs/stable/#installshellfiles

Btw: The CI failure does not seem related to my PR.

@MilesCranmer
Copy link
Owner

Cool, thanks! And yeah just looks like coveralls gets turned off for external user-triggered CI, will need to fix eventually.

Is there a way the CI can detect if the shell completions were installed correctly or not? Here's the test:

nix:
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os:
- ubuntu-latest
- macOS-latest
steps:
- uses: actions/checkout@v4
- name: Check Nix flake inputs
uses: DeterminateSystems/flake-checker-action@v4
- name: Install Nix
uses: DeterminateSystems/nix-installer-action@v3
- name: Build default package
run: nix-build
- name: Smoke test default package
run: ./result/bin/rip --version
- name: Test default app
run: nix run . -- --help
. I want to make sure it's working correctly from the CI

@ede1998
Copy link
Author

ede1998 commented Nov 7, 2024

Is there a way the CI can detect if the shell completions were installed correctly or not?

Is this what you imagined?

It uses expect to interactively start a bash shell and press TAB to trigger completions, then checks that there are at least some available.

Do you need fish and zsh too? (Though I don't use those so might have some trouble.)

I tested the workflow locally with act and while I did get some weird errors and timeouts, they didn't seem related to my test, which ran successfully after some tinkering.

@MilesCranmer
Copy link
Owner

Brilliant! bash is fine. This is perfect, nice work!

@MilesCranmer
Copy link
Owner

Seems like the completion tests fail on Darwin. I'm happy to filter those tests if they aren't supposed to work, but if we expect them to work, I wonder what the issue is?

@ede1998
Copy link
Author

ede1998 commented Nov 8, 2024

I assumed that they would work. :/
Unfortunately, I don't have a darwin system to test with.
Maybe I can try with the Github CI in my fork.

My current theory is that the bootstrap script from bash-completion fails to source for some reason and my check does not detect it correctly.

Also, I noticed in this log that my test could actually succeed when it should not if enough files are added to the repository. Will fix that too.

@ede1998
Copy link
Author

ede1998 commented Nov 8, 2024

Ok it should now finally work. The issue was that macOS ships an ancient bash version by default which is not supported by bash-completion.

I also refactored and improved the test a bit with more debug switches and links to resources for people unfamiliar with expect since I also struggled with it.

@MilesCranmer
Copy link
Owner

Very nice, thanks!!

Will merge now. Hope you don't mind if I do a git squash so that it will be in conventional commit format, it makes things easier for the release CI to run.

@MilesCranmer MilesCranmer self-requested a review November 8, 2024 21:27
@MilesCranmer MilesCranmer merged commit 0b6241a into MilesCranmer:master Nov 8, 2024
5 of 8 checks passed
@MilesCranmer
Copy link
Owner

P.S., in case you prefer nixpkgs, I added rip2 to it: NixOS/nixpkgs#353742. In theory I guess the automatic completions could be shipped there as well?

ede1998 added a commit to ede1998/nix-config that referenced this pull request Nov 8, 2024
@ede1998
Copy link
Author

ede1998 commented Nov 11, 2024

Thank you, I missed the comment before.

I'll check it out and see if I can add the completions there too if still needed.

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