Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --list to known subcommands. #719

Merged
merged 1 commit into from
May 28, 2022
Merged

Add --list to known subcommands. #719

merged 1 commit into from
May 28, 2022

Conversation

Alexhuszagh
Copy link
Contributor

cross --list should list the subcommands present for cargo in the image, rather on the host. This works because any option provided before a subcommand has priority over the subcommand. For example, cargo build --help prints the help menu for cargo build, but cargo --help build ignores build and prints the help for cargo. Therefore, the options --help, --version, and --list can be treated as pseudo-subcommands.

Fixes #715.

@Emilgardis
Copy link
Member

Hmm, im not sure if this is the correct behaviour, since the list may not actually be what we support

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented May 25, 2022

Hmm, im not sure if this is the correct behaviour, since the list may not actually be what we support

My mistake, since commands like clippy are supported but might not be installed, but clean isn't but is provided? Should we just print out all the subcommands in Subcommand, maybe implementing Subcommand::from using a hashmap and then printing out support subcommands by printing out the keys sorted?

@Emilgardis
Copy link
Member

I don't know what should be expected here. Maybe a compromise of first showing actually supported commands (e.g commands run in container), and then a list of those that are run on the host

@Alexhuszagh Alexhuszagh marked this pull request as draft May 25, 2022 14:15
@Alexhuszagh
Copy link
Contributor Author

I feel that might be the best solution, since then we get the supported subcommands for both cross and the host cargo, and it correctly handles edge-cases like cargo --list --version (which prints the version) and cargo --list build (which lists the subcommands).

The current output is:

Installed Commands:                                                                                                                                                                                 
    afl                                                                                                                                                                                             
    asm
    ...

So maybe the output should be something like:

Cross Commands:
    b                    alias: build                                                                                                                                                               
    bench                Execute all benchmarks of a local package                                                                                                                                  
    build                Compile a local package and all of its dependencies 
    ...

Host Commands:                                                                                                                                                                                 
    afl                                                                                                                                                                                             
    asm
    ...

@Alexhuszagh Alexhuszagh marked this pull request as ready for review May 27, 2022 20:22
@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented May 27, 2022

Ok I've implemented the requested changes, and a sample output is as follows:

$ target/debug/cross --list                                                                                                                                                
Warning: Falling back to `cargo` on the host.                                                                                                                                                       
Cross 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                                                                                                               
    clippy               Checks a package to catch common mistakes and improve your Rust code.                                                                                                      
    doc                  Build a package's documentation                                                                                                                                            
    metadata             Output the resolved dependencies of a package, the concrete used versions including overrides, in machine-readable format                                                  
    r                    alias: run                                                                                                                                                                 
    run                  Run a binary or example of the local package                                                                                                                               
    rustc                Compile a package, and pass extra options to the compiler                                                                                                                  
    t                    alias: test                                                                                                                                                                
    test                 Execute all unit and integration tests and build examples of a local package
Host Commands:
    afl
    asm
    clean                Remove artifacts that cargo has generated in the past
    config               Inspect configuration values
    count
    d                    alias: doc
    deb
    expand
    fetch                Fetch dependencies of a package from the network
    fix                  Automatically fix lint warnings reported by rustc
    fmt                  Formats all bin and lib files of the current crate using rustfmt.
    fuzz
    generate-lockfile    Generate the lockfile for a package
    git-checkout         This subcommand has been removed
    graph
    help                 Displays help for a cargo subcommand
    init                 Create a new cargo package in an existing directory
    install              Install a Rust binary. Default location is $HOME/.cargo/bin
    llvm-ir
    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.
    logout               Remove an API token from the registry locally
    miri
    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.
    report               Generate and display various kinds of reports
    rustdoc              Build a package's documentation, using specified custom flags.
    search               Search packages in crates.io
    tarpaulin
    tree                 Display a tree visualization of a dependency graph
    uninstall            Remove a Rust binary
    update               Update dependencies as recorded in the local lock file
    valgrind
    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

The logic is explained in the commit message, but basically, it captures the list of subcommands from cargo --list, uses Subcommand::from on each one, and group the subcommands by if it's a known or unknown subcommand. After grouping, we print to stdout. This also means if we ever add support for custom subcommands, like in Cross.toml, we should also be able print those subcommands.

src/extensions.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/extensions.rs Outdated Show resolved Hide resolved
@Alexhuszagh Alexhuszagh force-pushed the list branch 2 times, most recently from 9d3277c to f21a604 Compare May 27, 2022 21:58
@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented May 27, 2022

Ok I've implemented the above changes and I've added the OutputExt trait for this purpose, which implements stdout:

