Show the merged set of activated features to the user when inheriting… #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
epage
reviewed
Apr 25, 2022
src/cargo/ops/cargo_add/mod.rs
Outdated
Comment on lines
118
to
120
| req_feats | ||
| .difference(&available_features) | ||
| .for_each(|unknown| unknown_features.push(unknown.to_string())); |
There was a problem hiding this comment.
I think a unknown_features.extend(req_feats.different(...)) would b clearer than the for_each approach
There was a problem hiding this comment.
you run into issues with &&str not living long enough. You can transition things to String but then you introduce .to_string() and .clone()
There was a problem hiding this comment.
Try unknown_features.extend(req_feats.different(...).copied())
src/cargo/ops/cargo_add/mod.rs
Outdated
Comment on lines
680
to
675
| let mut workspace: IndexSet<_> = dep | ||
| .inherited_features | ||
| .iter() | ||
| .flatten() | ||
| .map(|s| s.as_str()) | ||
| .filter(|s| !activated.contains(s)) | ||
| .collect(); | ||
| workspace.sort(); | ||
| let mut deactivated = dep |
There was a problem hiding this comment.
This won't recursively activate features.
Instead add this into activated before we do the VecDeque and the rest of the code should just work out
src/cargo/ops/cargo_add/mod.rs
Outdated
Comment on lines
704
to
689
| for feat in workspace { | ||
| shell.write_stderr(&prefix, &ColorSpec::new())?; | ||
| shell.write_stderr('+', &ColorSpec::new().set_bold(true).set_fg(Some(Green)))?; | ||
| shell.write_stderr(format_args!(" {}\n", feat), &ColorSpec::new())?; | ||
| } | ||
| for feat in deactivated { |
There was a problem hiding this comment.
This isn't sorting the workspace features in with the regular ones and will look confusing.
If you merge into the activated features, this shouldn't be needed anymore
Comment on lines
5
to
7
| [features] | ||
| test = [] | ||
| merge = [] |
There was a problem hiding this comment.
Can you change this to
[features]
default-base = []
default-test-base = []
default-merge-base = []
default = ["default-base", "default-test-base", "default-merge-base"]
test-base = []
test = ["test-base", "default-test-base"]
merge-base = []
merge = ["merge-base", "default-merge-base"]
unrelated = []This will
- Show we recursively activate all
- Don't duplicate activations
- Don't just enable everything :)
| @@ -152,6 +155,13 @@ impl Dependency { | |||
| self | |||
| } | |||
|
|
|||
| /// Set features as an array of string (does some basic parsing) | |||
| #[allow(dead_code)] | |||
There was a problem hiding this comment.
I believe I just copied this from above. I'll remove it
… from a workspace dependency
Muscraft
force-pushed
the
merged-activated-features
branch
from
02a5439 to
55cf27e
Compare
epage
approved these changes
Apr 25, 2022
This was referenced Apr 25, 2022
bors
added a commit
to rust-lang/cargo
that referenced
this pull request
Apr 28, 2022
Cargo add support for workspace inheritance Tracking issue: #8415 RFC: rust-lang/rfcs#2906 This PR adds all the required support for workspace inheritance within `cargo-add`. It was split up across a few different PRs as it required `snapbox` support from #10581 and a new `toml_edit` version from #10603. `@epage` and I decided to go ahead with this PR and add in some of the changes those PRs made. `@epage's` name on the commits is from helping to rewrite commits and some very minor additions. Changes: - #10585 - Muscraft#1 - Muscraft#3 - Muscraft#2 - Muscraft#4 r? `@epage`
Muscraft
pushed a commit
that referenced
this pull request
Apr 27, 2025
…st-lang#15405) Resolves rust-lang#15401 Here is an example of the feature in [kitty](https://sw.kovidgoyal.net/kitty/) `0.40.1` with the following config set in `~/.config/kitty/kitty.conf` ```conf underline_hyperlinks always show_hyperlink_targets yes allow_hyperlinks yes # ... ```  Tested on `uname -a`: ``` Linux nixos 6.14.0 #1-NixOS SMP PREEMPT_DYNAMIC Mon Mar 24 14:02:41 UTC 2025 x86_64 GNU/Linux ``` Terminals tested with: - [x] [kitty](https://sw.kovidgoyal.net/kitty/) `0.40.1` - [x] [ghostty](https://ghostty.org/) `1.1.4-6f1b22a-nix`  - [x] [alacritty](https://alacritty.org/index.html) `0.15.1`  - [x] VScode's version `1.98` integrated terminal aka. [xterm.js](https://xtermjs.org/)  The following `cargo` invocations will have their output be modified by this change: ```shell cargo run # If multiple binaries are defined in the manifest and [package.default-bin] is not defined cargo run --bin cargo run --example cargo run --package cargo build --bin cargo build --example cargo build --package cargo test --test cargo test --bench ``` In addition I have done a slight refactor to have the printed indent of targets and packages be the same by using a shared constant named `const ITEM_INDENT: &str = " ";` This is my first PR to the cargo codebase, so I am not familiar with what is expected in terms of test for a feature such as this. <!-- Thanks for submitting a pull request 🎉! Here are some tips for you: * If this is your first contribution, read "Cargo Contribution Guide" first: https://doc.crates.io/contrib/ * Run `cargo fmt --all` to format your code changes. * Small commits and pull requests are always preferable and easy to review. * If your idea is large and needs feedback from the community, read how: https://doc.crates.io/contrib/process/#working-on-large-features * Cargo takes care of compatibility. Read our design principles: https://doc.crates.io/contrib/design.html * When changing help text of cargo commands, follow the steps to generate docs: https://github.com/rust-lang/cargo/tree/master/src/doc#building-the-man-pages * If your PR is not finished, set it as "draft" PR or add "WIP" in its title. * It's ok to use the CI resources to test your PR, but please don't abuse them. ### What does this PR try to resolve? Explain the motivation behind this change. A clear overview along with an in-depth explanation are helpful. You can use `Fixes #<issue number>` to associate this PR to an existing issue. ### How should we test and review this PR? Demonstrate how you test this change and guide reviewers through your PR. With a smooth review process, a pull request usually gets reviewed quicker. If you don't know how to write and run your tests, please read the guide: https://doc.crates.io/contrib/tests ### Additional information Other information you want to mention in this PR, such as prior arts, future extensions, an unresolved problem, or a TODO list. -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tracking issue: rust-lang#8415
RFC: rust-lang/rfcs#2906
PRs in this RFC:
Cargo.tomlrust-lang/cargo#10517license-path, anddepednency.pathrust-lang/cargo#10538readmerust-lang/cargo#10548rust-versionrust-lang/cargo#10563excludeandincluderust-lang/cargo#10565workspaceto… rust-lang/cargo#10564InheritableFieldsin aLazyCellinside `… rust-lang/cargo#10568key.workspace = truetokey = { workspace = true }rust-lang/cargo#10584cargo-addsupportcargo add foowhen afoo.workspace = truealready exists rust-lang/cargo#10585Changes:
inheritable_featurestocargo_add::Dependencyprint_msgto print outinheritable_featuresas neededinheritable_featuresinside of available_featuresRemaining implementation work for the RFC
cargo-addsupport, see epage's commentr? @epage