cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong #9166
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
So, I'm very positive to this change, but there are remnants of urfave/cli in at least some place in the cli code. Do you want to try to get rid of that, or is it deemed to tricky? |
|
I'd like to leave it alone, at least in this PR... |
* main: (69 commits) lib/nat: Fix test build failure (ref syncthing#9010) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010) gui, man, authors: Update docs, translations, and contributors gui: Show folder/device status on small screens (syncthing#8643) lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271) build: Update dependencies (syncthing#9265) build: Revert specifics for Go 1.21.4, build using Go 1.21.5 (syncthing#9264) lib/fs: Reduce memory usage in xattrs handling (syncthing#9251) lib/model: Improve LastSeen handling (syncthing#9256) lib/scanner: Record inode change time for directories and symlinks (syncthing#9250) lib/api: Improve ignore loading error handling (fixes syncthing#9253) (syncthing#9254) gui, man, authors: Update docs, translations, and contributors lib/fs: Better equality comparison in mtimefs cmd/stcrashreceiver: Add metrics for diskstore inventory cmd/stcrashreceiver: Minor cleanup, stricter file permissions cmd/stcrashreceiver: Add metrics for incoming reports ...
calmh
approved these changes
Dec 11, 2023
| } | ||
| cmd := exec.Command(os.Args[0], append(args[1:], input...)...) | ||
| out, err := cmd.CombinedOutput() | ||
| fmt.Print(string(out)) |
There was a problem hiding this comment.
This works quite differently however, we now have subprocesses instead of the execution happening in the same process.
Could we not get kong to re-parse the arguments?
There was a problem hiding this comment.
I missed this, it's not super pretty. I wonder if there's a compelling use case here to begin with... #!/bin/syncthing cli scripts?
There was a problem hiding this comment.
The original intent (I think), which is now broken, was that you do multiple actions with a single roundtrip.
Namely, it would fetch the config at startup, run series of your commands from a text file/stdin, applying all the config changes to the config object in memory, before posting the config back. This way you have one round trip, one set of locking and file renaming shenanigans for a large set of changes.
There was a problem hiding this comment.
I guess I'd rather kill this than keep it like this.
calmh
added a commit
to calmh/syncthing
that referenced
this pull request
Dec 11, 2023
calmh
added a commit
that referenced
this pull request
Dec 11, 2023
Seems to work for me, @AudriusButkevicius.
calmh
added a commit
to calmh/syncthing
that referenced
this pull request
Dec 16, 2023
* main: (89 commits) build: Update quic-go (fixes syncthing#9287) lib/model: Only handle relevant folder summaries (kqueue) (fixes syncthing#9183) (syncthing#9288) lib/model: Use a single lock (phase two: cleanup) (syncthing#9276) build(deps): bump actions/setup-go from 4 to 5 (syncthing#9279) lib/model: Use a single lock (syncthing#9275) cmd/syncthing: Better cli stdin handling (ref syncthing#9166) (syncthing#9281) cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong (syncthing#9166) lib/nat: Fix test build failure (ref syncthing#9010) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010) gui, man, authors: Update docs, translations, and contributors gui: Show folder/device status on small screens (syncthing#8643) lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271) build: Update dependencies (syncthing#9265) build: Revert specifics for Go 1.21.4, build using Go 1.21.5 (syncthing#9264) lib/fs: Reduce memory usage in xattrs handling (syncthing#9251) lib/model: Improve LastSeen handling (syncthing#9256) ...
calmh
added a commit
to calmh/syncthing
that referenced
this pull request
Jan 14, 2024
* main: lib/model: Use a single lock (phase two: cleanup) (syncthing#9276) build(deps): bump actions/setup-go from 4 to 5 (syncthing#9279) lib/model: Use a single lock (syncthing#9275) cmd/syncthing: Better cli stdin handling (ref syncthing#9166) (syncthing#9281) cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong (syncthing#9166) lib/nat: Fix test build failure (ref syncthing#9010) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010) gui, man, authors: Update docs, translations, and contributors gui: Show folder/device status on small screens (syncthing#8643) lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
syncthing clisubcommand was using urfave/cli as the command parser. This PR replace it with kong, which the main command uses.Some help texts and error message format are changed. Other than that, all the command usage and logic remains unchanged.
There's only one place which still uses urfave/cli, which is
syncthing cli config, because it uses some magic to dynamically build commands from struct reflects. I used kong'spassthrough:""tag to pass any argument followingsyncthing cli configto urfave/cli parser.This PR also fixes #9041
Testing
I manually tested some commands.