rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.7k stars 2.41k forks source link

display aliases in `cargo --list` #8486

Closed ehuss closed 3 years ago

ehuss commented 4 years ago

It can be confusing that there are commands (aliases) available that are not displayed with cargo --list. I think displaying these can help with discovery, and make it clearer that certain commands are actually aliases.

The tricky bit is exactly how it should be exposed. There are tools and scripts that parse the output of cargo --list, which unfortunately doesn't have an explicit flag for setting the output format (such as a machine-readable format). I think something like this should work:

b                   alias: build
c                   alias: check
t                   alias: test
r                   alias: run
rr                  alias: run --release
space_example       alias: run --release -- "command list"
CPerezz commented 4 years ago

Hi @ehuss I would like to work on this (if possible) but I might need some help.

As far as I've seen, the only place where the aliases are listed is in src/bin/cargo/main.rs in this function. I haven't been able to find any place in the code where this aliases are defined with a type or in a more "structured" way.

Therefore, I'm not sure if the aliases should pass to be handled more consistently (so if more of them get added, it's easier to make it happen without needing to touch strings and functions in different places) or instead, we should just edit the fn list_commands(config: &Config) -> BTreeSet<CommandInfo> and hardcore the aliases in there so they get printed with the rest of the output.

Another option is to create a list with the aliases and make the functions that interact with them point to that list. On this way, we could edit https://github.com/rust-lang/cargo/blob/f84f3f8c630c75a1ec01b818ff469d3496228c6b/src/bin/cargo/cli.rs#L92 and introduce the Avaliable aliases section which will collect the data from the aliases list.

Personally, I belive it would be nice to get something like:

$ cargo --list
Installed Commands:
    bench                Execute all benchmarks of a local package
    build                Compile a local package and all of its dependencies
    check                Check a local package and all of its dependencies for errors
    clean                Remove artifacts that cargo has generated in the past
    doc                  Build a package's documentation
    fetch                Fetch dependencies of a package from the network
    fix                  Automatically fix lint warnings reported by rustc
    generate-lockfile    Generate the lockfile for a package
    git-checkout         This subcommand has been removed
    init                 Create a new cargo package in an existing directory
    install              Install a Rust binary. Default location is $HOME/.cargo/bin
    locate-project       Print a JSON representation of a Cargo.toml file's location
    login                Save an api token from the registry locally. If token is not specified, it will be read from stdin.
    metadata             Output the resolved dependencies of a package, the concrete used versions including overrides, in machine-readable format
    new                  Create a new cargo package at <path>
    owner                Manage the owners of a crate on the registry
    package              Assemble the local package into a distributable tarball
    pkgid                Print a fully qualified package specification
    publish              Upload a package to the registry
    read-manifest        Print a JSON representation of a Cargo.toml manifest.
    run                  Run a binary or example of the local package
    rustc                Compile a package, and pass extra options to the compiler
    rustdoc              Build a package's documentation, using specified custom flags.
    search               Search packages in crates.io
    test                 Execute all unit and integration tests and build examples of a local package
    tree                 Display a tree visualization of a dependency graph
    uninstall            Remove a Rust binary
    update               Update dependencies as recorded in the local lock file
    vendor               Vendor all dependencies for a project locally
    verify-project       Check correctness of crate manifest
    version              Show version information
    yank                 Remove a pushed crate from the index
Avaliable aliases:
    b                   alias: build
    c                   alias: check
    t                   alias: test
    r                   alias: run
    rr                  alias: run --release
ehuss commented 4 years ago

Thanks @CPerezz!

I would probably try to extract the hard-coded table (with b, c, r, t) into a separate function (or constant), so that it can be shared with the list function.

As for the output, the different headings might be nice, but since this tends to get consumed by scripts, I don't think we can add them at this time. Perhaps some time in the future a flag could be added to specify the output style, and a specifically machine-readable format could be added, giving more flexibility.

CPerezz commented 4 years ago

Hey @ehuss I have it working. There's only one thing I'm not sure about.

Since we use a BTreeSet for storing the commands we will print when cargo --list gets triggered as you can see in: https://github.com/rust-lang/cargo/blob/aa6872140ab0fa10f641ab0b981d5330d419e927/src/bin/cargo/main.rs#L77 we get them on this way actually:

