cmd/syncthing: Add CLI completion functionality (implements #8616) #9226

danpadcz · 2023-11-15T13:17:47Z

Purpose

This implements CLI completion using the Kongplete module. As a side effect a CLI structure for syncthing/cli was created for kongplete to be able to parse and implement CLI completion.

This pull request implements issue #8616.

Testing

I've tested the autocompletion manually, and it had worked, but I hadn't added any tests so as to test it automatically. Additionally, I ran go run build.go test with all tests passing.

The completion can be tested when running the output of syncthing install-completions in bash. Additionally, I would like to ask the maintainers, if and where should the install-completions command be placed?

Authorship

Your name and email will be added automatically to the AUTHORS file
based on the commit metadata.

danpadcz · 2023-11-20T08:28:20Z

Hi, as was recommended in the original issue, I used kongplete for the cli completion, but since the syncthing cli subprogram uses "github.com/urfave/cli" instead of kong as the cli parser, I decided to create a struct representing the syncthing cli subprogram that I inserted into the kong cli parsing structure in cmd/syncthing/main.go. This enabled kongplete to create cli completion for the syncthing cli subprogram aswell. Is this a valid way of implementing cli completion for this project?

AudriusButkevicius · 2023-11-22T19:45:30Z

The whole point of the CLI is that it's auto generated from the structs, so we don't need to maintain the CLI, and every time we change the config, the CLI immediately supports setting that config element.

Maintaining this large struct by hand is definitely not the direction we want to go, as it will be out of date pretty quickly.
We should use the same reflective approach for generating completion as well.

danpadcz · 2023-11-29T14:49:39Z

That's understandable. Do you have an idea on how to implement the completion since it can't fully be autogenerated by kongplete due to the CLI subprogram using a different argument parser than kong?

AudriusButkevicius · 2023-11-29T21:32:58Z

I really don't know how shell completion works, as in, I don't know what you need to provide to the shell and at what point in time, but whatever that is, we should do that the same way we do the CLI generation, reflectively on the fly.

Seems that the library we use suggests it already supports it (https://cli.urfave.org/v2/examples/bash-completions/). The recli package we use for reflectiveness might need making it work with newer version of urfave/cli and perhaps we'd get completion if we just upgraded.

AudriusButkevicius · 2023-11-29T21:33:22Z

Actually, even v1 supports completion, so perhaps it's just switching the flag on.

AudriusButkevicius · 2023-11-29T21:44:59Z

Ok, the problem is a bit more convoluted, as the project actually uses two different command line parsers already.
One for normal entrypoint, the other one for CLI.
The normal entrypoint delegates to CLI after the CLI subcommand, so the completion for both (or even either maybe?) might be hard.

AudriusButkevicius · 2023-11-29T22:06:41Z

Seems that you enable that flag in urfave/cli it already does completion:

root@mail ~/syncthing # ./bin/syncthing cli config gui --generate-bash-completion
enabled
raw-address
raw-unix-socket-permissions
user
password
auth-mode
raw-use-tls
apikey
insecure-admin-access
theme
debugging
insecure-skip-host-check
insecure-allow-frame-loading
send-basic-auth-prompt
dump-json
help
h

So I think you just need a bit of glue (potentially fork of kongcomplete) to marry the two together.

danpadcz · 2023-12-09T16:36:06Z

In the end I tried to force urfave/cli completion generation to cooperate with the way kongplete does completion, but for the completion in the cli subcommand to work properly, we need to install it differently than kongplete does. Thats because urfave/cli returns all subcommands regardless of current partial input when doing completion, so I simply added a filter that removes all non-matching offered results.

So the changed installation command will be complete -C path_to_syncthing_binary -X '!&*' syncthing.
This seems like a much better solution than having a manually created structure to me

danpadcz · 2023-12-11T18:19:58Z

Sorry for the mention that just showed up, I am working on this as part of a school project and didn't realise linking this PR in another PR would also show up here. You can just ignore it :D

AudriusButkevicius · 2023-12-11T18:47:55Z

Could you please rebase it on latest master. I'll try to give it a go at home tonight.

AudriusButkevicius · 2023-12-11T18:48:32Z

cmd/syncthing/main.go

@@ -216,7 +217,11 @@ func main() {
 	// The "cli" subcommand uses a different command line parser, and e.g. help
 	// gets mangled when integrating it as a subcommand -> detect it here at the
 	// beginning.
-	if len(os.Args) > 1 && os.Args[1] == "cli" {
+	doCompgen := (strings.Contains(os.Getenv("COMP_LINE"), " cli") && os.Getenv("SHELL") == "/bin/bash")


Some explanation on what the intent of these checks would be nice. Why do we care if its bash or sh etc? Are the interfaces to these different?

AudriusButkevicius · 2023-12-11T19:51:55Z

Didn't realise quite a large PR landed that affects this area, so rebasing might be a bit of work. Sorry.

…#8616)

danpadcz · 2023-12-11T22:27:04Z

The changes on main today replaced urfave/cli with kong for all parts of the cli subprogram except for cli config and so completion can now be added for a large part of the project with one line of code :D
The rewrites made also mean that my gluing the two parsers together no longer works, so I removed that. And since completion is missing for only one part, I figured it might be better to let it be for now?

AudriusButkevicius · 2023-12-11T23:16:57Z

go.mod

@@ -57,19 +57,25 @@ require (
 	google.golang.org/protobuf v1.31.0
 )

+require github.com/willabides/kongplete v0.3.0
+


Did you add it using go get? The fact that it's in a separate block is strange.

I think I did, should I move it into the first block instead?

AudriusButkevicius · 2023-12-15T19:20:29Z

I don't see us adding a command for installing the completions, so I've built this locally, but not obvious to me how to set it up, as I assumed you need kongplete.InstallCompletions command somewhere.

danpadcz · 2023-12-16T08:52:44Z

The kongplete.InstallCompletions command gives you a command you run in your shell that adds completion for the current shell. So for bash the command is for example of the format complete -C /usr/bin/syncthing syncthing.
A user that wants to install completions for all future shell instances will save this command in a script in /etc/bash_completion.d/ or anywhere else if they prefer.
For easier installation it could be nice to have the kongplete.InstallCompletions, somewhere, but I also am not sure where it would be intuitive to place and also the user will still need to know where to place the command according to their own shell preference.

* main: Update dependencies (syncthing#9321) gui: Always inform about loading data in Restore Versions modal (syncthing#9317) lib/build: Allow semver build in version regex (fixes syncthing#9267) (syncthing#9316) gui: Keep short deviceID length consistent + xrefs (fixes syncthing#9313) (syncthing#9314) build(deps): bump actions/download-artifact from 3 to 4 (syncthing#9294) build(deps): bump actions/upload-artifact from 3 to 4 (syncthing#9293) gui, man, authors: Update docs, translations, and contributors gui, lib/scanner: Improve scan progress indication (ref syncthing#8331) (syncthing#9308) lib/protocol: handle empty names in unixOwnershipEqual (fixes syncthing#9039) (syncthing#9306) gui, man, authors: Update docs, translations, and contributors etc/linux-desktop: use double dash for long options (syncthing#9301) lib/connections: Skip allocation in check for missing port (syncthing#9297) lib/upgrade: Extract signing key to embedded file (fixes syncthing#9247) (syncthing#9296) gui, man, authors: Update docs, translations, and contributors build: Update quic-go (fixes syncthing#9287) lib/model: Only handle relevant folder summaries (kqueue) (fixes syncthing#9183) (syncthing#9288)

calmh · 2024-01-04T10:15:20Z

I added an install-completions entrypoint, because otherwise I couldn't figure out how to test it on my zsh :)

We'll have to add a doc entry for how to use this.

* main: (24 commits) lib/ignore: Refactor out result type (syncthing#9343) build: Testing infra images for infra-* branches lib/versioner: Expand tildes in version directory (fixes syncthing#9241) (syncthing#9327) lib/scanner: Prevent sync-conflict for receive-only local modifications (syncthing#9323) gui, man, authors: Update docs, translations, and contributors Fix website security link in README.md (syncthing#9325) cmd/syncthing: Add CLI completion functionality (fixes syncthing#8616) (syncthing#9226) lib/api: Save session & CSRF tokens to database, add option to stay logged in (fixes syncthing#9151) (syncthing#9284) Update dependencies (syncthing#9321) gui: Always inform about loading data in Restore Versions modal (syncthing#9317) lib/build: Allow semver build in version regex (fixes syncthing#9267) (syncthing#9316) gui: Keep short deviceID length consistent + xrefs (fixes syncthing#9313) (syncthing#9314) build(deps): bump actions/download-artifact from 3 to 4 (syncthing#9294) build(deps): bump actions/upload-artifact from 3 to 4 (syncthing#9293) gui, man, authors: Update docs, translations, and contributors gui, lib/scanner: Improve scan progress indication (ref syncthing#8331) (syncthing#9308) lib/protocol: handle empty names in unixOwnershipEqual (fixes syncthing#9039) (syncthing#9306) gui, man, authors: Update docs, translations, and contributors etc/linux-desktop: use double dash for long options (syncthing#9301) lib/connections: Skip allocation in check for missing port (syncthing#9297) ...

danpadcz force-pushed the cli_completion branch from d1bb32f to 1cf25c2 Compare December 9, 2023 16:20

danpadcz mentioned this pull request Dec 11, 2023

Odevzdání projektu do PB173 danpadcz/fakedrawerinbrno#2

Open

AudriusButkevicius reviewed Dec 11, 2023

View reviewed changes

cmd/syncthing: Add CLI completion functionality (implements syncthing…

78db3af

…#8616)

danpadcz force-pushed the cli_completion branch from 1cf25c2 to 78db3af Compare December 11, 2023 22:07

AudriusButkevicius approved these changes Dec 11, 2023

View reviewed changes

calmh added 2 commits January 4, 2024 11:08

install-completions

0f47ebc

calmh enabled auto-merge (squash) January 4, 2024 10:16

calmh merged commit 9387e10 into syncthing:main Jan 4, 2024
19 checks passed

calmh added this to the v1.27.3 milestone Jan 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/syncthing: Add CLI completion functionality (implements #8616) #9226

cmd/syncthing: Add CLI completion functionality (implements #8616) #9226

danpadcz commented Nov 15, 2023

danpadcz commented Nov 20, 2023

AudriusButkevicius commented Nov 22, 2023

danpadcz commented Nov 29, 2023

AudriusButkevicius commented Nov 29, 2023

AudriusButkevicius commented Nov 29, 2023

AudriusButkevicius commented Nov 29, 2023

AudriusButkevicius commented Nov 29, 2023

danpadcz commented Dec 9, 2023

danpadcz commented Dec 11, 2023

AudriusButkevicius commented Dec 11, 2023

AudriusButkevicius Dec 11, 2023 •

edited

AudriusButkevicius commented Dec 11, 2023

danpadcz commented Dec 11, 2023

AudriusButkevicius Dec 11, 2023

danpadcz Dec 15, 2023

AudriusButkevicius commented Dec 15, 2023

danpadcz commented Dec 16, 2023

calmh commented Jan 4, 2024

cmd/syncthing: Add CLI completion functionality (implements #8616) #9226

cmd/syncthing: Add CLI completion functionality (implements #8616) #9226

Conversation

danpadcz commented Nov 15, 2023

Purpose

Testing

Authorship

danpadcz commented Nov 20, 2023

AudriusButkevicius commented Nov 22, 2023

danpadcz commented Nov 29, 2023

AudriusButkevicius commented Nov 29, 2023

AudriusButkevicius commented Nov 29, 2023

AudriusButkevicius commented Nov 29, 2023

AudriusButkevicius commented Nov 29, 2023

danpadcz commented Dec 9, 2023

danpadcz commented Dec 11, 2023

AudriusButkevicius commented Dec 11, 2023

AudriusButkevicius Dec 11, 2023 • edited

Choose a reason for hiding this comment

AudriusButkevicius commented Dec 11, 2023

danpadcz commented Dec 11, 2023

AudriusButkevicius Dec 11, 2023

Choose a reason for hiding this comment

danpadcz Dec 15, 2023

Choose a reason for hiding this comment

AudriusButkevicius commented Dec 15, 2023

danpadcz commented Dec 16, 2023

calmh commented Jan 4, 2024

AudriusButkevicius Dec 11, 2023 •

edited