Only remove fingerprints and build script artifacts of the requested package #10621

CosmicHorrorDev · 2022-05-02T02:51:13Z

This is my first PR to cargo. Let me know if you want me to add tests, or if there are any other changes you would like to see :)

This PR changes the globs used to remove fingerprints and build artifacts when running cargo clean -p <pkid>. The glob used was <package_name>-* which would match artifacts for packages that are prefixed by <package_name>- (e.g. cargo clean -p sqlx would also remove artifacts for sqlx-{core,macros,etc.}). This problem is not seen with other artifacts since those use the crate name instead of package name which normalize hyphens to underscores.

The changes follow the naive approach mentioned in #10069 where some basic string manipulation is done to strip off the last hyphen, hash, and potential extension to get the original package name which can be used to determine if the artifact is actually for the desired package. This means that this does not handle trying to resolve the package to determine the artifacts, so it still ignores the url and version that may be passed in the pkgid

…package

rust-highfive · 2022-05-02T02:51:16Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

CosmicHorrorDev · 2022-05-02T03:03:13Z

The CI failures all look to be the same issue that I think is unrelated to this PR

weihanglo

Sorry for the long wait, and thanks for the patch!

It does make sense with this little hack. In the future, Cargo might introduce a small database to store these infos to make artefacts manipulations more deterministic.

As for tests, adding some in tests/testsuite/clean.rs would be awesome, thanks!

src/cargo/ops/cargo_clean.rs

CosmicHorrorDev · 2022-05-25T02:49:10Z

Thanks for the review, and no worries on the delay!

I just moved recently, so I'll be a bit busy during this week, but should be able to address the review comments and have a go at adding some tests this weekend

weihanglo · 2022-09-20T08:22:28Z

Ping @LovecraftianHorror. Hope the life is less busy now.
Just checking in to see if you are still interested in working on this, or if you had any questions.

CosmicHorrorDev · 2022-09-20T19:55:38Z

Thanks for the ping!

Sorry this ended up falling off my radar. Life is a bit busy for the next few weeks, but I'll have this at the top of my list of things to work on once I get back to OSS

…-artifact-cleanup

CosmicHorrorDev · 2022-10-30T06:03:30Z

Sorry for the long wait

All the review comments should be addressed now and I added a test to verify the behavior is right (PS the test helpers are very nice to work with)

weihanglo · 2022-10-30T09:49:53Z

Thank you for the update! I'll pick this up after my traveling.

r? @weihanglo

tests/testsuite/clean.rs

CosmicHorrorDev · 2022-11-01T03:51:29Z

Everything should hopefully be good after the last commit!

weihanglo

Thank you for the contribution so far! I am going to merge this soon.

weihanglo · 2022-11-01T09:59:04Z

tests/testsuite/clean.rs

+            name.rsplit_once('-')
+                .expect("Name should contain at least one hyphen")
+                .0
+                .to_owned()


It's a bit unfortunate we duplicate implementation logic here in test helper 😞. If you come up with a better way to avoid rsplit_once here, free feel to fix it afterward.

weihanglo · 2022-11-01T11:05:39Z

@bors r+

bors · 2022-11-01T11:05:41Z

📌 Commit 2ebcc75 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2022-11-01T11:49:15Z

⌛ Testing commit 2ebcc75 with merge 9e71316...

bors · 2022-11-01T12:49:36Z

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

jonhoo · 2022-11-01T15:17:07Z

src/cargo/ops/cargo_clean.rs

