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.

Sign up for GitHub

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

Jump to bottom

Only filter out target if its in the package root #12944

Merged
merged 5 commits into from
Nov 14, 2023
Merged

Only filter out target if its in the package root #12944

merged 5 commits into from
Nov 14, 2023

Conversation

linyihai
Copy link
Contributor

@linyihai linyihai commented Nov 9, 2023 •
edited
Loading

What does this PR try to resolve?

Only filter out target if its in the package root. Fixed #12790

How should we test and review this PR?

Add two testcase in tests/testsuite/package.rs for this PR.

  • include_files_called_target_project testcase test the logic for none git repository. By the way, our PR was based on git repository, so this testcase was a little irrelevant.
  • include_files_called_target_git testcase was made for git repository. There are two cases here, one is the target in the uncommitted state, at this time should not be included, and one is the target in the committed state, at this time should be included

Additional information

@rustbot
Copy link
Collaborator

rustbot commented Nov 9, 2023

r? @weihanglo

(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 Nov 9, 2023
@linyihai linyihai closed this Nov 9, 2023
@linyihai linyihai reopened this Nov 9, 2023
@linyihai linyihai marked this pull request as ready for review November 9, 2023 09:11
weihanglo
weihanglo requested changes Nov 9, 2023
View reviewed changes
Copy link
Member

@weihanglo weihanglo 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 the contribution!

Could you add some tests for this patch? Such as one commit adding a test to demonstrate the current "bad" behavior, and the next commit to fix both bug and test.

@rustbot rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 9, 2023
@KarelPeeters
Copy link
Contributor

I wrote a unit test for this a while back, I've pushed it here: https://github.com/KarelPeeters/cargo/commits/package-target

Feel free to cherry pick or take inspiration!

@linyihai
Copy link
Contributor Author

I wrote a unit test for this a while back, I've pushed it here: https://github.com/KarelPeeters/cargo/commits/package-target

Feel free to cherry pick or take inspiration!

That's awesome ! It's really a big help and can save a lot of work. Good Job.

@linyihai linyihai marked this pull request as draft November 10, 2023 08:02
@linyihai linyihai marked this pull request as ready for review November 13, 2023 02:03
@weihanglo weihanglo self-requested a review November 13, 2023 21:09
weihanglo
weihanglo reviewed Nov 13, 2023
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks. Just some nits in tests.

tests/testsuite/package.rs Outdated Show resolved Hide resolved
@weihanglo
Copy link
Member

Looks awesome. Thank you for the contribution!

@bors r+

@bors
Copy link
Contributor

bors commented Nov 14, 2023

📌 Commit b2b026b has been approved by weihanglo

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-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Nov 14, 2023
@bors
Copy link
Contributor

bors commented Nov 14, 2023

⌛ Testing commit b2b026b with merge dd94e9d...

@bors
Copy link
Contributor

bors commented Nov 14, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing dd94e9d to master...

@bors bors merged commit dd94e9d into rust-lang:master Nov 14, 2023
20 checks passed
@KarelPeeters
Copy link
Contributor

@linyihai Thanks a lot for fixing this bug (and the tests)!

@weihanglo weihanglo mentioned this pull request Nov 14, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 15, 2023
@bors
Auto merge of rust-lang#117916 - weihanglo:update-cargo, r=weihanglo
d1883fb 
Update cargo

15 commits in 6790a5127895debec95c24aefaeb18e059270df3..87ee3e96285e0142b71d8c11c02b18647e43974d
2023-11-10 17:09:35 +0000 to 2023-11-14 18:00:46 +0000
- fix error message for duplicate links (rust-lang/cargo#12973)
- Only filter out target if its in the package root (rust-lang/cargo#12944)
- Ignore changing_spec_relearns_crate_types on windows-gnu (rust-lang/cargo#12972)
- fix: do not panic when failed to parse rustc commit-hash (rust-lang/cargo#12965)
- query{_vec} use IndexSummary (rust-lang/cargo#12970)
- Bump to 0.77.0; update changelog (rust-lang/cargo#12966)
- Improve about information of `cargo search` (rust-lang/cargo#12962)
- Fix --quiet being used with nested subcommands. (rust-lang/cargo#12959)
- make some debug assertion failures more informative (rust-lang/cargo#12963)
- refactor(toml): Consistently lead with 'Toml' prefix (rust-lang/cargo#12960)
- refactor(toml): Remove unused method (rust-lang/cargo#12961)
- Fix non-deterministic behavior in last-use repopulation (rust-lang/cargo#12958)
- Add cache garbage collection (rust-lang/cargo#12634)
- refactor(toml): Improve consistency (rust-lang/cargo#12954)
- Fix typo (rust-lang/cargo#12956)

r? ghost
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 15, 2023
@bors
Auto merge of rust-lang#117916 - weihanglo:update-cargo, r=weihanglo
76265ff 
Update cargo

15 commits in 6790a5127895debec95c24aefaeb18e059270df3..87ee3e96285e0142b71d8c11c02b18647e43974d
2023-11-10 17:09:35 +0000 to 2023-11-14 18:00:46 +0000
- fix error message for duplicate links (rust-lang/cargo#12973)
- Only filter out target if its in the package root (rust-lang/cargo#12944)
- Ignore changing_spec_relearns_crate_types on windows-gnu (rust-lang/cargo#12972)
- fix: do not panic when failed to parse rustc commit-hash (rust-lang/cargo#12965)
- query{_vec} use IndexSummary (rust-lang/cargo#12970)
- Bump to 0.77.0; update changelog (rust-lang/cargo#12966)
- Improve about information of `cargo search` (rust-lang/cargo#12962)
- Fix --quiet being used with nested subcommands. (rust-lang/cargo#12959)
- make some debug assertion failures more informative (rust-lang/cargo#12963)
- refactor(toml): Consistently lead with 'Toml' prefix (rust-lang/cargo#12960)
- refactor(toml): Remove unused method (rust-lang/cargo#12961)
- Fix non-deterministic behavior in last-use repopulation (rust-lang/cargo#12958)
- Add cache garbage collection (rust-lang/cargo#12634)
- refactor(toml): Improve consistency (rust-lang/cargo#12954)
- Fix typo (rust-lang/cargo#12956)

r? ghost
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 16, 2023
@bors
Auto merge of rust-lang#117916 - weihanglo:update-cargo, r=weihanglo
525c91d 
Update cargo

19 commits in 6790a5127895debec95c24aefaeb18e059270df3..2c03e0e2dcd05dd064fcf10cc1050d342eaf67e3
2023-11-10 17:09:35 +0000 to 2023-11-16 04:21:44 +0000
- docs(ref): Find a place to comment on --cap-lints (rust-lang/cargo#12976)
- Switch from AtomicU64 to Mutex. (rust-lang/cargo#12981)
- If the only path is a loop then counted as the shortest path. (rust-lang/cargo#12977)
- fix(resolver): Prefer MSRV, rather than ignore incompatible (rust-lang/cargo#12950)
- fix error message for duplicate links (rust-lang/cargo#12973)
- Only filter out target if its in the package root (rust-lang/cargo#12944)
- Ignore changing_spec_relearns_crate_types on windows-gnu (rust-lang/cargo#12972)
- fix: do not panic when failed to parse rustc commit-hash (rust-lang/cargo#12965)
- query{_vec} use IndexSummary (rust-lang/cargo#12970)
- Bump to 0.77.0; update changelog (rust-lang/cargo#12966)
- Improve about information of `cargo search` (rust-lang/cargo#12962)
- Fix --quiet being used with nested subcommands. (rust-lang/cargo#12959)
- make some debug assertion failures more informative (rust-lang/cargo#12963)
- refactor(toml): Consistently lead with 'Toml' prefix (rust-lang/cargo#12960)
- refactor(toml): Remove unused method (rust-lang/cargo#12961)
- Fix non-deterministic behavior in last-use repopulation (rust-lang/cargo#12958)
- Add cache garbage collection (rust-lang/cargo#12634)
- refactor(toml): Improve consistency (rust-lang/cargo#12954)
- Fix typo (rust-lang/cargo#12956)
@ehuss ehuss added this to the 1.76.0 milestone Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.

Cargo publish excludes files called target
6 participants
@linyihai @rustbot @KarelPeeters @weihanglo @bors @ehuss