Installed Commands:
    b                    alias: build
    bench                Execute all benchmarks of a local package
    build                Compile a local package and all of its dependencies
    c                    alias: check
    check                Check a local package and all of its dependencies for errors
    clean                Remove artifacts that cargo has generated in the past
    doc                  Build a package's documentation
    fetch                Fetch dependencies of a package from the network
    fix                  Automatically fix lint warnings reported by rustc
    generate-lockfile    Generate the lockfile for a package
    git-checkout         This subcommand has been removed
    init                 Create a new cargo package in an existing directory
    install              Install a Rust binary. Default location is $HOME/.cargo/bin
    locate-project       Print a JSON representation of a Cargo.toml file's location
    login                Save an api token from the registry locally. If token is not specified, it will be read from stdin.
    metadata             Output the resolved dependencies of a package, the concrete used versions including overrides, in machine-readable format
    new                  Create a new cargo package at <path>
    owner                Manage the owners of a crate on the registry
    package              Assemble the local package into a distributable tarball
    pkgid                Print a fully qualified package specification
    publish              Upload a package to the registry
    r                    alias: run
    read-manifest        Print a JSON representation of a Cargo.toml manifest.
    run                  Run a binary or example of the local package
    rustc                Compile a package, and pass extra options to the compiler
    rustdoc              Build a package's documentation, using specified custom flags.
    search               Search packages in crates.io
    t                    alias: test
    test                 Execute all unit and integration tests and build examples of a local package
    tree                 Display a tree visualization of a dependency graph
    uninstall            Remove a Rust binary
    update               Update dependencies as recorded in the local lock file
    vendor               Vendor all dependencies for a project locally
    verify-project       Check correctness of crate manifest
    version              Show version information
    yank                 Remove a pushed crate from the index

So the aliases and the commands are both mixed since the BTreeSet enforces the Ord.

