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

chore: get rid of remove_dir_all #542

Merged

Conversation

Rustin170506
Copy link
Collaborator

@Rustin170506 Rustin170506 commented Apr 9, 2024

close #539

I did two things:

  1. chore: cargo update -p rustix --precise 0.38.32
  2. chore: cargo update -p tempfile --precise 3.10.1

See GHSA-mc8h-8q98-g5hr

console on  rustin-patch-remove_dir-with-1.70.0 [$!] via 🦀 v1.78.0 took 21s cargo tree -i remove_dir_all
error: package ID specification `remove_dir_all` did not match any packages

@Rustin170506 Rustin170506 requested a review from a team as a code owner April 9, 2024 15:07
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

It would be nice to avoid the MSRV bump --- is the clap dependency update necessary?

console-api/Cargo.toml Outdated Show resolved Hide resolved
console-api/Cargo.toml Outdated Show resolved Hide resolved
@Rustin170506
Copy link
Collaborator Author

It would be nice to avoid the MSRV bump --- is the clap dependency update necessary?

As you can tell from the dependency tree:

cargo tree -i remove_dir_all
remove_dir_all v0.5.3
└── tempfile v3.3.0
    ├── prost-build v0.12.0
    │   └── tonic-build v0.10.0
    │       └── xtask v0.1.0 (/Users/joshka/local/tokio-console/xtask)
    │       [dev-dependencies]
    │       └── console-api v0.6.0 (/Users/joshka/local/tokio-console/console-api)
    │           ├── console-subscriber v0.2.0 (/Users/joshka/local/tokio-console/console-subscriber)
    │           └── tokio-console v0.1.10 (/Users/joshka/local/tokio-console/tokio-console)
    │   [dev-dependencies]
    │   └── console-api v0.6.0 (/Users/joshka/local/tokio-console/console-api) (*)
    └── snapbox v0.5.9
        └── trycmd v0.15.1
            [dev-dependencies]
            └── tokio-console v0.1.10 (/Users/joshka/local/tokio-console/tokio-console)

If we want to get rid of the remove_dir_all then we need to bump the tempfile. But if we try to bump it directly, we will get an error:

cargo update -p tempfile --precise 3.10.1
    Updating crates.io index
error: failed to select a version for `rustix`.
    ... required by package `tempfile v3.10.1`
    ... which satisfies dependency `tempfile = "^3"` of package `prost-build v0.12.0`
    ... which satisfies dependency `prost-build = "^0.12.0"` (locked to 0.12.0) of package `console-api v0.6.0 (/Volumes/t7/code/console/console-api)`
    ... which satisfies path dependency `console-api` (locked to 0.6.0) of package `console-subscriber v0.2.0 (/Volumes/t7/code/console/console-subscriber)`
versions that meet the requirements `^0.38.31` are: 0.38.32, 0.38.31

all possible versions conflict with previously selected packages.

  previously selected package `rustix v0.38.15`
    ... which satisfies dependency `rustix = "^0.38.0"` (locked to 0.38.15) of package `is-terminal v0.4.9`
    ... which satisfies dependency `is-terminal = "^0.4.1"` (locked to 0.4.9) of package `clap_builder v4.1.14`
    ... which satisfies dependency `clap_builder = "=4.1.14"` (locked to 4.1.14) of package `clap v4.1.14`
    ... which satisfies dependency `clap = "~4.1.14"` (locked to 4.1.14) of package `tokio-console v0.1.10 (/Volumes/t7/code/console/tokio-console)`

failed to select a version for `rustix` which could resolve this conflict

So before we bump the tempfile, we have to bump the clap to use the higher version rustix.

@Rustin170506 Rustin170506 force-pushed the rustin-patch-remove_dir-with-1.70.0 branch 2 times, most recently from ef9862d to 4c2e77f Compare April 10, 2024 13:14
Copy link
Collaborator 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 (PR reviewed by myself and ready for feedback.)

@Rustin170506 Rustin170506 requested review from hawkw and hds April 10, 2024 13:15
@Rustin170506 Rustin170506 force-pushed the rustin-patch-remove_dir-with-1.70.0 branch 2 times, most recently from b24975e to d566015 Compare May 16, 2024 13:10
Copy link
Collaborator 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 (PR reviewed by myself and ready for feedback.)

@@ -1500,15 +1507,6 @@ version = "0.7.5"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "dbb5fb1acd8a1a18b3dd5be62d25485eb770e05afb408a9627d14d451bae12da"

[[package]]
name = "remove_dir_all"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed!

@Rustin170506 Rustin170506 force-pushed the rustin-patch-remove_dir-with-1.70.0 branch 2 times, most recently from df5c1f4 to 160d0c0 Compare May 16, 2024 13:21
Copy link
Collaborator

@hds hds left a comment

Choose a reason for hiding this comment

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

There seem to be a lot of new transient dependencies added as part of this change.

Is it possible to make the change without doing this?

Or perhaps we could have a "chore: cargo update" PR first so that we don't get these things mixed up. What do you think?

@Rustin170506
Copy link
Collaborator Author

There seem to be a lot of new transient dependencies added as part of this change.

Is it possible to make the change without doing this?

You can refer to #542 (comment)

But I think we can split them into two PRs. But the change would be the same.

@Rustin170506
Copy link
Collaborator Author

But I think we can split them into two PRs. But the change would be the same.

Split: #552

@Rustin170506 Rustin170506 force-pushed the rustin-patch-remove_dir-with-1.70.0 branch 3 times, most recently from 5280649 to 72d95eb Compare May 16, 2024 13:42
@Rustin170506 Rustin170506 force-pushed the rustin-patch-remove_dir-with-1.70.0 branch from 72d95eb to 1bfb515 Compare May 16, 2024 13:45
@Rustin170506 Rustin170506 requested a review from hds May 16, 2024 13:46
Copy link
Collaborator 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 (PR reviewed by myself and ready for feedback.)

Copy link
Collaborator

@hds hds left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@Rustin170506 Rustin170506 merged commit 60bcf87 into tokio-rs:main May 16, 2024
17 checks passed
@Rustin170506 Rustin170506 deleted the rustin-patch-remove_dir-with-1.70.0 branch May 16, 2024 14:01
@Rustin170506
Copy link
Collaborator Author

Thanks for your review! 💚 💙 💜 💛 ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants