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

Integrate shell-completion with Pants #16315

Closed
sureshjoshi opened this issue Jul 27, 2022 · 27 comments · Fixed by #19478
Closed

Integrate shell-completion with Pants #16315

sureshjoshi opened this issue Jul 27, 2022 · 27 comments · Fixed by #19478
Assignees

Comments

@sureshjoshi
Copy link
Member

Since #16200 has landed, integration with Pants should be the next focus (plus dedicated ZSH completions wouldn't hurt). The merged PR was strictly a Python script to generate bash completions.

The outstanding questions are:

  • Which goal does this fall under?
  • Is there any way to auto-generate completions when pants.toml is changed?
  • Can the completions be auto-sourced after the first Pants run?
@sureshjoshi
Copy link
Member Author

@Eric-Arellano @benjyw

Any thoughts on the outstanding questions? I'm not really sure in which direction I can take this to integrate with Pants.

From the Slack conversations, export would seem like a decent goal, but not today's version of export, which is more related to venvs

With all the API discussions we've had the past few weeks and my general dislike of API bloat, I'd rather this not be a new goal, but somehow be shoe-horned into something else. I can take a crack at something, but I don't yet know what that "thing" is.

Alternatively (and I'm not super into this either) I could intermezzo this with an experimental-completions goal with the knowledge that "experimental-" prefixed goals will be changed when they graduate.

@benjyw
Copy link
Contributor

benjyw commented Aug 8, 2022

I'd love to play with this, but it wasn't working for me, as we discussed on Slack or on the ticket, I forget which... Has anything been fixed there? Once I get to using it regularly I can form opinions about how to proceed...

@sureshjoshi
Copy link
Member Author

Yep, just pushed two fixes: #16442

Works on Mac and Debian Bullseye for me now, no warnings/complaints.

@Eric-Arellano
Copy link
Contributor

I think we should redesign export, possibly merging export-codegen into it.

If you don't want to be blocked, you could call it experimental-shell-autocomplete.

Is there any way to auto-generate completions when pants.toml is changed?

This would be #12014

@sureshjoshi
Copy link
Member Author

I'm fine with a long-term alias. And it is worth rethinking the "temporary name while a feature is experimental" [habit]?

Maybe we should get more cozy with "experimental" in general?

Or less...

Re: #16397 @benjyw Thoughts? I don't want to start creating new experimentally-namespaced goals, while discussing in another ticket whether we should have them or not.

Here's my entirely selfish perspective of my DX workflow over the last couple weeks.

From mainline Pants, I've generated the most thorough completion.bash I can (python3 build-support/bin/generate_completions.py > pants-completions.bash), and aliased it into my user's completions folder. So, while I may not have the same tools/plugins as mainline in any given repo, I know enough that I can use my auto-complete without any problems.

This isn't the best, as it doesn't react to my pants.toml... But so be it, it's already miles ahead of me trying to remember something, or jumping into help to find the letters to type.

The biggest reason for completions for me was related to introspection commands, which I'm trying to solve by creating that super trivial VSCode extension, so introspection tools are always visible.

This is all to say that I'm totally fine not creating any custom goals until export or whatever is sorted out. Personally, I'm in no rush and any Pants maintainers/contributors can probably end up in the same boat.

The completions might be much more useful for new/casual Pants users, who don't have all the commands memorized. For that case, we could even do something similar to what I'm doing - which is to generate a single, massive completions file, host that on the website and let users know that it might not all work, depending on their pants.toml.

Ugly workaround? Sure - but better than touching the public API in my mind.

@benjyw
Copy link
Contributor

benjyw commented Aug 9, 2022

I want to play with this for a couple of days (now that it should work, after your recent changes), and hopefully that will give me ideas.

@sureshjoshi
Copy link
Member Author

Just so this isn't fully lost to Slack history (between Stu and I):

So what about doing something more like --version? ./pants --completions ?

this would work too probably. goals don’t have access to the help APIs in general, but “BuiltinGoals” do. so a ./pants completions would be possible.

I'll take a crack at this, and see if I can get anything created by Pants for arbitrary repos, and then we can work on naming.

sureshjoshi added a commit to sureshjoshi/pants that referenced this issue Sep 26, 2022
…tins folder

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@stuhood
Copy link
Member

stuhood commented Sep 27, 2022

Thanks a lot @sureshjoshi!

@sureshjoshi
Copy link
Member Author

@stuhood No problem, but we still need an endgame here wrt my Slack thread (https://pantsbuild.slack.com/archives/C0D7TNJHL/p1664221191673119).

However, this leaves me with a couple outstanding questions:

  1. What should the completions "goal" be called? ./pants completions - even if it's contrived, I like the idea of something like ./pants --completions which kinda indicates that it's acting on the pants instance itself, rather than being a verb for some other work
  2. What should the output of said pseudo-goal be? Print the completions to the console? Export to dist? This is probably a per-repo file, since it depends on what the installed backend packages are - so it may not be something trivially exported to a completions folder somewhere on the user's filesystem
  3. Can this be auto-updated if the backend-packages change?

According to Eric, 3 relates to #12014

@stuhood
Copy link
Member

stuhood commented Sep 27, 2022

  1. What should the output of said pseudo-goal be? Print the completions to the console? Export to dist? This is probably a per-repo file, since it depends on what the installed backend packages are - so it may not be something trivially exported to a completions folder somewhere on the user's filesystem

This is an important question, yea. Looking around at prior art would be good here, as I don't know how tools do this in general.

But do note that one annoying issue in this case is that each repo on your system might be using a different version of pants. Any bit of automation that attempted to write out a global config would need to be version aware (and perhaps have a shim at the top level to decide which copy of the completions to use?) Or we could recommend that people use direnv, perhaps.

  1. Can this be auto-updated if the backend-packages change?

According to Eric, 3 relates to #12014

I don't think that it needs to be: because this would be a built-in goal, it would not be running as @rule code, and so it would not be subject to restrictions on what it could read/write to the filesystem (with the caveat that it would need to invent its own memoization if you wanted to avoid checking the filesystem on each run).

  1. What should the completions "goal" be called? ./pants completions - even if it's contrived, I like the idea of something like ./pants --completions which kinda indicates that it's acting on the pants instance itself, rather than being a verb for some other work

If it ends up auto-updated, then I don't think that it would need a goal name: rather, an option name? But perhaps you'd still need a goal for the initial install of completions. It's also possible though that the installation script could bootstrap what is needed there.

@sureshjoshi
Copy link
Member Author

This is an important question, yea. Looking around at prior art would be good here, as I don't know how tools do this in general.

For, let's say, "static" tooling - it often comes down at installation time.
e.g. /usr/local/Cellar/git/2.37.3/share/zsh/site-functions/git-completion.bash which can be symlinked into wherever (I seem to have completions in like, 10 folders on my machine).
/usr/local/share/zsh/site-functions
/usr/local/etc/bash_completion.d/

For something a bit more dynamic, python-fire has a command to generate them and then you pipe those completions wherever you want them:
https://github.com/google/python-fire/blob/master/docs/using-cli.md#completion-flag

It's also possible though that the installation script could bootstrap what is needed there.

My original idea in the long long ago times was that it would be part of the installation script somehow, which also might give us some ability to hook into changes to the backend packages. Felt both like a good and bad idea to be in that script, if we could enable/disable it to the user's preference.

Whether it's a good idea or not, I was thinking that you could source the repo's completions after your first ./pants command, and then the completions would be available and ready-sourced.

Any bit of automation that attempted to write out a global config would need to be version aware (and perhaps have a shim at the top level to decide which copy of the completions to use?)

This is something I didn't think too much about - I'd never seen it done (at least, I don't think I have). If we could dynamically re-direct to the current directory's dist from a global shim, that would be interesting.

@stuhood
Copy link
Member

stuhood commented Sep 27, 2022

This is something I didn't think too much about - I'd never seen it done (at least, I don't think I have). If we could dynamically re-direct to the current directory's dist from a global shim, that would be interesting.

Yea... this would be a bit like a custom implementation of direnv I suppose?

@sureshjoshi
Copy link
Member Author

sureshjoshi commented Sep 27, 2022

Oh yeah, I guess so. I don't currently use direnv (or any shell extensions, actually).

Do we have any idea on the ratios of users to number of Pants repos a given user works on? For me, it's about 10:1 repos per SJ, but I would think that in companies focusing on monorepos, it might be less.

So, if there is a default single-repo path, and then "here is a tool/way/mechanism for a multi-repo case"?

For the single-repo case, I think just printing to console, and then letting the user pipe wherever they so choose could make sense.

Hmm, though, this kinda falls over to backend-package changes... Blah.

@tgolsson
Copy link
Contributor

tgolsson commented Sep 27, 2022

Ok; so... coming in here like a buffoon and probably asking questions you've talked about elsewhere... Wouldn't going for fully dynamic completion be somewhat easier (from a user viewpoint, not necessarily tech)? :-)

_pants () {
    pants="${COMP_WORDS[0]}"; 
    local words=("${COMP_WORDS[@]}")
    local completions=$($pants --complete --word $COMP_CWARD --point $COMP_POINT "${words[@]}")
    COMPREPLY=( $(compgen -W "$completions" -- "$word") )
}

complete -F _pants pants

Works for ./pants, ../pants, foobar/pants, and so on. Solves the "which pants repo" since it'll use the one the user is using. Avoids the whole "is it up to date" conundrum. Even if implementing it like this might turn out too complicated, one could take the "which pants" approach and use that to refresh the list on each run.

@sureshjoshi
Copy link
Member Author

Thanks for the feedback, and yeah, we briefly talked about this yesterday on the slack channel.

In short, it's not a bad idea to do that on tab, BUT, you're at the mercy of the time taken to re-run the pants process on each tab.

On my machine (6-core i5 Mac Mini), I think it ended up somewhere between 0.5-2 seconds per call - with the daemon running, and that's without any real substantial changes to the code base.

If calling ./pants --complete was essentially a no-op, it would be a great approach, but it's a bit more involved. Also, backend packages don't really change frequently, so the "is it up-to-date" should be a rare (but important) check...

In lieu of fully dynamic, as you suggested, putting something in the pants script would be the intermezzo - but I'm just not sure how that would look, with config as well.

@stuhood
Copy link
Member

stuhood commented Sep 27, 2022

On my machine (6-core i5 Mac Mini), I think it ended up somewhere between 0.5-2 seconds per call - with the daemon running, and that's without any real substantial changes to the code base.

Finishing shipping #7369 will unblock improving latency, because it will ease shipping the native-client that is already implemented. It will also touch the pants script.

That will still only be for the "warm pantsd" case, but an approach which can evolve into dynamic would be great.

@tgolsson
Copy link
Contributor

Thanks for the feedback, and yeah, we briefly talked about this yesterday on the slack channel.

In short, it's not a bad idea to do that on tab, BUT, you're at the mercy of the time taken to re-run the pants process on each tab.

On my machine (6-core i5 Mac Mini), I think it ended up somewhere between 0.5-2 seconds per call - with the daemon running, and that's without any real substantial changes to the code base.

Figured as much, but wanted to check. Mine doesn't take that long (<0.5s) which IMO is fine, but 2s would be a bit annoying if it's every tab.

If calling ./pants --complete was essentially a no-op, it would be a great approach, but it's a bit more involved. Also, backend packages don't really change frequently, so the "is it up-to-date" should be a rare (but important) check...

In lieu of fully dynamic, as you suggested, putting something in the pants script would be the intermezzo - but I'm just not sure how that would look, with config as well.

My primary use-case isn't goals, it's targets1. Finding the right target in a directory, finding out which ones exist, etc. That's going to change much more often. I had a bit of a check in how Buck and Bazel do it; and they seem to both have options for either querying via the tool or dumb grepping for targets. Both seem to depend on some known lists of commands and flags as well; likely shipped with the binary. But since both goals and targets (and which goals affect which target) can change (plus depending on location), that is a bit more complex for Pants.

Spitballing here; could the list be maintained by the daemon? So instead of ./pants calling the daemon, the daemon pushes a file to .pants.d?

Footnotes

  1. At least from my experience with Bazel, which admittedly overloads the targets much more than the verbs.

@sureshjoshi
Copy link
Member Author

That's great information!

This first version of the completions only cover the goals/options/subsystems/etc - targets are a step two, as other than filesystem targets, those internal to BUILD files are going to need to be more clever (which is another good use case for dynamically created completions!)

@sureshjoshi
Copy link
Member Author

I've got a new implementation coming down the pipe (hopefully PR this weekend). No more statically generated completions.

The new implementation calls into Pants itself to perform the completion, so we should be able to go repo-to-repo with the same bash completion script.

@grihabor
Copy link
Contributor

Hey @sureshjoshi, thank you for the pr! I can see it is still in draft, what's the status?

@sureshjoshi
Copy link
Member Author

sureshjoshi commented Feb 20, 2024

#19478

That's the proper one I think - I apparently forgot that it was never merged, but I think it was the testing that was falling over.

When I'm back working on Pants in a week or two, I'm going to land this and a few other open items I have.

Specifically, I think I was waiting on closing another VSCode plugin related ticket first, before I came back to this - but the scope of that one increased, and then I ran out of time while landing some work projects

@sureshjoshi
Copy link
Member Author

sureshjoshi commented Feb 24, 2024

Okay, I just recalled what went down here. Making notes for when I come back to this in about 2 weeks:

The overall completion functionality was roughly complete, but I was running into testing issues. The tests themselves were meh, but as I was trying to clean them up and fix them, I ran into a bunch of dreaded Coursier networking/Haskell architecture bugs which effectively kill the workflow on my machine.

Anyways:

  • Ensure tests run and are adequately cleaned up
  • Implementation and testing for exporting Bash and ZSH scripts from Pants (e.g. pants some-goal --bash > completions.bash)
  • Review Bash and ZSH scripts for triggering autocompletion (should we auto-complete the filesystem? Suppress it? Context-based? etc)
    • After playing around more with the ZSH auto-complete, we need to be able to fall back to the filesystem autocompletions - we have stuff like fix/fmt which we can run on a per-file, rather than targets
  • Test target completions Kicked to a separate issue Shell Completions - Target Completions #20675
  • Discussion: Should brew and the pants install script automatically install completions? Deferring this until we're happy with the results
  • Long-term: First-run suffers from the slow Pants startup, which should be mitigated by Introduce call-by-name syntax for @rules #19730
    • There's literally no action in this ticket... We could be clever by adding caching, but that feels overly complex - when we should really solve the underlying issue

@sureshjoshi
Copy link
Member Author

@benjyw @kaos
Do we have a location where we can store loose files, and pull them into the built-in goals anywhere?

e.g. I wanted to store the bash and zsh completions as separate files, and then just print them out using the CLI, in the event anyone wanted to be able to download them out-of-band of Pants itself (e.g. automation scripts).

Not sure where to put them though

@sureshjoshi
Copy link
Member Author

The closest analogy I have are the various per-backend/per-tool config or lock files, which are inline.

@benjyw
Copy link
Contributor

benjyw commented Mar 15, 2024

@benjyw @kaos Do we have a location where we can store loose files, and pull them into the built-in goals anywhere?

e.g. I wanted to store the bash and zsh completions as separate files, and then just print them out using the CLI, in the event anyone wanted to be able to download them out-of-band of Pants itself (e.g. automation scripts).

Not sure where to put them though

Would these be per-backend? How would the backends I opt in to affect the completions?

I think it's fine to create a src/resources/pants and add files there. Better than src/python/pants/resources, because we may want to access such files from Rust. We can add src/resources as a source root, so that Python code can load files as resources.

@sureshjoshi
Copy link
Member Author

@benjyw They'd be single-instance files. For instance, this is something like the zsh completion:

#compdef _pants_completions pants

# zsh completion support for Pants
function _pants_completions() {
  compadd $(pants complete -- "${words[@]}")
}

Right now, they just live next to the completions.py, but ... yeah, doesn't feel great.

@kaos
Copy link
Member

kaos commented Mar 15, 2024

Right now, they just live next to the completions.py, but ... yeah, doesn't feel great.

I don't have any issue with them living next to the python source that use them. Consider the alternative of having the shell source verbatim inline in the python source file.. we've just made them easier to work with by placing them in their own source file.

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