I can think about a couple of things now:

  1. Leave it as it is (not my favourite, but maybe you estimate that it's fine on this way).
  2. Instead of injecting the aliases as :
    for cmd in commands::builtin() {
        commands.insert(CommandInfo::BuiltIn {
            name: cmd.get_name().to_string(),
            about: cmd.p.meta.about.map(|s| s.to_string()),
        });
    }
    for command in &BUILTIN_ALIASES {
        commands.insert(CommandInfo::BuiltIn {
            name: command.0.to_string(),
            about: Some(command.1.to_string()),
        });
    }

    in: https://github.com/rust-lang/cargo/blob/aa6872140ab0fa10f641ab0b981d5330d419e927/src/bin/cargo/main.rs#L74, add them later once all the prints have finnished in: https://github.com/rust-lang/cargo/blob/aa6872140ab0fa10f641ab0b981d5330d419e927/src/bin/cargo/cli.rs#L91

Any suggestions? Since looks like all of this aliases stuff should get a refactor and propper struct supporting so it's less tricky to work with it and expand it on the future. But I'm new to Cargo so I'm not sure at all on which is the direction you guys want to take.

ehuss commented 4 years ago

Hm, I don't have a strong preference either way. Whichever way appeals to you. I'm fine with it being alphabetized, and I'm also fine if you want to keep them separate. (Sorry for the non-answer! 😄 )

CPerezz commented 4 years ago

The code will be easier to read and all in one file if we stay with this solution.

But at some point in the future I believe a refactor will be needed in order to make easier and more readable the interaction with builtin aliases.

Thanks!! Pushing the PR then!

ehuss commented 4 years ago

Reopening since the intent was to show all aliases. #8542 implemented for built-in aliases. @CPerezz indicated they may be able to follow up to also include user aliases.

CPerezz commented 4 years ago

The aliases that now are shown appart of the builtin ones are the cargo extensions that the user has installed.

So for example, this is what I get when executing cargo --list

bench                Execute all benchmarks of a local package
    build                Compile a local package and all of its dependencies
    check                Check a local package and all of its dependencies for errors
    clean                Remove artifacts that cargo has generated in the past
    doc                  Build a package's documentation
    fetch                Fetch dependencies of a package from the network
    fix                  Automatically fix lint warnings reported by rustc
    generate-lockfile    Generate the lockfile for a package
    git-checkout         This subcommand has been removed
    init                 Create a new cargo package in an existing directory
    install              Install a Rust binary. Default location is $HOME/.cargo/bin
    locate-project       Print a JSON representation of a Cargo.toml file's location
    login                Save an api token from the registry locally. If token is not specified, it will be read from stdin.
    metadata             Output the resolved dependencies of a package, the concrete used versions including overrides, in machine-readable format
    new                  Create a new cargo package at <path>
    owner                Manage the owners of a crate on the registry
    package              Assemble the local package into a distributable tarball
    pkgid                Print a fully qualified package specification
    publish              Upload a package to the registry
    read-manifest        Print a JSON representation of a Cargo.toml manifest.
    run                  Run a binary or example of the local package
    rustc                Compile a package, and pass extra options to the compiler
    rustdoc              Build a package's documentation, using specified custom flags.
    search               Search packages in crates.io
    test                 Execute all unit and integration tests and build examples of a local package
    tree                 Display a tree visualization of a dependency graph
    uninstall            Remove a Rust binary
    update               Update dependencies as recorded in the local lock file
    vendor               Vendor all dependencies for a project locally
    verify-project       Check correctness of crate manifest
    version              Show version information
    yank                 Remove a pushed crate from the index
    clippy
    clippy
    clippy
    clippy
    flamegraph
    fmt
    fmt
    fmt
    fmt
    miri
    miri
    miri
    miri
    outdated
    tree

What would you like to add here @ehuss ? If I understood correctly, you would also like to list the aliases added in .cargo/config right? As seen in https://doc.rust-lang.org/cargo/reference/config.html#alias

If that's the case, I'll work on it. The only thing is that since the output can be larger now, I would probably like to make a slightly bigger refactor so that we can print the aliases in different sections or rather, just have an Arg for the aliases.

WDYT?

ehuss commented 4 years ago

If I understood correctly, you would also like to list the aliases added in .cargo/config right? As seen in https://doc.rust-lang.org/cargo/reference/config.html#alias

Yes, that is correct. If someone has this in their config:

[alias]
space_example = ["run", "--release", "--", "\"command list\""]

Then cargo list should include something like this somewhere in the output:

space_example       alias: run --release -- "command list"

I would not expect most users to have a lot of aliases, so I wouldn't worry too much about that. Fixing #6088 would probably go a much longer way towards improving the output.

I'm uncertain about changing the structure of the output. There are tools that parse the output of the command, and changing the structure would break them. It would definitely be good to add a parseable format, but I think that will require a long transition time before we could change the default output. You're welcome to work on that, but I think that is a separate concern.

CPerezz commented 3 years ago

Hi @ehuss.

Sorry for the long delay. I've been extremely busy and also sick for some time. Quick question. Why do you use the format space_example = ["run", "--release", "--", "\"command list\""] instead of directly using run --release -- "command list"?

It's easier to parse and this type of separation doesn't seem to provide any advantages to me. (At least that I'm aware obviously).

ehuss commented 3 years ago

No worries, please take care of yourself first.

Cargo doesn't have any sort of shell parsing code, so it is unable to understand "quoted strings" or escaping or anything like that. To support arguments with spaces, the alias must use TOML's array syntax so that each argument is a separate string.

CPerezz commented 3 years ago

Hi @ehuss I've been putting a bit of time into this.

I have a couple of questions. if you have some time. I saw that you agreed on just checking $CARGO_HOME/config(.toml). The problem is that as the docs say, there's a hierarchical structure that opens two ways(at least that I see).

  1. Parse just $CARGO_HOME/config(.toml) and get the [aliases] from there. Something like:

    // Add the Manifest-defined aliases.
    match config.get_file_path(config.home().as_path_unlocked(), "config", true) {
        Ok(Some(config_path)) => {
            let toml_contents = crate::util::toml::parse(
                &util::paths::read(&config_path).expect("Error on read"),
                &config_path,
                &config,
            )
            .unwrap();
            // Get aliases and add them to the BTreeSet
        }
        _ => (),
    }
  2. If we need to reach all of the possible configfiles, the most reasonable way I've seen to do that is to use https://github.com/rust-lang/cargo/blob/8a3361c126d0af96b5c30b3b3db336d04a0d810b/src/cargo/util/config/mod.rs#L462 doing something like config.get_cv(&crate::util::config::ConfigKey::from_str("alias"))

Having the following in my .cargo/config.toml

[alias]     # command aliases                                                                                                                                                                                                            
dnd = ["doc", "--no-deps"]

This step returns the following (making it panic to get the print) with :

Installed Commands:
thread 'main' panicked at 'Ok(Some({"dnd": [doc (from /home/kr0/.cargo/config.toml), --no-deps (from /home/kr0/.cargo/config.toml)] (from /home/kr0/.cargo/config.toml)}))', src/bin/cargo/main.rs:131:5

It's really weird since using TOML format I get this, which contains these (from /home/kr0/.cargo/config.toml) in the middle of the tokens of the expression.

On the other side, If instead I have:

[alias]     # command aliases                                                                                                                                                                                                            
dnd = "doc --no-deps"

I get

Installed Commands:
thread 'main' panicked at 'Ok(Some({"dnd": doc --no-deps (from /home/kr0/.cargo/config.toml)}))', src/bin/cargo/main.rs:131:5

which seems a way easier to parse.

I just would appreciate advice on which you consider the best approach or indeed if there's any other one that I'm not considering and would work better.

It would also be helpful to know if you're only interested in .cargo/config.toml or all of the hierarchical configfiles.

Thanks a lot.

ehuss commented 3 years ago

Sorry, I'm not sure I follow the questions about loading config files. The config is already loaded.

I would start by changing list_aliases to return a BTreeMap instead a vec of keys. I would also fix a bug in the implementation, it assumes the value is a string. It should be config.get::<BTreeMap<String, StringOrVec>>("alias"). Then I would change list_commands to call list_aliases. I would probably add a new variant to CommandInfo for aliases. The call to list_aliases could then be removed from execute_external_subcommand.

CPerezz commented 3 years ago

Should I take the opportunity to migrate the list command code from main.rs and cli.rs to be inside of commands and implement the corresponding exec so that it's in-line with the rest of the cargo commands?

ehuss commented 3 years ago

I would probably avoid major changes like that for now. --list is not a real "clap" subcommand, and it may be confusing to add commands that aren't clap subcommands, and it would introduce some inconsistencies on how those modules are used.

CPerezz commented 3 years ago

Perfect!!

djmitche commented 3 years ago

@rustbot claim