-                    rm_rf_glob(&Path::new(&dir).join(&pkg_dir), config, &mut progress)?;
+                    rm_rf_package_glob_containing_hash(
+                        &pkg.name(),
+                        &Path::new(&dir).join(&pkg_dir),


I wonder if we need to escape this path, otherwise a path that contains one of the glob special characters (like [ or ?) may end up doing weird things. I think there's a helper for that kind of escaping in cargo somewhere already since I think we had another bug (also in clean?) related up that in the past.

Ah yes: #10072

Thanks for the heads up! I can work on addressing that later this week unless someone else beats me to it

14 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..810cbad9a123ad4ee0a55a96171b8f8478ff1c03 2022-10-27 15:20:57 +0000 to 2022-11-02 21:04:31 +0000 - Update curl-sys (rust-lang/cargo#11326) - Mention fix on build script deadlock (rust-lang/cargo#11325) - Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285) - Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242) - Update the outdated link for rust-semverver (rust-lang/cargo#11322) - Fix broken link to compilation entry point (rust-lang/cargo#11317) - Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621) - Newer anyhow features are required (rust-lang/cargo#11316) - Clean stale git temp files (rust-lang/cargo#11308) - Report crate size on package and publish (rust-lang/cargo#11270) - add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989) - Update libcurl (rust-lang/cargo#11307) - artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183) - Fix singular verb in tests page (rust-lang/cargo#11300)

Update cargo 14 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..810cbad9a123ad4ee0a55a96171b8f8478ff1c03 2022-10-27 15:20:57 +0000 to 2022-11-02 21:04:31 +0000 - Update curl-sys (rust-lang/cargo#11326) - Mention fix on build script deadlock (rust-lang/cargo#11325) - Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285) - Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242) - Update the outdated link for rust-semverver (rust-lang/cargo#11322) - Fix broken link to compilation entry point (rust-lang/cargo#11317) - Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621) - Newer anyhow features are required (rust-lang/cargo#11316) - Clean stale git temp files (rust-lang/cargo#11308) - Report crate size on package and publish (rust-lang/cargo#11270) - add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989) - Update libcurl (rust-lang/cargo#11307) - artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183) - Fix singular verb in tests page (rust-lang/cargo#11300) r? `@ghost`

20 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..9286a1beba5b28b115bad67de2ae91fb1c61eb0b 2022-10-27 15:20:57 +0000 to 2022-11-04 06:41:49 +0000 - chore: Upgrade dependencies (rust-lang/cargo#11328) - Clean more aggressively in CI (rust-lang/cargo#11335) - Remove remove_dir_all (rust-lang/cargo#11333) - test(publish): Cover more wait-for-publish cases (rust-lang/cargo#11327) - Revert rust-lang/cargo#11183 (rust-lang/cargo#11331) - fix(semver-check): adapt to a different error for variant not covered (rust-lang/cargo#11332) - Update curl-sys (rust-lang/cargo#11326) - Mention fix on build script deadlock (rust-lang/cargo#11325) - Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285) - Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242) - Update the outdated link for rust-semverver (rust-lang/cargo#11322) - Fix broken link to compilation entry point (rust-lang/cargo#11317) - Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621) - Newer anyhow features are required (rust-lang/cargo#11316) - Clean stale git temp files (rust-lang/cargo#11308) - Report crate size on package and publish (rust-lang/cargo#11270) - add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989) - Update libcurl (rust-lang/cargo#11307) - artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183) - Fix singular verb in tests page (rust-lang/cargo#11300)

Update cargo 20 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..9286a1beba5b28b115bad67de2ae91fb1c61eb0b 2022-10-27 15:20:57 +0000 to 2022-11-04 06:41:49 +0000 - chore: Upgrade dependencies (rust-lang/cargo#11328) - Clean more aggressively in CI (rust-lang/cargo#11335) - Remove remove_dir_all (rust-lang/cargo#11333) - test(publish): Cover more wait-for-publish cases (rust-lang/cargo#11327) - Revert rust-lang/cargo#11183 (rust-lang/cargo#11331) - fix(semver-check): adapt to a different error for variant not covered (rust-lang/cargo#11332) - Update curl-sys (rust-lang/cargo#11326) - Mention fix on build script deadlock (rust-lang/cargo#11325) - Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285) - Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242) - Update the outdated link for rust-semverver (rust-lang/cargo#11322) - Fix broken link to compilation entry point (rust-lang/cargo#11317) - Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621) - Newer anyhow features are required (rust-lang/cargo#11316) - Clean stale git temp files (rust-lang/cargo#11308) - Report crate size on package and publish (rust-lang/cargo#11270) - add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989) - Update libcurl (rust-lang/cargo#11307) - artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183) - Fix singular verb in tests page (rust-lang/cargo#11300) r? `@ghost`

Only remove fingerprints and build script artifacts of the requested …

1f2d5ef

…package

rust-highfive assigned ehuss May 2, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2022

weihanglo reviewed May 25, 2022

View reviewed changes

src/cargo/ops/cargo_clean.rs Outdated Show resolved Hide resolved

src/cargo/ops/cargo_clean.rs Outdated Show resolved Hide resolved

src/cargo/ops/cargo_clean.rs Outdated Show resolved Hide resolved

weihanglo 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 May 25, 2022

CosmicHorrorDev added 3 commits October 29, 2022 11:49

Merge branch 'master' of github.com:rust-lang/cargo into more-precise…

738f084

…-artifact-cleanup

Add test to verify functionality

a6d0e96

Address review comments

604aeb5

CosmicHorrorDev requested a review from weihanglo October 30, 2022 06:03

weihanglo added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Oct 30, 2022

rustbot assigned weihanglo and unassigned ehuss Oct 30, 2022

weihanglo reviewed Oct 31, 2022

View reviewed changes

tests/testsuite/clean.rs Outdated Show resolved Hide resolved

tests/testsuite/clean.rs Outdated Show resolved Hide resolved

Actually make test effective

2ebcc75

weihanglo approved these changes Nov 1, 2022

View reviewed changes

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 Nov 1, 2022

bors merged commit 9e71316 into rust-lang:master Nov 1, 2022

jonhoo reviewed Nov 1, 2022

View reviewed changes

weihanglo mentioned this pull request Nov 2, 2022

Update cargo rust-lang/rust#103860

Merged

weihanglo mentioned this pull request Nov 4, 2022

Update cargo rust-lang/rust#103992

Merged

ehuss added this to the 1.67.0 milestone Nov 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only remove fingerprints and build script artifacts of the requested package #10621

Only remove fingerprints and build script artifacts of the requested package #10621

CosmicHorrorDev commented May 2, 2022 •

edited

rust-highfive commented May 2, 2022

CosmicHorrorDev commented May 2, 2022

weihanglo left a comment

CosmicHorrorDev commented May 25, 2022

weihanglo commented Sep 20, 2022

CosmicHorrorDev commented Sep 20, 2022

CosmicHorrorDev commented Oct 30, 2022

weihanglo commented Oct 30, 2022

CosmicHorrorDev commented Nov 1, 2022

weihanglo left a comment

weihanglo Nov 1, 2022

weihanglo commented Nov 1, 2022

bors commented Nov 1, 2022

bors commented Nov 1, 2022

bors commented Nov 1, 2022

jonhoo Nov 1, 2022

jonhoo Nov 1, 2022

CosmicHorrorDev Nov 1, 2022

Only remove fingerprints and build script artifacts of the requested package #10621

Only remove fingerprints and build script artifacts of the requested package #10621

Conversation

CosmicHorrorDev commented May 2, 2022 • edited

rust-highfive commented May 2, 2022

CosmicHorrorDev commented May 2, 2022

weihanglo left a comment

Choose a reason for hiding this comment

CosmicHorrorDev commented May 25, 2022

weihanglo commented Sep 20, 2022

CosmicHorrorDev commented Sep 20, 2022

CosmicHorrorDev commented Oct 30, 2022

weihanglo commented Oct 30, 2022

CosmicHorrorDev commented Nov 1, 2022

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo Nov 1, 2022

Choose a reason for hiding this comment

weihanglo commented Nov 1, 2022

bors commented Nov 1, 2022

bors commented Nov 1, 2022

bors commented Nov 1, 2022

jonhoo Nov 1, 2022

Choose a reason for hiding this comment

jonhoo Nov 1, 2022

Choose a reason for hiding this comment

CosmicHorrorDev Nov 1, 2022

Choose a reason for hiding this comment

CosmicHorrorDev commented May 2, 2022 •

edited