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(deps): bump ratatui to 0.26.2 and crossterm to 0.27.0 #515

Merged
merged 3 commits into from
May 16, 2024

Conversation

joshka
Copy link
Contributor

@joshka joshka commented Feb 3, 2024

Bumps MSRV to 1.74.0

This necessitated 3 changes to the codebase:

  • Spans was renamed to the more ergonomic Line type.
  • Frame no longer requires a backend type parameter.
  • Table::new requires a widths parameter, so we use
    Table::default().rows(rows) instead of Table::new(rows).

Crossterm on Windows triggers events for key press as well as release
and repeat, which causes duplicate key presses. This change filters out
those events.

@joshka joshka requested a review from a team as a code owner February 3, 2024 00:45
@hawkw
Copy link
Member

hawkw commented Feb 6, 2024

It looks like this change requires a MSRV bump, as the new ratatui seems not to support Rust 1.64.0: https://github.com/tokio-rs/console/actions/runs/7763271246/job/21175134601#step:8:1

@joshka
Copy link
Contributor Author

joshka commented Feb 6, 2024

It looks like this change requires a MSRV bump, as the new ratatui seems not to support Rust 1.64.0: https://github.com/tokio-rs/console/actions/runs/7763271246/job/21175134601#step:8:1

Thanks - bumped this to 1.70

@hawkw
Copy link
Member

hawkw commented Feb 8, 2024

@joshka mind rebasing this branch onto the latest master and resolving the lockfile conflicts? Once that's done, this should be good to merge!

@joshka joshka changed the title chore(deps): update ratatui to 0.26.0 chore(deps): update ratatui to 0.26.1 Feb 15, 2024
@joshka
Copy link
Contributor Author

joshka commented Feb 15, 2024

1:31 PM]joshka: I notice that some tests are failing now - digging to check what's happening
[1:31 PM]Hayden (he/him): if it's the self wakes test then it's known flaky. 
[1:31 PM]Hayden (he/him): (I need to remove it)
[1:37 PM]joshka: Ah got it - yep it was that. I did't a quick cargo update on main and saw the tests fail there too (it didn't fail before then)

@joshka
Copy link
Contributor Author

joshka commented Apr 1, 2024

Squashed and rebased

@joshka joshka requested a review from hawkw April 4, 2024 04:24
Copy link
Collaborator

@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.

Regarding the failed test, it is failing consistently. Maybe it is not an unstable test. More like we updated some dependencies causing a change in behaviour?
Thanks for your contribution! 🤟 🖤

Cargo.lock Outdated Show resolved Hide resolved
@Rustin170506
Copy link
Collaborator

There are some proto changes:

/// Each `ResourceUpdate` contains any resource data that has changed since the last
diff --git a/console-api/src/generated/rs.tokio.console.tasks.rs b/console-api/src/generated/rs.tokio.console.tasks.rs
index 41b8396..e8ee5fa 100644
--- a/console-api/src/generated/rs.tokio.console.tasks.rs
+++ b/console-api/src/generated/rs.tokio.console.tasks.rs
@@ -1,3 +1,4 @@
+// This file is @generated by prost-build.
 /// A task state update.
 ///
 /// Each `TaskUpdate` contains any task data that has changed since the last
