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

Migrating to windows-sys #718

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

WanderLanz
Copy link

@WanderLanz WanderLanz commented Oct 3, 2022

Attempting at move windows-sys for win32 api.

@WanderLanz WanderLanz marked this pull request as draft October 3, 2022 22:24
@WanderLanz WanderLanz closed this Nov 9, 2022
@sigmaSd
Copy link
Contributor

sigmaSd commented Nov 9, 2022

I think you should keep this open, it's a reasonable improvement

@WanderLanz
Copy link
Author

WanderLanz commented Nov 11, 2022

I think you should keep this open, it's a reasonable improvement

I've just gotten too busy to keep track of this pr and didn't see anyone saying anything about moving from winapi, besides #662 for compile times, in addition to needing the crossterm-winapi pr in order to make the build work (without breaking changes).

I'll go ahead and reopen it and the crossterm-winapi pr for review, but I might not be responsive.
I've done most of the (albeit easy) work so anyone else is more than welcome to make another pr if this one becomes stale or breaking.

@WanderLanz WanderLanz reopened this Nov 11, 2022
@WanderLanz WanderLanz marked this pull request as ready for review November 11, 2022 02:57
@TimonPost
Copy link
Member

TimonPost commented Jan 29, 2023

Thanks for PR, just haven't gotten to it yet as I'm not sure about the benefits yet. The only reason I can think of is that winapi is not actively maintained, and windows-rs is owned by Microsoft. But winapi works perfectly and we do not have the need to switch really. So kinda low priority-wise to get this in. Especially this change needs to be well tested. Altough

@WanderLanz
Copy link
Author

@TimonPost No problem at all. windows-rs is not even technically 1.0 yet and we need to make changes in crossterm-winapi as well, so it's maybe not worth the change just for minimal improvements.

I just figured that it might help in keeping everything up to date with any new API, e.g. ConPTY, allowing the awesome contributors here to provide more features down the road and keep everything working well.

Also, if winapi is unmaintained for the foreseeable future, this issue and others might creep up on us.

Hope everyone's having a good day, remember to take frequent breaks.

@jasiukiewicztymon
Copy link

@TimonPost can you review this maybe it would help for #772 issue and resolve this problem.. Thanks!

@WanderLanz
Copy link
Author

Hey I've been busy on other things so I can't maintain this to current. It probably won't happen until @TimonPost makes a concerted effort themselves, but it's easy enough for someone to do again within a day.

I'll leave it open just as a reminder that winapi is, at least slowly, becoming deprecated, its last commit was three years ago, and migration to windows-sys is requested, but feel free to close anytime.

  • Posture check!

@kirawi
Copy link

kirawi commented Jan 8, 2024

Thanks for PR, just haven't gotten to it yet as I'm not sure about the benefits yet. The only reason I can think of is that winapi is not actively maintained, and windows-rs is owned by Microsoft. But winapi works perfectly and we do not have the need to switch really. So kinda low priority-wise to get this in. Especially this change needs to be well tested. Altough

I do want to add that it helps out dependents since a lot of newer crates and some older crates use windows instead. So having both winapi and windows in the dependency tree is not ideal.

@TimonPost
Copy link
Member

TimonPost commented Jan 21, 2024

Agreed! If its something the ecosystem works towards we can make this effort. If someone feels like it it can be rebased. Try run the interactive demo in the examples. Mainly testing the features that are essentially different in windows vs unix. resizing, cursor position, events etc...

@WanderLanz
Copy link
Author

I found some time, so I went ahead and just forced a rebase here. quick tests seem to pass. ...except resizing, and I don't know why, so if anyone wants to take a shot at it go ahead.

Just to note, async-std has upgraded to windows-sys (via async-io), but no release yet so that will still pull in winapi for now.

crossterm-winapi still needs update

@joshka
Copy link
Collaborator

joshka commented Jun 24, 2024

It looks like everything except for socket2 now uses windows-sys/windows-target (part of the same dependency tree). The latest version of socket2 also uses windows-sys. Moving away from winapi is definitely be the right path forward here.

Inverse dependency trees from cargo tree:

winapi v0.3.9
├── crossterm v0.27.0 (/Users/joshka/local/crossterm)
├── crossterm_winapi v0.9.1
│   └── crossterm v0.27.0 (/Users/joshka/local/crossterm)
├── filedescriptor v0.8.2
│   └── crossterm v0.27.0 (/Users/joshka/local/crossterm)
└── socket2 v0.4.10
    └── async-io v1.13.0
        └── async-std v1.12.0
            [dev-dependencies]
            └── crossterm v0.27.0 (/Users/joshka/local/crossterm)
