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: handle Windows path correctly #555

Merged
merged 5 commits into from
Jul 3, 2024

Conversation

Rustin170506
Copy link
Collaborator

@Rustin170506 Rustin170506 commented May 30, 2024

fix #550

We need to make truncate_registry_path can handle Windows path as well. Because tokio-console can connect to any server from different platforms, so we use the same path separator to have the same experience.

Some test snapshots:
local package:
image

crates.io package:
image

@Rustin170506 Rustin170506 requested a review from a team as a code owner May 30, 2024 12:41
@Rustin170506 Rustin170506 force-pushed the rustin-patch-path branch 3 times, most recently from b476d31 to ee7bedd Compare May 30, 2024 12:50
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 a review from hds May 30, 2024 12:53
@hds
Copy link
Collaborator

hds commented May 30, 2024

I haven't had a chance to look at the change properly yet (although what I have checked looks good). However I'm not sure what the image in the PR description is showing. Should the "local" one also show a crate path, rather than one from the application code?

@Rustin170506
Copy link
Collaborator Author

Should the "local" one also show a crate path, rather than one from the application code?

Because I ran the example from the console project directly. So I guess we have to show the application code. This path does not match the regex, so we directly show it.

tokio-console/src/state/mod.rs Outdated Show resolved Hide resolved
};

