Skip to content
New issue

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

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

Already on GitHub? # to your account

cargo add: Show features that were explicitly enabled #10681

Open
Aloso opened this issue May 20, 2022 · 18 comments
Open

cargo add: Show features that were explicitly enabled #10681

Aloso opened this issue May 20, 2022 · 18 comments
Labels
A-console-output Area: Terminal output, colors, progress bar, etc. C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-add S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@Aloso
Copy link

Aloso commented May 20, 2022

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

@Aloso Aloso added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label May 20, 2022
@epage
Copy link
Contributor

epage commented May 20, 2022

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

@epage
Copy link
Contributor

epage commented May 20, 2022

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
Copy link
Contributor

epage commented May 20, 2022

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

@marcospb19
Copy link

I'm taking my shot at this one.

@weihanglo
Copy link
Member

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

@weihanglo weihanglo added A-console-output Area: Terminal output, colors, progress bar, etc. S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. labels Jul 16, 2023
@Aloso
Copy link
Author

Aloso commented Jul 16, 2023

@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
Copy link
Member

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 #10681 (comment).

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

@epage
Copy link
Contributor

epage commented Jul 17, 2023

@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
Copy link
Author

Aloso commented Jul 17, 2023

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

@marcospb19
Copy link

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:

  • Can I remove this feature?
    • What enabled it in the first place? Was it enabled ...
      • by another feature?
      • by default?
      • by me explicitly?
      • by another crate?

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
Copy link
Contributor

epage commented Jul 20, 2023

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
Copy link
Contributor

@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
Copy link
Contributor

epage commented Mar 26, 2024

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
Copy link
Contributor

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 😀:

$ 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 😄.

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

This comment was marked as off-topic.

@epage
Copy link
Contributor

epage commented Mar 26, 2024

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

This comment was marked as off-topic.

@BartMassey
Copy link
Contributor

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.

epage added a commit to epage/cargo-information that referenced this issue Apr 1, 2024
epage added a commit to epage/cargo-information that referenced this issue Apr 5, 2024
epage added a commit to epage/cargo-information that referenced this issue Apr 6, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-console-output Area: Terminal output, colors, progress bar, etc. C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-add S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

No branches or pull requests

5 participants