pub trait OutputExt {
    fn stdout(&self) -> Result<String>;
}

impl OutputExt for std::process::Output {
    fn stdout(&self) -> Result<String> {
        String::from_utf8(self.stdout.clone())
            .wrap_err_with(|| format!("`{:?}` output was not UTF-8", self))
    }
}

Next, I implemented run_and_get_output and run_and_get_stdout in terms of run_and_get_output:

    /// Runs the command to completion and returns its stdout
    fn run_and_get_stdout(&mut self, verbose: bool) -> Result<String> {
        let out = self.run_and_get_output(verbose)?;
        self.status_result(out.status)?;
        out.stdout()
    }

    /// Runs the command to completion and returns the status and its stdout.
    fn run_and_get_output(&mut self, verbose: bool) -> Result<std::process::Output> {
        self.print_verbose(verbose);
        self.output()
            .wrap_err_with(|| format!("couldn't execute `{:?}`", self))
            .map_err(Into::into)
    }

The rest is just trivial changes. However, this is mostly your code, so you should probably get credit for it? One major difference I've added is run_and_get_output doesn't fail if a non-zero status is returned, since I think that should be handled elsewhere.

@Emilgardis
Copy link
Member

However, this is mostly your code, so you should probably get credit for it?

Doesn't matter to me, if you want you could add

Co-authored-by: Emil Gardström <[email protected]>

to the commit message

src/cargo.rs Show resolved Hide resolved
Comment on lines 51 to 56
/// Runs the command to completion and returns the status and its stdout.
fn run_and_get_output(&mut self, verbose: bool) -> Result<std::process::Output> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Runs the command to completion and returns the status and its stdout.
fn run_and_get_output(&mut self, verbose: bool) -> Result<std::process::Output> {
/// Runs the command to completion and returns the status and its [output](std::process::Output).
///
/// # Notes
///
/// This command does not check the status.
fn run_and_get_output(&mut self, verbose: bool) -> Result<std::process::Output> {

@Alexhuszagh
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request May 28, 2022
719: Add --list to known subcommands. r=Alexhuszagh a=Alexhuszagh

`cross --list` should list the subcommands present for cargo in the image, rather on the host. This works because any option provided before a subcommand has priority over the subcommand. For example, `cargo build --help` prints the help menu for `cargo build`, but `cargo --help build` ignores `build` and prints the help for `cargo`. Therefore, the options `--help`, `--version`, and `--list` can be treated as pseudo-subcommands.

Fixes #715.

Co-authored-by: Alex Huszagh <[email protected]>
@bors
Copy link
Contributor

bors bot commented May 28, 2022

Build failed:

@Alexhuszagh
Copy link
Contributor Author

Hmm both x86_64-apple-darwin and x86_64-pc-windows-msvc are failing at the cross check --target $TARGET, where it seems that we're falling back to the host container for a recognized subcommand, causing unreachable code to be reached.

I believe the None | Some(Subcommand::Other) match arm should just _, since we can fall back to the host cargo if the image doesn't exist or if the target isn't supported (the two failing examples above).

src/main.rs Outdated
}
Ok(out.status)
}
None | Some(Subcommand::Other) => cargo::run(&argv, verbose),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be the universal match arm, since we can fall back to the host cargo if the image doesn't exist or if the target isn't supported (the two failing examples above).

_ => cargo::run(&argv, verbose),

`cross --list` should list the subcommands present for cargo in the
image, rather on the host. This works because any option provided before
a subcommand has priority over the subcommand. For example, `cargo build
--help` prints the help menu for `cargo build`, but `cargo --help build`
ignores `build` and prints the help for `cargo`. Therefore, the options
`--help`, `--version`, and `--list` can be treated as pseudo-subcommands.

This works by capturing the output subcommand list, and then classifying them
as either subcommands from the host or container. This also future proofs
our logic, if we ever get support for custom subcommands, since only `Other`
(unrecognized) subcommands are treated as host commands.

Sample Output:
```
Cross Commands:
    b                    alias: build
    bench                Execute all benchmarks of a local package
Host Commands:
    afl
    asm
    clean                Remove artifacts that cargo has generated in the past
```

Co-authored-by: Emil Gardström <[email protected]>

Fixes cross-rs#715.
@Alexhuszagh
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request May 28, 2022
@bors
Copy link
Contributor

bors bot commented May 28, 2022

try

Build succeeded:

@Alexhuszagh
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented May 28, 2022

Build succeeded:

@bors bors bot merged commit c8df353 into cross-rs:main May 28, 2022
@Emilgardis Emilgardis added this to the v0.2.2 milestone Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cross --list lists commands that will not run
2 participants