install fails earlier when no binaries can be found

[marshall]

fixes #8970

this automatically filters out the lib targets in cargo install when no specific targets are provided by the user, so they won't get accidentally built.

now when you try to install a crate with no satisfied bin targets, cargo will skip the meat of the compile phase and give the same error message it currently gives when it finds no bin targets at all.

a new flag called warn_unmatched was also added to CompileFilter::Only that can suppress the warning about unmatched target filters.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[marshall]

I've been testing locally with the cargo-lock crate, which has a bin target that requires the cli feature. on my machine, cargo takes 15.88s to build and determine no binaries can be found and looks like this:

$ cargo install cargo-lock
    Updating crates.io index
  Installing cargo-lock v7.0.0
   Compiling proc-macro2 v1.0.27
   Compiling unicode-xid v0.2.2
   ... [clipped for brevity]
   Compiling cargo-lock v7.0.0
    Finished release [optimized] target(s) in 15.88s
error: no binaries are available for install using the selected features

with this patch, cargo gives the same error, but has skipped the compile phase, meaning it only takes 1.26s:

$ ./target/debug/cargo install cargo-lock
    Updating crates.io index
  Installing cargo-lock v7.0.0
    Finished release [optimized] target(s) in 1.26s
error: no binaries are available for install using the selected features

[ehuss]

r? @ehuss

Thanks for the PR!

Adding compile modes can cause some subtle issues since it is threaded in so many places. I was wondering if it would be possible to just change the filter instead? That is, perhaps somewhere in cargo/commands/install.rs, have something like this:

    if !compile_opts.filter.is_specific() {
        compile_opts.filter = CompileFilter::new(
            LibRule::False,
            FilterRule::All,
            FilterRule::Just(vec![]),
            FilterRule::Just(vec![]),
            FilterRule::Just(vec![]),
        );
    }

This should change it so that if all the bins get filtered by required-features, it should quickly present an error that there aren't any matching bins.

As for the test, I would probably just update required_features::install_arg_features to run cargo install first, and check for the error output (with_stderr("...")). There is a guide for writing/running tests at https://doc.crates.io/contrib/tests/writing.html, and there should be plenty of examples in the testsuite/required_features.rs and testsuite/install.rs files for how the tests look.

[marshall]

heya @ehuss, thanks so much for the review.. I'm finally getting around to this feedback.

I ran with your suggestion and tweaked it a bit to make sure a spurious warning wasn't accidentally reported when overriding the CompileFilter (see more info in my commit msg).

I realized there were a lot of tests already covering install with or without the necessary features in required_features, and I had to update a few to pass with the slight change in error message brought on by this change.

I could also see an argument for diving deeper and making sure external dependencies aren't fetched when trying to install a binary that doesn't have a feature requirement met -- this would actually exercise the new behavior more fully. not sure how important that is though, wdyt?

[marshall]

forgot to mention, I force pushed my changes as a new commit because this was a totally different approach. hope that is OK!

[ehuss]

Hm, so this seems to cause a regression in the error message for trying to install a library. I think we want to try to keep the original error message there, as it contains some important help for new users. In full, it looks like this:

error: there is nothing to install in `toml v0.5.8`, because it has no binaries
`cargo install` is only for installing programs, and can't be used with libraries.
To use a library crate, add it as a dependency in a Cargo project instead.

Changing that to no binaries are available for install using the selected feature loses that information.

Would it be possible to rework the check here to retain that error?

making sure external dependencies aren't fetched when trying to install a binary that doesn't have a feature requirement met

I think that might be too difficult or too complex to be worth attempting.

I force pushed my changes as a new commit because this was a totally different approach. hope that is OK!

Yep, that's fine.

[ehuss]

Ping @marshall Just checking in to see if you are still interested in working on this, or if you had any questions.

[marshall]

@ehuss thanks for the ping, I meant to address this a while ago. I will send an updated patch soon

[marshall]

heya @ehuss, I addressed both of your concerns in this latest commit. lmk what you think!

[ehuss]

This doesn't seem to have any tests for the new behavior.

[ehuss]

I'm a little unclear, why doesn't this just check for the warn_unmatched value?

[bors]

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

[ehuss]

Ping @marshall, just wondering if you're still interested in working on this or if you had any questions.

[ehuss]

I'm closing this due to inactivity. If you (or anyone else) wants to pick this up again, feel free to open a new PR (it looks like #10842 is currently open).
Thanks for your contribution!