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

Upgrade to windows-sys crate #20

Merged
merged 11 commits into from
Jul 18, 2024
Merged

Conversation

ssrlive
Copy link
Contributor

@ssrlive ssrlive commented May 31, 2024

No description provided.

Copy link
Collaborator

@TroyNeubauer TroyNeubauer left a comment

Choose a reason for hiding this comment

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

Thanks for this! Some minor comments

src/util.rs Show resolved Hide resolved
src/util.rs Show resolved Hide resolved
.github/workflows/main.yml Show resolved Hide resolved
@thomaseizinger
Copy link

Thank you for updating this dependency! Is there something one can do to move this forward?

@TroyNeubauer
Copy link
Collaborator

I'll jump in and get this through

@ssrlive
Copy link
Contributor Author

ssrlive commented Jul 13, 2024

Hi @TroyNeubauer , since you lack the time and mind to maintain this project, could you add me as a co-contributor and publisher to this project? Or transfer to me?

@TroyNeubauer
Copy link
Collaborator

Open to do that at some point in the future. For the purposes of this PR, I would prefer you to respond to my feedback and then we can merge and release as usual

@ssrlive ssrlive changed the title Upgrade windows crate Upgrade to windows-sys crate Jul 16, 2024
@ssrlive
Copy link
Contributor Author

ssrlive commented Jul 16, 2024

Hi @TroyNeubauer, I have switched to windows-sys crate for stability reason and fixed a bug about SetInterfaceDnsSettings function.

Copy link
Collaborator

@TroyNeubauer TroyNeubauer left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this. Almost there!

I still have some open questions about removing logging and memory safety.

What are the specific reasons for switching to windows-sys? I see this as a significant readability decrease in our code since we have to handle low-level pointer details that the windows crate previously handled for us

src/util.rs Outdated Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
src/util.rs Show resolved Hide resolved
src/util.rs Show resolved Hide resolved
src/util.rs Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
@ssrlive
Copy link
Contributor Author

ssrlive commented Jul 18, 2024

Because each version of the windows crate makes completely incompatible changes to the API provided to users,
this forces users to spend a considerable amount of effort on unnecessary code modifications to adapt to the new version.

For example, the definition of HANDLE, has changed from

pub struct HANDLE(pub isize);

to

pub struct HANDLE(pub *mut core::ffi::c_void);

This is completely unacceptable.

The windows-sys crate, on the other hand, simply translates C-style APIs directly into Rust language, with no changing data structure definitions. Therefore, using it greatly benefits code stability.

What are the specific reasons for switching to windows-sys? I see this as a significant readability decrease in our code since we have to handle low-level pointer details that the windows crate previously handled for us

@thomaseizinger
Copy link

The windows-sys crate, on the other hand, simply translates C-style APIs directly into Rust language, with no changing data structure definitions. Therefore, using it greatly benefits code stability.

It still tracks the same version number though, right? i.e. windows-sys bumps its minor version in lockstep with windows (or the other way round I guess). You'd expect a -sys crate to be more stable but unfortunately, that doesn't seem to be the case with the Windows one.

@ssrlive
Copy link
Contributor Author

ssrlive commented Jul 18, 2024

Since unexpected issues might occur regardless of which crate we use, let's choose a crate that appears relatively reliable. Right? @thomaseizinger

@thomaseizinger
Copy link

My primary interest in this PR is to reduce the number of different versions of windows and windows-sys in our dependency tree.

I just realised that windows-sys indeed has less version churn! It just updates to the matching windows version when it does get bumped: https://crates.io/crates/windows-sys/versions

With this in mind, I support the use of windows-sys instead of windows.

@TroyNeubauer
Copy link
Collaborator

Nice find! Seems reasonable enough

@TroyNeubauer TroyNeubauer merged commit 5c801d0 into nulldotblack:main Jul 18, 2024
6 checks passed
@TroyNeubauer
Copy link
Collaborator

Will release shortly after some testing

@ssrlive ssrlive deleted the patch3 branch July 19, 2024 02:36
@TroyNeubauer
Copy link
Collaborator

Released as v0.5.0. Thanks again!

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