Allow cargo aliases to shadow existing commands with flags #9768

nipunn1313 · 2021-08-06T19:15:44Z

Teaches cargo to accept default-args aliases of the form
[alias]
fmt = fmt --verbose

Teaches cargo to failfast (rather than stack overflow on corecursive
aliases)
[alias]
test-1 = test-2
test-2 = test-3
test-3 = test-1

Fixes #8360

rust-highfive · 2021-08-06T19:15:47Z

r? @Eh2406

(rust-highfive has picked a reviewer for you, use r? to override)

nipunn1313 · 2021-08-06T19:17:05Z

r? @alexcrichton

since it's related to #9767

nipunn1313 · 2021-08-06T19:22:34Z

tests/testsuite/cargo_alias_config.rs

@@ -73,6 +76,56 @@ fn recursive_alias() {
        .run();
 }

+#[cfg(unix)]


cfg(unix) because I use a small bash program in the test.
Open to other ideas. Just needed a small program which echoed back out args.

nipunn1313 · 2021-08-06T19:25:32Z

src/bin/cargo/cli.rs

+                }
+
+                already_expanded.push(cmd.to_string());
+                if already_expanded.contains(&new_cmd.to_string()) {


happy to switch to a LinkedHashMap if we want cargo to support O(n*log(n)) resolution of dependent aliases instead of O(n^2). I figured n is probably small, and linked_hash_map crate is a new dependency - but happy to switch it if preferred.

Why LinkedHashMap over indexmap? But yes n is probably to small for it to matter.

wasn't familiar with indexmap - looks great to me.
It still appears that it would be an additional dependency to cargo (not sure if you want to pull it in)

I think this is fine for now, yeah, and if it's an issue there's a number of other data structures to use here instead.

alexcrichton

Seems reasonable to me, thanks! Would you be ok updating the documentation for the alias section as well?

alexcrichton · 2021-08-09T16:10:58Z

tests/testsuite/cargo_alias_config.rs

+        .executable(
+            Path::new("subcommands").join("cargo-test-1"),
+            r#"
+                #!/bin/bash


Instead of a shell script, would you be up for updating this test to build a small cargo project that produces a binary that does the same thing? We do that in a few other places in the test suite as well, primarily for Windows testing support.

Found a place where this happens and refactored it into cargo-test-support/src/tools.rs

alexcrichton · 2021-08-09T16:11:24Z

src/bin/cargo/cli.rs

+                }
+
+                already_expanded.push(cmd.to_string());
+                if already_expanded.contains(&new_cmd.to_string()) {


I think this is fine for now, yeah, and if it's an issue there's a number of other data structures to use here instead.

nipunn1313 · 2021-08-09T17:17:22Z

Added documentation
Migrated the test to support windows using @alexcrichton suggestion (thanks!)
Factored into 3 separate commits to ease reviewing
Added test to confirm builtins can't use this feature

alexcrichton · 2021-08-09T18:41:06Z

Thanks! This affects the public API of cargo so I'm gonna ask for sign-off from others as well:

@rfcbot fcp merge

rfcbot · 2021-08-09T18:41:07Z

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

breaking-automated-usage (Allow cargo aliases to shadow existing commands with flags #9768 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

ehuss · 2021-08-10T15:01:28Z

I'm slightly concerned that it may be confusing to people what "built-in" means, since we don't define that anywhere. To many users, they perceive cargo fmt and cargo clippy to be built-in because they ship with rustup. I think it's fine for now, we can always tweak the docs or something if there are reports of confusion.

I guess there's also some concerns about why built-in commands are blessed in such a way that they can't be overridden. Why are they that much different from third-party commands? There are legitimate reasons why this is not allowed for built-in commands, so why would third-party commands be different?

This could introduce some future compatibility hazards. For example, if someone has an alias add = "add --sort", and then we make add a built-in command, that would break their setup.

I guess, I'd like to see an explanation of why these are different.

nipunn1313 · 2021-08-10T19:47:48Z

Re: Concern - builtins are blessed

Good point.

I believe disallowing aliases on builtin commands was an idea stolen from git. Here's an SO Post

It doesn't elaborate much on why. There's a benefit to disallowing aliasing of builtins away entirely - users confusing themselves out of fundamental commands. Aliasing with default args seems more ok. Perhaps it needn't be blessed for the default-args use case.

[alias]
build = my_custom_command  # might confuse users
build = "build --verbose". # maybe this is more ok?

Re: Concern - compatibility hazard

I think this compatibility hazard is preexisting. There's always the risk of adding a new builtin command - which can mess with people's aliases and custom subcommands. I think it's just the cost of adding a new builtin (it won't be backward compatible).

I know that appealing to the preexisting nature of the issue is not the best argument/defense - so consider it an opportunity to reconsider, but probably we'd have to think about how to handle subcommand / builtin conflicts as well.

joshtriplett · 2021-08-10T22:13:35Z

Another issue is that tools invoke Cargo commands as part of build processes, and such aliases could disrupt automated builds. As far as I can tell, we don't even have an option to ignore a user's ~/.cargo/config.toml, other than overriding all of CARGO_HOME.

This is of course also a problem for other commands such as fmt, but that seems less likely to cause build breakage than overriding cargo build or cargo test or cargo run with local configuration.

I'd also hate to see a crate trying to override cargo build with its own non-standard build command.

joshtriplett · 2021-08-10T22:14:12Z

@rfcbot concern breaking-automated-usage

nipunn1313 · 2021-08-15T01:11:52Z

I pulled out the non-controversial majority of this diff into #9791 - simply ensuring that we give a reasonable error message instead of stack-overflow when there are recursively defined aliases. Assuming that one can get in with less discussion, then we can have the discussion here on a smaller diff.

Another issue is that tools invoke Cargo commands as part of build processes, and such aliases could disrupt automated builds. As far as I can tell, we don't even have an option to ignore a user's ~/.cargo/config.toml, other than overriding all of CARGO_HOME.

This is of course also a problem for other commands such as fmt, but that seems less likely to cause build breakage than overriding cargo build or cargo test or cargo run with local configuration.

I'd also hate to see a crate trying to override cargo build with its own non-standard build command.

Great point - another reason to lean toward disallowing default-flags on builtin commands (or potentially disallowing default-flags entirely).

Eg. [alias] test-1 = test-2 test-2 = test-3 test-3 = test-1 Previously it would stack overflow It pulls out non controversial bits from from rust-lang#9768

Teach cargo to failfast on recursive/corecursive aliases Eg. [alias] test-1 = test-2 test-2 = test-3 test-3 = test-1 Previously it would stack overflow Pulled out non controversial bits from from #9768

bors · 2021-08-16T15:33:39Z

☔ The latest upstream changes (presumably #9791) made this pull request unmergeable. Please resolve the merge conflicts.

Teaches cargo to accept default-args aliases of the form [alias] fmt = fmt --verbose Disallows this on builtins. Fixes rust-lang#8360

joshtriplett · 2021-08-17T19:47:41Z

We discussed this in today's @rust-lang/cargo meeting. We came to the conclusion that we really want to have consistency between internal and external cargo commands, so we don't want to allow aliases to override either one.

(We'd be happy to have improvements to the error messages for overriding external commands, though.)

nipunn1313 · 2021-08-17T20:09:42Z

Thanks @joshtriplett, @alexcrichton and all. Appreciated.

Consider giving the explanation on the tracking issue #8360 as well (and closing it)!

rust-highfive assigned Eh2406 Aug 6, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 6, 2021

rust-highfive assigned alexcrichton and unassigned Eh2406 Aug 6, 2021

nipunn1313 force-pushed the cargo_alias_self branch from d4f3097 to e3ca6c4 Compare August 6, 2021 19:17

nipunn1313 commented Aug 6, 2021

View reviewed changes

nipunn1313 force-pushed the cargo_alias_self branch 2 times, most recently from 4d45bb6 to 5f8b329 Compare August 6, 2021 19:34

alexcrichton reviewed Aug 9, 2021

View reviewed changes

nipunn1313 force-pushed the cargo_alias_self branch 4 times, most recently from 4bfaee8 to 9687a9b Compare August 9, 2021 17:16

alexcrichton added the T-cargo Team: Cargo label Aug 9, 2021

rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Aug 9, 2021

nipunn1313 mentioned this pull request Aug 15, 2021

Teach cargo to failfast on recursive/corecursive aliases #9791

Merged

nipunn1313 force-pushed the cargo_alias_self branch from 9687a9b to 6e5574a Compare August 15, 2021 01:19

nipunn1313 added 2 commits August 16, 2021 09:52

Allow cargo aliases to shadow existing commands with flags

043c4f1

Teaches cargo to accept default-args aliases of the form [alias] fmt = fmt --verbose Disallows this on builtins. Fixes rust-lang#8360

Add documentation for alias with default args configuration

2173224

nipunn1313 force-pushed the cargo_alias_self branch from 6e5574a to 2173224 Compare August 16, 2021 16:53

joshtriplett closed this Aug 17, 2021

rfcbot removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Aug 17, 2021

nipunn1313 deleted the cargo_alias_self branch September 3, 2021 02:31

joshtriplett mentioned this pull request Nov 9, 2021

Aliases should not be able to shadow external subcommands #10049

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow cargo aliases to shadow existing commands with flags #9768

Allow cargo aliases to shadow existing commands with flags #9768

nipunn1313 commented Aug 6, 2021

rust-highfive commented Aug 6, 2021

nipunn1313 commented Aug 6, 2021

nipunn1313 Aug 6, 2021

nipunn1313 Aug 6, 2021

Eh2406 Aug 6, 2021

nipunn1313 Aug 6, 2021

alexcrichton Aug 9, 2021

alexcrichton left a comment

alexcrichton Aug 9, 2021

nipunn1313 Aug 9, 2021

alexcrichton Aug 9, 2021

nipunn1313 commented Aug 9, 2021

alexcrichton commented Aug 9, 2021

rfcbot commented Aug 9, 2021 •

edited

Loading

ehuss commented Aug 10, 2021

nipunn1313 commented Aug 10, 2021

joshtriplett commented Aug 10, 2021

joshtriplett commented Aug 10, 2021

nipunn1313 commented Aug 15, 2021

bors commented Aug 16, 2021

joshtriplett commented Aug 17, 2021

nipunn1313 commented Aug 17, 2021

Allow cargo aliases to shadow existing commands with flags #9768

Allow cargo aliases to shadow existing commands with flags #9768

Conversation

nipunn1313 commented Aug 6, 2021

rust-highfive commented Aug 6, 2021

nipunn1313 commented Aug 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nipunn1313 commented Aug 9, 2021

alexcrichton commented Aug 9, 2021

rfcbot commented Aug 9, 2021 • edited Loading

ehuss commented Aug 10, 2021

nipunn1313 commented Aug 10, 2021

joshtriplett commented Aug 10, 2021

joshtriplett commented Aug 10, 2021

nipunn1313 commented Aug 15, 2021

bors commented Aug 16, 2021

joshtriplett commented Aug 17, 2021

nipunn1313 commented Aug 17, 2021

rfcbot commented Aug 9, 2021 •

edited

Loading