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

deps: Move from winapi to windows #950

Merged

Conversation

poliorcetics
Copy link
Contributor

winapi is in maintenance mode and the new blessed way to access Windows APIs are the windows
and windows-sys crates. I was forced to use windows because sysinfo uses the COM APIs
which are not present in windows-sys.

@poliorcetics
Copy link
Contributor Author

Currently blocked by microsoft/windows-rs#2370 because ULARGE_INTEGER is not available yet and I didn't want to make it myself here. I could though ?

Also, it should be noted ntapi still uses winapi behind the scene. I plan to work on that but it will be much harder than this since ntapi exposes structures with members that are marked as Reserved in windows-rs.

There is microsoft/windows-rs#2358 (comment) which could probably help, and it's now possible to build crates above windows-rs using the metadata and generator, I'll investigate that too

@riverar
Copy link

riverar commented Mar 9, 2023

Can you point to what's blocking you?

ULARGE_INTEGER => u64
LARGE_INTEGER => i64

@GuillaumeGomez
Copy link
Owner

Currently blocked by microsoft/windows-rs#2370 because ULARGE_INTEGER is not available yet and I didn't want to make it myself here. I could though ?

I'd rather wait for the alias to be available in the dependency rather than adding it in sysinfo.

Overall, it seems to go into the right direction, but windows doesn't seem ready yet.

@riverar
Copy link

riverar commented Mar 9, 2023

@GuillaumeGomez Don't think we have any plans to add that alias. It's not needed as the PR demonstrates?

@GuillaumeGomez
Copy link
Owner

If it's used by the windows API, then it's needed, even if it's just an alias. Just like libc provides aliases for int64_t and other sized types, it's not because "we can't do without them" but rather to make it much easier for maintainers to compare between their code and the API.

@poliorcetics poliorcetics marked this pull request as ready for review March 10, 2023 08:18
@poliorcetics
Copy link
Contributor Author

error: package windows v0.44.0 cannot be built because it requires rustc 1.64 or newer, while the currently active rustc version is 1.59.0

Ah, that's sad.

@poliorcetics poliorcetics marked this pull request as draft March 10, 2023 09:03
@GuillaumeGomez
Copy link
Owner

We can update the minimum required version. It'll just require us to change sysinfo major version number.

@poliorcetics
Copy link
Contributor Author

We can update the minimum required version. It'll just require us to change sysinfo major version number.

What do you prefer ? Doing it in this MR (in new commits) or in a separate one ?

@GuillaumeGomez
Copy link
Owner

In this one, otherwise CI will always fail. Just put it into its own commit.

@kennykerr
Copy link

error: package windows v0.44.0 cannot be built because it requires rustc 1.64 or newer, while the currently active rustc version is 1.59.0

Ah, that's sad.

The latest version of the windows crate supports Rust 1.48.

https://kennykerr.ca/rust-getting-started/windows-or-windows-sys.html

src/windows/bindings.rs Outdated Show resolved Hide resolved
@poliorcetics
Copy link
Contributor Author

running 27 tests
test common::tests::check_display_impl_mac_address ... ok
test common::tests::check_display_impl_process_status ... ok
test common::tests::check_mac_address_is_unspecified_false ... ok
test common::tests::check_mac_address_is_unspecified_true ... ok
test system::tests::check_hostname_has_no_nuls ... ok
test common::tests::check_uid_gid_from_impls ... ok
test system::tests::check_if_send_and_sync ... ok
test system::tests::test_consecutive_cpu_usage_update ... ignored
test system::tests::test_get_process ... ok
test system::tests::test_refresh_process ... ok
test system::tests::test_refresh_system ... ok
test system::tests::check_uptime ... ok
error: test failed, to rerun pass `--lib`

Caused by:
  process didn't exit successfully: `D:\a\sysinfo\sysinfo\target\debug\deps\sysinfo-3e0191f40579cb26.exe` (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)
note: test exited abnormally; to see the full output pass --nocapture to the harness.

I'm not managing to reproduce locally, makes searching for the bug much more annoying than it should be

@GuillaumeGomez
Copy link
Owner

Try to update CI script for windows to run with RUST_BACKTRACE=full and with --test-threads 1 maybe?

@poliorcetics poliorcetics marked this pull request as ready for review September 25, 2023 03:54
@poliorcetics
Copy link
Contributor Author

Rebased and green CI, should be good to go 🎉

src/windows/cpu.rs Outdated Show resolved Hide resolved
src/windows/cpu.rs Outdated Show resolved Hide resolved
src/windows/sid.rs Outdated Show resolved Hide resolved
src/windows/sid.rs Outdated Show resolved Hide resolved
src/windows/sid.rs Outdated Show resolved Hide resolved
Copy link
Owner

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

Apart from the few code style nits, it looks great, thanks for working on this!

`winapi` is in maintenance mode and the new blessed way to access Windows APIs are the `windows`
and `windows-sys` crates. I was forced to use `windows` because `sysinfo` uses the COM APIs
which are not present in `windows-sys`.
@poliorcetics
Copy link
Contributor Author

All fixed :)

@GuillaumeGomez
Copy link
Owner

Looks all good to me, thanks a lot!

@GuillaumeGomez GuillaumeGomez merged commit a9f8b65 into GuillaumeGomez:master Sep 25, 2023
67 checks passed
@poliorcetics poliorcetics deleted the winapi-to-windows-sys branch September 25, 2023 18:14
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.

None yet

4 participants