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

Automatically inherit workspace fields when running cargo new/init #12069

Merged
merged 4 commits into from
May 23, 2023

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented May 1, 2023

What does this PR try to resolve?

close #10870

One thing to note: we will try to ensure the order of the inherited fields. You can see more at #12069 (comment)

How should we test and review this PR?

  • unit tests

@rustbot
Copy link
Collaborator

rustbot commented May 1, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added Command-new S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 1, 2023
Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Working in process.

src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
@Rustin170506 Rustin170506 force-pushed the rustin-patch-ws branch 9 times, most recently from 0df76c9 to ba20265 Compare May 3, 2023 05:33
@Rustin170506 Rustin170506 marked this pull request as ready for review May 3, 2023 05:34
Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Self check

@@ -24,6 +24,7 @@ mod cargo_command;
mod cargo_config;
mod cargo_env_config;
mod cargo_features;
mod cargo_new;
Copy link
Member Author

Choose a reason for hiding this comment

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

I will move other tests for the new command to this mod after this PR merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

@epage Do you believe it's necessary to migrate it, or can we retain it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Moving things around are under discussion, so don't worry about it

@Rustin170506
Copy link
Member Author

r? @epage

@rustbot rustbot assigned epage and unassigned ehuss May 3, 2023
@Rustin170506 Rustin170506 requested a review from epage May 3, 2023 06:06
Copy link
Member

@Muscraft Muscraft left a comment

Choose a reason for hiding this comment

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

This appears to be happening inside of mv, which is used by cargo new and cargo init. I am not sure if we want to support this for cargo init, currently only cargo new is specified in #10870. This should probably be brought up and discussed in #10870 before this is merged.

src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented May 4, 2023

This appears to be happening inside of mv, which is used by cargo new and cargo init. I am not sure if we want to support this for cargo init, currently only cargo new is specified in #10870. This should probably be brought up and discussed in #10870 before this is merged.

It specified new because it is easy to overlook one or the other. Looking at the documentation for init, it doesn't seem like we advertise these as slightly different roles, so I think its reasonable to do for both

@Rustin170506 Rustin170506 force-pushed the rustin-patch-ws branch 2 times, most recently from c19e6dd to 89c6a6f Compare May 4, 2023 01:33
@Rustin170506

This comment was marked as outdated.

@Rustin170506

This comment was marked as outdated.

@rustbot rustbot added the A-manifest Area: Cargo.toml issues label May 13, 2023
@Rustin170506 Rustin170506 force-pushed the rustin-patch-ws branch 2 times, most recently from f7895e9 to 72205b8 Compare May 23, 2023 01:52
src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Self check

Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this and sticking through the more dramatic changes in the approach! This both better prepares us for future changes and makes it easier to add other rich features to cargo add / cargo init.

Let me know if this is ready to merge or if you had other updates you were working on.

@Rustin170506
Copy link
Member Author

Let me know if this is ready to merge or if you had other updates you were working on.

I think it is ready to go.

Thanks for your work on this and sticking through the more dramatic changes in the approach! This both better prepares us for future changes and makes it easier to add other rich features to cargo add / cargo init.

Thanks for your patient review! 💚 💙 💜 💛 ❤️
This approach definitely better than before! Learned a lot.

@epage
Copy link
Contributor

epage commented May 23, 2023

@bors r+

@bors
Copy link
Contributor

bors commented May 23, 2023

📌 Commit 821725d has been approved by epage

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2023
@bors
Copy link
Contributor

bors commented May 23, 2023

⌛ Testing commit 821725d with merge 50d56ca...

@bors
Copy link
Contributor

bors commented May 23, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 50d56ca to master...

@bors bors merged commit 50d56ca into rust-lang:master May 23, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2023
Update cargo

10 commits in 09276c703a473ab33daaeb94917232e80eefd628..64fb38c97ac4d3a327fc9032c862dd28c8833b17
2023-05-16 21:43:35 +0000 to 2023-05-23 18:53:23 +0000
- Consider rust-version when selecting packages for cargo add (rust-lang/cargo#12078)
- fix(lints): Switch to -Zlints so stable projects can experiment (rust-lang/cargo#12168)
- Automatically inherit workspace fields when running cargo new/init (rust-lang/cargo#12069)
- ci: check if any version bump needed for member crates (rust-lang/cargo#12126)
- feat: `lints` feature (rust-lang/cargo#12148)
- fix: pass `-C debuginfo` after weakening if explicitly set (rust-lang/cargo#12165)
- Tweak build help to clarify role of --bin (rust-lang/cargo#12157)
- fix: Pass CI on nightly (rust-lang/cargo#12160)
- docs(source): doc comments for Source and its impls (rust-lang/cargo#12159)
- docs(source): doc comments for `Source` and friends (rust-lang/cargo#12153)

r? `@ghost`
saethlin pushed a commit to saethlin/miri that referenced this pull request May 26, 2023
Update cargo

10 commits in 09276c703a473ab33daaeb94917232e80eefd628..64fb38c97ac4d3a327fc9032c862dd28c8833b17
2023-05-16 21:43:35 +0000 to 2023-05-23 18:53:23 +0000
- Consider rust-version when selecting packages for cargo add (rust-lang/cargo#12078)
- fix(lints): Switch to -Zlints so stable projects can experiment (rust-lang/cargo#12168)
- Automatically inherit workspace fields when running cargo new/init (rust-lang/cargo#12069)
- ci: check if any version bump needed for member crates (rust-lang/cargo#12126)
- feat: `lints` feature (rust-lang/cargo#12148)
- fix: pass `-C debuginfo` after weakening if explicitly set (rust-lang/cargo#12165)
- Tweak build help to clarify role of --bin (rust-lang/cargo#12157)
- fix: Pass CI on nightly (rust-lang/cargo#12160)
- docs(source): doc comments for Source and its impls (rust-lang/cargo#12159)
- docs(source): doc comments for `Source` and friends (rust-lang/cargo#12153)

r? `@ghost`
@ehuss ehuss added this to the 1.71.0 milestone May 30, 2023
@epage
Copy link
Contributor

epage commented Jul 14, 2023

FYI @hi-rustin though this didn't make the release blog post, people seemed to be excited by it

So far my favorite quote:

That is a really cool feature! I didn't knew I needed it, but I actually do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-manifest Area: Cargo.toml issues Command-new S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically inherit workspace fields when running cargo new
6 participants