diff --git a/console-api/src/generated/rs.tokio.console.trace.rs b/console-api/src/generated/rs.tokio.console.trace.rs
index [220](https://github.com/tokio-rs/console/actions/runs/8603263859/job/23574759021?pr=515#step:6:221)b55a..be3516e 100644
--- a/console-api/src/generated/rs.tokio.console.trace.rs
+++ b/console-api/src/generated/rs.tokio.console.trace.rs
@@ -1,3 +1,4 @@
+// This file is @generated by prost-build.
 /// Start watching trace events with the provided filter.
 #[allow(clippy::derive_partial_eq_without_eq)]
 #[derive(Clone, PartialEq, ::prost::Message)]
test bootstrap ... FAILED

Because there are some other issues in this PR, I would recommend:

  1. This PR only focuses on bumping the rataui version.
  2. If the rataui requires a higher MSRV, we bump the MSRV but do not change other dependencies' versions.

Perhaps we could address the issue of the failed test that was caused by changes in other dependencies.

Also, this would help us make sure the scope of this PR is not extended. We can only focus on bumping the rataui version.

Thank you for your patch again!

hawkw
hawkw previously requested changes Apr 10, 2024
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.

I've noticed a large number of unrelated formatting changes. I would really prefer not to merge those as part of this PR, because then, when viewing the Git history, those lines will have been changed as part of the Ratatui update, when it was not actually part of that change. I'm happy to accept formatting changes if they are considered better or more idiomatic style, but please make those changes in a separate branch.

If you're manually reformatting TOML as part of this PR, can you please put it back the way it was? Or, if your editor is doing this automatically, can you please disable that setting for this repo? Thank you!

tokio-console/Cargo.toml Outdated Show resolved Hide resolved
tokio-console/README.md Outdated Show resolved Hide resolved
tokio-console/README.md Outdated Show resolved Hide resolved
tokio-console/README.md Outdated Show resolved Hide resolved
tokio-console/README.md Outdated Show resolved Hide resolved
xtask/Cargo.toml Outdated Show resolved Hide resolved
xtask/Cargo.toml Outdated Show resolved Hide resolved
console-subscriber/README.md Outdated Show resolved Hide resolved
console-subscriber/Cargo.toml Outdated Show resolved Hide resolved
@joshka joshka force-pushed the dev branch 2 times, most recently from 3127d33 to 4413a62 Compare April 25, 2024 20:19
@joshka
Copy link
Contributor Author

joshka commented Apr 25, 2024

I've fixed the PR to just focus on the Ratatui changes (and bumped it to 0.26.2 which was released after this PR was created). I've also bumped crossterm to 0.27.0 to avoid incompatible versions, and fixed a small bug which hits most crossterm based apps on Windows (duplicate key presses).

I've bumped all the projects to MSRV 1.74 as this is the version that we build with in Ratatui. I'm not sure if it was right to just bump the tokio-console project or all the files, but this seems to be the least likely to cause CI pain (e.g. building the libs with 1.64 and the tui app with 1.74).

@joshka joshka changed the title chore(deps): update ratatui to 0.26.1 chore(deps): bump ratatui to 0.26.2 and crossterm to 0.27.0 Apr 25, 2024
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.

Thank you so much for following through with this change @joshka!

This looks good to me now.

@hawkw Your change request will be blocking this change from being merged now. Would you mind having another look.

Copy link
Collaborator

@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.

The rest looks good to me.

Thank you very much for working on this!

@@ -95,6 +95,11 @@ async fn main() -> color_eyre::Result<()> {
let input = input
.ok_or_else(|| eyre!("keyboard input stream ended early"))
.with_section(|| "this is probably a bug".header("Note:"))??;

if input::should_ignore_key_event(&input) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

let mut input = Box::pin(input::EventStream::new().try_filter(|event| {

We already have a filter here to ignore the release event. Maybe we should consider merging them together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed seeing that, fixing it by just using the should_ignore_key_event instead of the filter. Either way would be fine I guess.

Copy link
Collaborator

@Rustin170506 Rustin170506 May 8, 2024

Choose a reason for hiding this comment

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

nit: @hds @hawkw Do you have any thoughts about which one is better? It seems both work well. The only difference is that the try_filter will filter it asynchronously.

Copy link
Collaborator

@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.

Looks good to me. Thanks! 👍

@Rustin170506
Copy link
Collaborator

Lint CI fixed in #548

@Rustin170506
Copy link
Collaborator

@joshka There are some unused imports after your change. Could you please help to remove them? Thanks!

@joshka
Copy link
Contributor Author

joshka commented May 9, 2024

@joshka There are some unused imports after your change. Could you please help to remove them? Thanks!

Done

@hds
Copy link
Collaborator

hds commented May 9, 2024

@hawkw Could you please rereview and approve (if you're happy with it) this PR? Your request for changes is blocking at the moment. Thanks!

Bumps MSRV to 1.74.0

This necessitated 3 changes to the codebase:
- `Spans` was renamed to the more ergonomic `Line` type.
- `Frame` no longer requires a backend type parameter.
- `Table::new` requires a widths parameter, so we use
  `Table::default().rows(rows)` instead of `Table::new(rows)`.

Crossterm on Windows triggers events for key press as well as release
and repeat, which causes duplicate key presses. This change filters out
those events.
@hds hds dismissed hawkw’s stale review May 16, 2024 08:10

The changes that Eliza requested have been made, and she also previously approved the PR.

@hds hds merged commit 6cbd6db into tokio-rs:main May 16, 2024
17 checks passed
@joshka joshka deleted the dev branch May 19, 2024 04:43
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.

4 participants