// This help use the same path separator on all platforms.
s.replace('\\', "/")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this is a good idea, it might cause issues if people have a / or \ in some directory name (although I'm not sure we should worry about that case really).

Also, while "C:/Users/user/projects/tokio-1.0.1/src/lib.rs" looks fine to me, maybe it looks weird to a Windows user?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, while "C:/Users/user/projects/tokio-1.0.1/src/lib.rs" looks fine to me, maybe it looks weird to a Windows user?

The reason I want to use the same path style is that users can use tokio-console to connect to servers from different servers. So maybe using the same path style would help us reduce confusion. But I also agree that it might cause some confusion for Windows users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure this is a good idea, it might cause issues if people have a / or \ in some directory name (although I'm not sure we should worry about that case really).

Yes. This is possible. At least, on macOS, I can create a dir that contains it by command: mkdir test\\a.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about:

  1. <cargo>/tokio-1.0.1/src/lib.rs for macOS and Linux path.
  2. <cargo>\tokio-1.0.1\src\lib.rs for Windows path.

But I am not sure if there is an easy way to detect if the path string is from the Windows OS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hds Do you have any thoughts on this? I am not sure which way we should go.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about:

1. `<cargo>/tokio-1.0.1/src/lib.rs` for macOS and Linux path.

2. `<cargo>\tokio-1.0.1\src\lib.rs` for Windows path.

But I am not sure if there is an easy way to detect if the path string is from the Windows OS.

Let's go with this. I would suggest we just go with what separator we see in the path. If it's / we leave it as /, if it's \ we leave it \. Naively you could probably count the number of each character in the string and only use \ (which I would consider less likely) if it is stricly higher.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used:

fn is_windows_path(path: &str) -> bool {
    use once_cell::sync::OnceCell;
    use regex::Regex;

    static REGEX: OnceCell<Regex> = OnceCell::new();
    let regex = REGEX.get_or_init(|| Regex::new(r"^[a-zA-Z]:\\").expect("failed to compile regex"));
    let has_drive_letter = regex.is_match(path);
    let slash_count = path.find('/').iter().count();
    let backslash_count = path.find('\\').iter().count();
    has_drive_letter && backslash_count > slash_count
}

It is not perfect, but it seems to work.

@Rustin170506
Copy link
Collaborator Author

Rustin170506 commented Jun 8, 2024

I am oncall this week and next week. I will update it when I get out of jail(oncall).

We need to make truncate_registry_path can
handle Windows path as well.
Because tokio-console can connect to any
server from different platforms,
so we use the same path separator to have same experience.

Signed-off-by: hi-rustin <rustin.liu@gmail.com>
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
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.

Looks good. Thank you!

@Rustin170506
Copy link
Collaborator Author

Tested locally again:
image

@Rustin170506 Rustin170506 merged commit 6ad0def into tokio-rs:main Jul 3, 2024
17 checks passed
@Rustin170506 Rustin170506 deleted the rustin-patch-path branch July 3, 2024 13:31
@github-actions github-actions bot mentioned this pull request Jul 3, 2024
@github-actions github-actions bot mentioned this pull request Jul 16, 2024
hds pushed a commit that referenced this pull request Jul 29, 2024
…onsole-v0.1.12

## 🤖 New release
* `tokio-console`: 0.1.11 -> 0.1.12
* `console-api`: 0.7.0 -> 0.8.0
* `console-subscriber`: 0.3.0 -> 0.4.0

## `tokio-console`

## 0.2.0 - (2024-07-29)

### Fixed

- Handle Windows path correctly ([#555](#555)) ([6ad0def](6ad0def))
- Avoid crash when accessing selected item ([#570](#570)) ([9205e15](9205e15))

### Updated

- Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))
</blockquote>

## `console-api`

## 0.8.0 - (2024-07-29)

### <a id = "0.8.0-breaking"></a>Breaking Changes
- **Upgrade tonic to 0.12 ([#571](#571 ([5f6faa2](https://github.com/tokio-rs/console/commit/5f6faa22d944735c2b8c312cac03b35a4ab228ef))<br />This is a breaking change for users of `console-api` and
`console-subscriber`, as it changes the public `tonic`, `prost` and
`prost-types` dependency to a semver-incompatible version. This breaks
compatibility with `tonic` 0.11.x as well as `prost`/`prost-types`
0.12.x.

### Updated

- [**breaking**](#0.8.0-breaking) Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))

## `console-subscriber`

## 0.4.0 - (2024-07-29)

### <a id = "0.4.0-breaking"></a>Breaking Changes
- **Upgrade tonic to 0.12 ([#571](#571 ([5f6faa2](https://github.com/tokio-rs/console/commit/5f6faa22d944735c2b8c312cac03b35a4ab228ef))<br />This is a breaking change for users of `console-api` and
`console-subscriber`, as it changes the public `tonic`, `prost` and
`prost-types` dependency to a semver-incompatible version. This breaks
compatibility with `tonic` 0.11.x as well as `prost`/`prost-types`
0.12.x.

### Added

- Add `TOKIO_CONSOLE_BUFFER_CAPACITY`  env variable ([#568](#568)) ([a6cf14b](a6cf14b))

### Updated

- [**breaking**](#0.4.0-breaking) Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))
hds pushed a commit that referenced this pull request Jul 29, 2024
…onsole-v0.1.12

## 🤖 New release
* `tokio-console`: 0.1.11 -> 0.1.12
* `console-api`: 0.7.0 -> 0.8.0
* `console-subscriber`: 0.3.0 -> 0.4.0

## `tokio-console`

## 0.2.0 - (2024-07-29)

### Fixed

- Handle Windows path correctly ([#555](#555)) ([6ad0def](6ad0def))
- Avoid crash when accessing selected item ([#570](#570)) ([9205e15](9205e15))

### Updated

- Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))

## `console-api`

## 0.8.0 - (2024-07-29)

### <a id = "0.8.0-breaking"></a>Breaking Changes
- **Upgrade tonic to 0.12 ([#571](#571 ([5f6faa2](https://github.com/tokio-rs/console/commit/5f6faa22d944735c2b8c312cac03b35a4ab228ef))<br />This is a breaking change for users of `console-api` and
`console-subscriber`, as it changes the public `tonic`, `prost` and
`prost-types` dependency to a semver-incompatible version. This breaks
compatibility with `tonic` 0.11.x as well as `prost`/`prost-types`
0.12.x.

### Updated

- [**breaking**](#0.8.0-breaking) Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))

## `console-subscriber`

## 0.4.0 - (2024-07-29)

### <a id = "0.4.0-breaking"></a>Breaking Changes
- **Upgrade tonic to 0.12 ([#571](#571 ([5f6faa2](https://github.com/tokio-rs/console/commit/5f6faa22d944735c2b8c312cac03b35a4ab228ef))<br />This is a breaking change for users of `console-api` and
`console-subscriber`, as it changes the public `tonic`, `prost` and
`prost-types` dependency to a semver-incompatible version. This breaks
compatibility with `tonic` 0.11.x as well as `prost`/`prost-types`
0.12.x.

### Added

- Add `TOKIO_CONSOLE_BUFFER_CAPACITY`  env variable ([#568](#568)) ([a6cf14b](a6cf14b))

### Fixed

- Remove unused `AggregatorHandle` and fix other lints ([#578](#578)) ([c442063](c442063))

### Updated

- [**breaking**](#0.4.0-breaking) Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))
hds pushed a commit that referenced this pull request Jul 29, 2024
…onsole-v0.1.12

## 🤖 New release
* `tokio-console`: 0.1.11 -> 0.1.12
* `console-api`: 0.7.0 -> 0.8.0
* `console-subscriber`: 0.3.0 -> 0.4.0

## `tokio-console`

## 0.1.12 - (2024-07-29)

### Fixed

- Handle Windows path correctly ([#555](#555)) ([6ad0def](6ad0def))
- Avoid crash when accessing selected item ([#570](#570)) ([9205e15](9205e15))

### Updated

- Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))

## `console-api`

## 0.8.0 - (2024-07-29)

### <a id = "0.8.0-breaking"></a>Breaking Changes
- **Upgrade tonic to 0.12 ([#571](#571 ([5f6faa2](https://github.com/tokio-rs/console/commit/5f6faa22d944735c2b8c312cac03b35a4ab228ef))<br />This is a breaking change for users of `console-api` and
`console-subscriber`, as it changes the public `tonic`, `prost` and
`prost-types` dependency to a semver-incompatible version. This breaks
compatibility with `tonic` 0.11.x as well as `prost`/`prost-types`
0.12.x.

### Updated

- [**breaking**](#0.8.0-breaking) Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))

## `console-subscriber`

## 0.4.0 - (2024-07-29)

### <a id = "0.4.0-breaking"></a>Breaking Changes
- **Upgrade tonic to 0.12 ([#571](#571 ([5f6faa2](https://github.com/tokio-rs/console/commit/5f6faa22d944735c2b8c312cac03b35a4ab228ef))<br />This is a breaking change for users of `console-api` and
`console-subscriber`, as it changes the public `tonic`, `prost` and
`prost-types` dependency to a semver-incompatible version. This breaks
compatibility with `tonic` 0.11.x as well as `prost`/`prost-types`
0.12.x.

### Added

- Add `TOKIO_CONSOLE_BUFFER_CAPACITY`  env variable ([#568](#568)) ([a6cf14b](a6cf14b))

### Fixed

- Remove unused `AggregatorHandle` and fix other lints ([#578](#578)) ([c442063](c442063))

### Updated

- [**breaking**](#0.4.0-breaking) Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))
hds pushed a commit that referenced this pull request Jul 29, 2024
…onsole-v0.1.12

## 🤖 New release
* `tokio-console`: 0.1.11 -> 0.1.12
* `console-api`: 0.7.0 -> 0.8.0
* `console-subscriber`: 0.3.0 -> 0.4.0

## `tokio-console`

## 0.1.12 - (2024-07-29)

### Fixed

- Handle Windows path correctly ([#555](#555)) ([6ad0def](6ad0def))
- Avoid crash when accessing selected item ([#570](#570)) ([9205e15](9205e15))

### Updated

- Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))

## `console-api`

## 0.8.0 - (2024-07-29)

### <a id = "0.8.0-breaking"></a>Breaking Changes
- **Upgrade tonic to 0.12 ([#571](#571 ([5f6faa2](https://github.com/tokio-rs/console/commit/5f6faa22d944735c2b8c312cac03b35a4ab228ef))<br />This is a breaking change for users of `console-api` and
`console-subscriber`, as it changes the public `tonic`, `prost` and
`prost-types` dependency to a semver-incompatible version. This breaks
compatibility with `tonic` 0.11.x as well as `prost`/`prost-types`
0.12.x.

### Updated

- [**breaking**](#0.8.0-breaking) Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))

## `console-subscriber`

## 0.4.0 - (2024-07-29)

### <a id = "0.4.0-breaking"></a>Breaking Changes
- **Upgrade tonic to 0.12 ([#571](#571 ([5f6faa2](https://github.com/tokio-rs/console/commit/5f6faa22d944735c2b8c312cac03b35a4ab228ef))<br />This is a breaking change for users of `console-api` and
`console-subscriber`, as it changes the public `tonic`, `prost` and
`prost-types` dependency to a semver-incompatible version. This breaks
compatibility with `tonic` 0.11.x as well as `prost`/`prost-types`
0.12.x.

### Added

- Add `TOKIO_CONSOLE_BUFFER_CAPACITY`  env variable ([#568](#568)) ([a6cf14b](a6cf14b))

### Fixed

- Remove unused `AggregatorHandle` and fix other lints ([#578](#578)) ([c442063](c442063))

### Updated

- [**breaking**](#0.4.0-breaking) Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))
hds pushed a commit that referenced this pull request Jul 29, 2024
…onsole-v0.1.12

## 🤖 New release
* `tokio-console`: 0.1.11 -> 0.1.12
* `console-api`: 0.7.0 -> 0.8.0
* `console-subscriber`: 0.3.0 -> 0.4.0

## `tokio-console`

## 0.1.12 - (2024-07-29)

### Fixed

- Handle Windows path correctly ([#555](#555)) ([6ad0def](6ad0def))
- Avoid crash when accessing selected item ([#570](#570)) ([9205e15](9205e15))

### Updated

- Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))

## `console-api`

## 0.8.0 - (2024-07-29)

### <a id = "0.8.0-breaking"></a>Breaking Changes
- **Upgrade tonic to 0.12 ([#571](#571 ([5f6faa2](5f6faa2))
This is a breaking change for users of `console-api` and
`console-subscriber`, as it changes the public `tonic`, `prost` and
`prost-types` dependency to a semver-incompatible version. This breaks
compatibility with `tonic` 0.11.x as well as `prost`/`prost-types`
0.12.x.

### Updated

- [**breaking**](#0.8.0-breaking) Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))

## `console-subscriber`

## 0.4.0 - (2024-07-29)

### <a id = "0.4.0-breaking"></a>Breaking Changes
- **Upgrade tonic to 0.12 ([#571](#571 ([5f6faa2](5f6faa2))
This is a breaking change for users of `console-api` and
`console-subscriber`, as it changes the public `tonic`, `prost` and
`prost-types` dependency to a semver-incompatible version. This breaks
compatibility with `tonic` 0.11.x as well as `prost`/`prost-types`
0.12.x.

### Added

- Add `TOKIO_CONSOLE_BUFFER_CAPACITY`  env variable ([#568](#568)) ([a6cf14b](a6cf14b))

### Fixed

- Remove unused `AggregatorHandle` and fix other lints ([#578](#578)) ([c442063](c442063))

### Updated

- [**breaking**](#0.4.0-breaking) Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))
hds pushed a commit that referenced this pull request Jul 29, 2024
…onsole-v0.1.12 (#576)

## 🤖 New release
* `tokio-console`: 0.1.11 -> 0.1.12
* `console-api`: 0.7.0 -> 0.8.0
* `console-subscriber`: 0.3.0 -> 0.4.0

## `tokio-console`

## 0.1.12 - (2024-07-29)

### Fixed

- Handle Windows path correctly ([#555](#555)) ([6ad0def](6ad0def))
- Avoid crash when accessing selected item ([#570](#570)) ([9205e15](9205e15))

### Updated

- Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))

## `console-api`

## 0.8.0 - (2024-07-29)

### <a id = "0.8.0-breaking"></a>Breaking Changes
- **Upgrade tonic to 0.12 ([#571](#571 ([5f6faa2](5f6faa2))
This is a breaking change for users of `console-api` and
`console-subscriber`, as it changes the public `tonic`, `prost` and
`prost-types` dependency to a semver-incompatible version. This breaks
compatibility with `tonic` 0.11.x as well as `prost`/`prost-types`
0.12.x.

### Updated

- [**breaking**](#0.8.0-breaking) Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))

## `console-subscriber`

## 0.4.0 - (2024-07-29)

### <a id = "0.4.0-breaking"></a>Breaking Changes
- **Upgrade tonic to 0.12 ([#571](#571 ([5f6faa2](5f6faa2))
This is a breaking change for users of `console-api` and
`console-subscriber`, as it changes the public `tonic`, `prost` and
`prost-types` dependency to a semver-incompatible version. This breaks
compatibility with `tonic` 0.11.x as well as `prost`/`prost-types`
0.12.x.

### Added

- Add `TOKIO_CONSOLE_BUFFER_CAPACITY`  env variable ([#568](#568)) ([a6cf14b](a6cf14b))

### Fixed

- Remove unused `AggregatorHandle` and fix other lints ([#578](#578)) ([c442063](c442063))

### Updated

- [**breaking**](#0.4.0-breaking) Upgrade tonic to 0.12 ([#571](#571)) ([5f6faa2](5f6faa2))

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

tokio-console cannot handle window path correctly
2 participants