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

fix: only inherit workspace package table if the new package is a member #13261

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Jan 8, 2024

What does this PR try to resolve?

part of #13235

We should not inherit from the workspace if the new package is in the exclude list.

  • Updated the test case to cover it.
  • Do not inherit the workspace package table if it is not a member of the workspace.

How should we test and review this PR?

Check out the unit tests.

Additional information

None

@Rustin170506 Rustin170506 marked this pull request as draft January 8, 2024 13:59
@rustbot
Copy link
Collaborator

rustbot commented Jan 8, 2024

r? @epage

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 8, 2024
@Rustin170506 Rustin170506 changed the title test: correct not_inherit_workspace_package_table_if_not_members case WIP: test: correct not_inherit_workspace_package_table_if_not_members case Jan 8, 2024
@Rustin170506 Rustin170506 changed the title WIP: test: correct not_inherit_workspace_package_table_if_not_members case fix: only inherit workspace package table if the new package is a member Jan 8, 2024
@Rustin170506 Rustin170506 marked this pull request as ready for review January 8, 2024 14:54
@Rustin170506 Rustin170506 force-pushed the rustin-patch-not_inherit_workspace_package_table_if_not_members branch from 84ba0ae to 96d3dee Compare January 8, 2024 14:57
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

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

epage commented Jan 8, 2024

part of #13235

If this doesn't close that issue, then what is left after this?

@Rustin170506 Rustin170506 force-pushed the rustin-patch-not_inherit_workspace_package_table_if_not_members branch from 96d3dee to 47f4b75 Compare January 9, 2024 13:24
@Rustin170506
Copy link
Member Author

GitHub is down.

@Rustin170506 Rustin170506 force-pushed the rustin-patch-not_inherit_workspace_package_table_if_not_members branch from 7df9fc3 to db4ed03 Compare January 9, 2024 14:10
@Rustin170506
Copy link
Member Author

GitHub is down.

If this doesn't close that issue, then what is left after this?

This is another bug in that issue.

tree . --gitignore
.
├── Cargo.toml
├── crates
│   └── crate1
│       ├── Cargo.toml
│       └── src
│           └── main.rs
├── data
│   └── test2
│       ├── Cargo.toml
│       └── src
│           └── main.rs
└── src
    └── main.rs

8 directories, 6 files

The root workspace:

[package]
name = "test1"
version = "0.1.0"
edition = "2021"

[workspace]
resolver = "2"
members = ["crates/*", "data/test2"]
exclude = ["data/**"]


[workspace.package]
version = "0.1.0"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]

If you create a package inside the data directory, it will inherit the version from the workspace and add the package into the members list, even if it should be ignored by the exclude glob.

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

@@ -993,46 +1042,9 @@ fn update_manifest_with_new_member(
.as_str()
.with_context(|| format!("invalid non-string exclude path `{}`", member))?;
if pat == display_path {
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 think this problem comes from here. Shouldn't we consider glob here?

Copy link
Contributor

Choose a reason for hiding this comment

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

According to #6009, we don't support globs on exclude...

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the problem with globs or that exclude is a path prefix?

Copy link
Member Author

Choose a reason for hiding this comment

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

According to #6009, we don't support globs on exclude...

Oh, I forgot that 🤣

Is the problem with globs or that exclude is a path prefix?

No. It works well.

@Rustin170506 Rustin170506 requested a review from epage January 9, 2024 14:24
@epage
Copy link
Contributor

epage commented Jan 9, 2024

@bors r+

This fixes a definitive problem and we can work on the question of glob excludes separate from this

@bors
Copy link
Contributor

bors commented Jan 9, 2024

📌 Commit db4ed03 has been approved by epage

It is now in the queue for this repository.

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

bors commented Jan 9, 2024

⌛ Testing commit db4ed03 with merge b90f770...

@bors
Copy link
Contributor

bors commented Jan 9, 2024

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

@bors bors merged commit b90f770 into rust-lang:master Jan 9, 2024
22 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 10, 2024
Update cargo

14 commits in 2ce45605d9db521b5fd6c1211ce8de6055fdb24e..3e428a38a34e820a461d2cc082e726d3bda71bcb
2024-01-04 18:04:13 +0000 to 2024-01-09 20:46:36 +0000
- refactor: replace `iter_join` with `itertools::join` (rust-lang/cargo#13275)
- docs(unstable): doc comments for items and fields (rust-lang/cargo#13274)
- crates-io: Set `Content-Type: application/json` only for requests with a body payload (rust-lang/cargo#13264)
- fix: only inherit workspace package table if the new package is a member (rust-lang/cargo#13261)
- feat(cli): add colors to `-Zhelp` console output (rust-lang/cargo#13269)
- chore(deps): update msrv (rust-lang/cargo#13266)
- refactor(toml): Make it more obvious to update package-dependent fields (rust-lang/cargo#13267)
- chore(ci): Fix MSRV:3 updates (rust-lang/cargo#13268)
- chore(ci): Shot-in-the-dark fix for MSRV updating (rust-lang/cargo#13265)
- fix: set OUT_DIR for all units with build scripts (rust-lang/cargo#13204)
- fix(manifest): Provide unused key warnings for lints table (rust-lang/cargo#13262)
- test(manifest): Verify we warn on unused workspace.package fields (rust-lang/cargo#13263)
- docs(changelog): Call out cargo-new lockfile change (rust-lang/cargo#13260)
- chore: Add dependency dashboard (rust-lang/cargo#13255)

r? ghost
@rustbot rustbot added this to the 1.77.0 milestone Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants