rust-lang / cargo

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

cargo add: Show features that were explicitly enabled #10681

Open Aloso opened 2 years ago

Aloso commented 2 years ago

Problem

cargo add can be used to enable features of an existing dependency. In that situation, it's useful to see which features were enabled explicitly, i.e. are listed in the Cargo.toml file.

Proposed Solution

In the list of features, add the text (enabled explicitly) after explicitly enabled features:

cargo add once_cell -F parking_lot,unstable
    Updating crates.io index
      Adding once_cell v1.11.0 to dependencies.
             Features:
             + alloc
             + parking_lot (enabled explicitly)
             + parking_lot_core
             + race
             + std
             + unstable (enabled explicitly)
             - atomic-polyfill

Another possibility is to only show this with the --verbose/-v flag, but I think it's useful enough that it can be shown always.

Notes

No response

epage commented 2 years ago

While we shouldn't show state exclusively through formatting, we could also bold these items.

epage commented 2 years ago

In terms of intended use cases, this is a bit lower in the priority list for cargo-add

  1. Add a new dependency: they user knows what was explicitly enabled, we don't have to tell them
  2. Run a command from documentation and it just works regardless of the existing manifest: they just want it to work, knowing exactly what happened doesn't matter
  3. Switch back and forth between registry and git dependency: features aren't as relevant
  4. Be a feature editor: this is the case where knowing the state of the manifest is more important
epage commented 2 years ago

In doing this, we might also want to explicitly list a default feature to show whether it is explicitly enabled or not.

marcospb19 commented 1 year ago

I'm taking my shot at this one.

weihanglo commented 1 year ago

@marcospb19 Please note that this is still in discussion, so probably better sharing your idea before filing a pull request.

Aloso commented 1 year ago

@weihanglo where is that discussion taking place? If there are any concerns, I would like to know about them. I assumed that the "Proposed Solution" in my issue would be uncontroversial, because there was no feedback here suggesting otherwise.

weihanglo commented 1 year ago

I don't really have a strong preference at this moment. Just need to weigh on the complexity added and the value gained. Perhaps the implementation turns out to be quite simple! @epage may have more thoughts since he commented https://github.com/rust-lang/cargo/issues/10681#issuecomment-1133026120.

That said, it's always worth an exploration if someone wants to deep dive into the codebase.

epage commented 1 year ago

@Aloso lack of feedback is not consent for change. For non-trivial Issues, we label them as S-accepted when we've decided we are on board with it.

For me, this is a lot of visual noise with lower value and I lean against making the change, especially when we have other information we might want to display in the future, like features that are unstable which deserve more visual attention.

I'm also not really seeing the need for reporting which features were enabled explicitly

Aloso commented 1 year ago

@epage thank you for elaborating, I appreciate the feedback.

marcospb19 commented 1 year ago

I think the original proposal adds visual noise, but it can be improved so I'd love to discuss more what we're trying to achieve and how to best achieve that.

I'm also not really seeing the need for reporting which features were enabled explicitly

I use cargo-add a lot and its output always gets me confused due to the nature of crate features:

  1. Some features enable other features (implicit features).
  2. Crates can enable other crates' features.
  3. Default feature lists might be toggled off.

I like to reduce the number of features and crates in order to decrease compile time and workspace folder size, so I always ask myself some questions:

I usually go to docs.rs or read the source code to answer these questions, but I think cargo-add could answer it right away.

I also ask "How many (and which) dependencies did this feature add?", even though I don't want cargo-add to answer this directly, answering the above questions would give me a shorter investigation path to figure it all out.

About implicit features: they obfuscate the rest of them while being mostly useless, and most people don't know about them (especially beginners), if cargo-add marks them that would change (lots of people are using cargo-add) and I believe it would help the ecosystem migrate to the new dep: syntax.

Motivation

My main motivation is providing an easy path to achieve lower compilation times and avoid compiling/checking/linting on unused crates, but understanding how a crate structures its features (and deps) is very helpful for countless reasons.

Brainstorming a solution

I don't know if that's something for cargo-add to solve or if we'd prefer in another subcommand.

But here's an uglier and arguably more useful output example:

Explicitly enabled features:
 + serde
 + small_rng
Enabled by default:
 + std
 + std_rng
 + getrandom
 + rand_chacha
 + alloc
