feat!: consolidate cot cli commands into one - #587
Conversation
|
| Project | cot |
| Branch | elijah/cot-proxy-cmd |
| Testbed | github-ubuntu-latest |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result microseconds (µs) (Result Δ%) | Upper Boundary microseconds (µs) (Limit %) |
|---|---|---|---|
| empty_router/empty_router | 📈 view plot 🚷 view threshold | 13,652.00 µs(+59.40%)Baseline: 8,564.87 µs | 16,300.96 µs (83.75%) |
| json_api/json_api | 📈 view plot 🚷 view threshold | 1,054.90 µs(+0.33%)Baseline: 1,051.42 µs | 1,373.79 µs (76.79%) |
| nested_routers/nested_routers | 📈 view plot 🚷 view threshold | 989.43 µs(+0.62%)Baseline: 983.37 µs | 1,257.69 µs (78.67%) |
| single_root_route/single_root_route | 📈 view plot 🚷 view threshold | 954.81 µs(+0.85%)Baseline: 946.79 µs | 1,219.97 µs (78.27%) |
| single_root_route_burst/single_root_route_burst | 📈 view plot 🚷 view threshold | 17,185.00 µs(+0.53%)Baseline: 17,094.45 µs | 21,564.70 µs (79.69%) |
- help for workspaces and packages now dispatch to the custom help handler
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| #[cfg(unix)] | ||
| { | ||
| let err = std::process::Command::new(&proj.path).args(args).exec(); | ||
| anyhow::bail!("Failed to exec {}: {err}", proj.path.display()); | ||
| } | ||
|
|
||
| #[cfg(not(unix))] | ||
| { | ||
| let status = std::process::Command::new(&proj.path).args(args).status()?; | ||
| std::process::exit(status.code().unwrap_or(1)); | ||
| } |
There was a problem hiding this comment.
Where's the difference between Unix-like and non-Unix-like platforms coming from? Why do we need separate code paths here?
There was a problem hiding this comment.
The idea here is to replace the cot process with that of the child(the binary) whenever we forward to(or run) the binary. The exec call is only available on POSIX. Windows doesn't provide a way to do this, so we just spawn the child and block until it exits. Left a comment to outline this behavior as well.
On a side note, one issue i think isnt being handled is signal forwarding in the non-Unix case
There was a problem hiding this comment.
Do we need all this machinery here? Any chance that running cargo run would suffice instead?
…h/cot-proxy-cmd # Conflicts: # cot-cli/tests/snapshot_testing/external/check.rs
m4tx
left a comment
There was a problem hiding this comment.
Oof, this is a big change! One big major possible improvement I still see is whether we could offload some of the logic (especially in the cot-cli crate) to some existing crates, or cargo itself. (see the comments) Let me know what you think!
| /// argv, before clap has parsed anything. Needed because `project::load` | ||
| /// must run before `Cli::parse` for the `--help` interception path. | ||
| #[must_use] | ||
| pub fn extract_package_arg(raw: &[String]) -> Option<String> { |
There was a problem hiding this comment.
Should we also handle cases like cot check -- -p package here? (and if so, we should have tests for these as well)
There was a problem hiding this comment.
This function is only called in main() after we split on -- to differentiate internal args from args forwarded to the binary. In practice, it wouldn't receive raw args, but I've added a defensive check to the function to handle that case.
Also, shouldn't this be cot check -p package instead of cot check -- -p package? I'd imagine that external args (args after the -- delimiter) are forwarded to the binary, so we should expect this to throw an error.
That also brings me to this scenario: commands like migration rollback, collect-static, and check live in the binary and are treated as external commands from the perspective of cot-cli, just like custom commands. However, we market these commands as first-class cot-cli commands to end users. So users would expect all cot commands to follow the same convention, with args coming before the delimiter, rather than having to figure out which internal commands require args after the delimiter and which require args before.
I think as a follow-up to this PR, we should have some mechanism to make cot-cli aware of what commands to treat as first-class citizens, and also maybe a way to make CliTask aware of flags already registered by cot-cli (like --package and --release).
What do you think?
| .context("Cargo.toml has no [package] section and no [[bin]] targets") | ||
| } | ||
|
|
||
| fn resolve_target_dir(start_dir: &Path) -> PathBuf { |
There was a problem hiding this comment.
What if the target dir is set in ~/.cargo/config.toml? Will it also work?
There was a problem hiding this comment.
This should be handled by cargo-metadata since it delegates that discovery to the cargo toolchain
| @@ -0,0 +1,946 @@ | |||
| use std::fmt::Write; | |||
There was a problem hiding this comment.
Another issue I have with this file is that it duplicates a lot of logic with cargo itself. This seems very brittle, and sounds like we could be missing some edge cases our users could run into (e.g. see my comment about resolving the target directory).
I'm wondering if all of that is really needed? I'm thinking whether we could:
- Use cargo itself for some of that, e.g. by using
cargo build --mesage-format json - Use a specialised crate for this, e.g. cargo_toml
There was a problem hiding this comment.
Yeah, the main motivation for moving away from cargo run on every run is that on a hot path where no recompile of the binary is needed, cargo still walks up the dependency graph to find fresh/dirty deps that need to be recompiled, which isn't free (time-wise). This isn't a problem for smaller projects, but I'm concerned it can give a bad user experience for larger projects. I figured using cargo build as the last resort (the cold path where the binary isn't compiled at all) would a much better experience.
I also agree on the brittle approach of manually obtaining the target directory/binary. I looked into the cargo_metadata crate, which seems to obtain the outputs of cargo metadata and cargo --message-format=json and correctly resolves the target dir and handles the case where CARGO_TARGET_DIR is set (which cargo_toml crate falls short of). It's also less expensive compared to cargo build when you run with the --no-deps flag. One thing about cargo_metadata to note is that it gets you the information about the target dir and available binaries, which is one part of the job done. However, it leaves it up to you to identify which binary or handle any disambiguations if there are multiple binaries.
Overall, the flow I had in mind looks like this:
- Try to find the path to the binary by getting this info from cargo_metadata
a. If the binary exists, try to obtain metadata information from the cache(hot path) if it exists and binary hasnt been recompiled or from the binary itself(warm path) using the--cot-internal-cli-metadataflag
b. If the binary does not exist(cold path), build the binary at request of the user usingcargo buildvia the build flag(I'm wondering if this should be implicit and automatic). Then go ahead to obtain metadata information like in stepa - After obtaining the metadata, invoke the binary and forward any args to it.
- The help command follows the same approach; it tries to check if the binary exists and tries to obtain metadata. If it doesn't exist or metadata extraction fails, it warns the user and falls back to the generic help command by clap, which shows only commands defined in the cot-cli
What do you think of this?
Background
cotcurrently exposes CLI commands through two separate entry points:cot-clicrate (cot <command>): handles project scaffolding, migration listing, migration generation, and shell completions.check, running migrations (which is also triggered implicitly at startup), and any custom user-defined task commands.In addition, running and building a
cotapp relies on Cargo tooling. In summary, there are three ways to invoke CLI commands today:cot <command>via thecot-clicratecot-cliThis PR focuses on unifying 1 and 2 for ergonomics:
cotnow acts as the single entry point for all commands, proxying any unrecognized command to the compiled binary if it exists there. Option 3 (Cargo invocation) is out of scope for this PR and can be revisited in a follow-up if proxying those commands makes sense.Approach
When
cotreceives a command it does not recognize, it resolves the target binary (target/debugby default, ortarget/releaseif--releaseis passed), queries it for its available commands via a metadata flag, and either forwards the command along with all provided arguments or returns an error if the command is not found in the binary either.Metadata
To support proxying, the
cotcrate exposes a--metadataflag. At runtime, the binary uses reflection to enumerate all registered CLI commands and prints them as JSON to stdout. This serves two purposes: it tellscot-cliwhether a given command should be forwarded, and it provides the information needed to render accurate help text.Example:
Caching
Querying the binary on every invocation would be wasteful, so the metadata response is cached in
.cot/command-cache.json. The cache stores the binary's modified time (mtime) alongside the metadata and is invalidated automatically whenever the binary is rebuilt.Workspaces
When working inside a Cargo workspace, a
--package(-p) flag must be provided to specify which package's binary should be targeted.Type of change