windows-sys v0.48.0
├── io-lifetimes v1.0.11
│   └── rustix v0.37.27
│       └── async-io v1.13.0
│           └── async-std v1.12.0
│               [dev-dependencies]
│               └── crossterm v0.27.0 (/Users/joshka/local/crossterm)
├── mio v0.8.11
│   ├── crossterm v0.27.0 (/Users/joshka/local/crossterm)
│   ├── signal-hook-mio v0.2.3
│   │   └── crossterm v0.27.0 (/Users/joshka/local/crossterm)
│   └── tokio v1.38.0
│       [dev-dependencies]
│       └── crossterm v0.27.0 (/Users/joshka/local/crossterm)
├── polling v2.8.0
│   └── async-io v1.13.0 (*)
├── rustix v0.37.27 (*)
└── tokio v1.38.0 (*)
windows-sys v0.52.0
├── async-io v2.3.3
│   └── async-global-executor v2.4.1
│       └── async-std v1.12.0
│           [dev-dependencies]
│           └── crossterm v0.27.0 (/Users/joshka/local/crossterm)
├── errno v0.3.9
│   ├── rustix v0.37.27
│   │   └── async-io v1.13.0
│   │       └── async-std v1.12.0 (*)
│   └── rustix v0.38.34
│       ├── async-io v2.3.3 (*)
│       ├── crossterm v0.27.0 (/Users/joshka/local/crossterm)
│       └── polling v3.7.2
│           └── async-io v2.3.3 (*)
├── polling v3.7.2 (*)
├── rustix v0.38.34 (*)
└── socket2 v0.5.7
    └── tokio v1.38.0
        [dev-dependencies]
        └── crossterm v0.27.0 (/Users/joshka/local/crossterm)
windows-targets v0.48.5
└── windows-sys v0.48.0
    ├── io-lifetimes v1.0.11
    │   └── rustix v0.37.27
    │       └── async-io v1.13.0
    │           └── async-std v1.12.0
    │               [dev-dependencies]
    │               └── crossterm v0.27.0 (/Users/joshka/local/crossterm)
    ├── mio v0.8.11
    │   ├── crossterm v0.27.0 (/Users/joshka/local/crossterm)
    │   ├── signal-hook-mio v0.2.3
    │   │   └── crossterm v0.27.0 (/Users/joshka/local/crossterm)
    │   └── tokio v1.38.0
    │       [dev-dependencies]
    │       └── crossterm v0.27.0 (/Users/joshka/local/crossterm)
    ├── polling v2.8.0
    │   └── async-io v1.13.0 (*)
    ├── rustix v0.37.27 (*)
    └── tokio v1.38.0 (*)
windows-targets v0.52.5
├── parking_lot_core v0.9.10
│   ├── dashmap v5.5.3
│   │   └── serial_test v2.0.0
│   │       [dev-dependencies]
│   │       └── crossterm v0.27.0 (/Users/joshka/local/crossterm)
│   └── parking_lot v0.12.3
│       ├── crossterm v0.27.0 (/Users/joshka/local/crossterm)
│       ├── serial_test v2.0.0 (*)
│       └── tokio v1.38.0
│           [dev-dependencies]
│           └── crossterm v0.27.0 (/Users/joshka/local/crossterm)
└── windows-sys v0.52.0
    ├── async-io v2.3.3
    │   └── async-global-executor v2.4.1
    │       └── async-std v1.12.0
    │           [dev-dependencies]
    │           └── crossterm v0.27.0 (/Users/joshka/local/crossterm)
    ├── errno v0.3.9
    │   ├── rustix v0.37.27
    │   │   └── async-io v1.13.0
    │   │       └── async-std v1.12.0 (*)
    │   └── rustix v0.38.34
    │       ├── async-io v2.3.3 (*)
    │       ├── crossterm v0.27.0 (/Users/joshka/local/crossterm)
    │       └── polling v3.7.2
    │           └── async-io v2.3.3 (*)
    ├── polling v3.7.2 (*)
    ├── rustix v0.38.34 (*)
    └── socket2 v0.5.7
        └── tokio v1.38.0 (*)

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.

6 participants