Implicitly enabled by other feature:
 + example1
 + example2
Disabled features:
 - log
 - min_const_gen
 - nightly
 - packed_simd
 - serde1
 - simd_support

(I'd expect the "Implicitly enabled by other feature:" section to not be present most of the time, as crates start using the new feature-dep syntax)

How it looks like in the terminal:

image

That's just an example to show how it could be done without introducing too much visual noise.

Any thoughts?

epage commented 1 year ago

Can I remove this feature?

I think this is a good sign we might be using the wrong tool. cargo add cannot remove features and isn't meant to be a general viewer for analyzing them so if someone is trying to optimize for that, we probably should look else where, like #948

BartMassey commented 7 months ago

@marcospb19 I really like your proposed plan for greater feature transparency. Understanding what cargo add is reporting is important. My thing a bit ago in #13652 would have been solved by this.

epage commented 7 months ago

Counter proposal for the output

Today Proposal
Directly enabled features + prefix + prefix, colored
Indirectly enabled features + prefix colored
Inactive features - prefix no color or dimmed

Using + for explicitly enabled features would make this align with #10809.

While we'd be using color to distinguish a state to the user, this is likely a lower priority of state and seems like it'd be reasonable.

This doesn't add anything more to the UI so we'd be free to distinguish other state in the future.

BartMassey commented 7 months ago

The counter-proposal would certainly be an improvement over the current situation and would be welcome.

That said, I would prefer some signifier in the text itself that made it easier for my students and I to figure out what cargo add was telling us. It would also be nice to know the source of implicit features.

Counter-counter-proposal :grinning::

$ cargo add mycrate --features f2,f5 --no-default-features
    Updating crates.io index
      Adding mycrate v0.1 to dependencies.
             Features:
             + f2
             + f1 (from f2, f5)
             - f3 (default)
             - f4

Would also probably want to add (default) to defaulted-in features:

$ cargo add mycrate --features f1
    Updating crates.io index
      Adding mycrate v0.1 to dependencies.
             Features:
             + f1
             + f3 (default)
             - f2
             - f4
             - f5

If nothing else, that would mean we could copy-paste the output into issue reports here without losing anything :smile:.

I haven't looked at cargo data structures enough to know how much it currently knows about where its features are coming from: thus, I don't know how big a code change this would be.

BartMassey commented 7 months ago

I assume that it is intended that cargo add --feature is cumulative? That is, if I

$ cargo add --features=f1 mycrate
    Updating crates.io index
      Adding mycrate v0.1.0 to dependencies.
             Features:
             + f1
             - f2
$ cargo add --features=f2 mycrate
    Updating crates.io index
      Adding mycrate v0.1.0 to dependencies.
             Features:
             + f1
             + f2

Thus, to get rid of the current accumulated features, the easiest solution is to cargo remove and then re-add with cargo add (or just edit Cargo.toml, of course).

This behavior surprised me. I would love to see it indicated in help messages and whatnot. Alternatively, I can file an issue if it wasn't intended.

epage commented 7 months ago

That proposal does not address the problems stated earlier. There is other relevant information being discussed in RFCs, like descriptions, unstable status, etc. What of those would be the most important to show, and why? How do we balance showing the information so the output remains usable.

cargo add is not a genera purpose feature tool. Its to help you know what you added and what you might want to add.

Some feature information is surfaced in cargo tree which we note when explaining dependency resolution.

There is also active work on a cargo info command which could also potentially show more detailed feature information.

epage commented 7 months ago

I assume that it is intended that cargo add --feature is cumulative?

All of cargo add is cumulative. Consider:

$ cargo add clap@3.5
$ cargo add clap --optional
$ cargo add clap -F derive

This results in

clap = { version = "3.5", optional = true, features = ["derive"] }

The user is able to see the output from cargo add and then adjust as needed or go back and later adjust things without re-typing the whole command (if its lost in their history buffer or they didn't think to do use it)

BartMassey commented 7 months ago

Thanks much for the prompt and cogent replies! Like I say, a color change would be much better than nothing, so if that's what is offered I'll surely take it.

My current workflow is often cargo add, look at the listed features, cargo remove if necessary, then cargo add again with the features I now know I want. It would be great to short-circuit that somehow. I'll try installing cargo-information and see how that goes: thanks much for the link.