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

Replace windows with windows-sys #1245

Closed
wants to merge 7 commits into from
Closed

Conversation

Jake-Shadle
Copy link
Contributor

I happened to notice that gix-sec was the sole reason that the windows crate was being used in cargo-deny, windows is an incredibly over complicated crate that I'd rather not have a dependency on, especially since the only reason to use it is if COM is being used, which is not the case for gix-sec.

Copy link
Owner

@Byron Byron 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 sorting this out!

Given that this should be all that gitoxide ever needs, it should be beneficial to have one big (and regularly breaking) crate gone from the dependency tree.

And since the code is fine to be 'write-once-work-forever', it's mainly about getting it to work (whether the code is more maintainable or not). Unfortunately I wouldn't be able to help with this, as having a overly complicated windows crate seems to be mirroring the whole Windows API too well.

@Byron
Copy link
Owner

Byron commented Jan 15, 2024

I put it back into draft so you can signal me to take another look for when CI is succeeding. Sorry for not being able to help here.

@Byron
Copy link
Owner

Byron commented Jan 18, 2024

I truly recommend to get access to a Windows machine for local debugging, triggering CI for each run is painful in so many ways.
Recently I realised that there are many cloud offerings that should make it easy to get a Windows session, even though I didn't use it myself yet (thus far I am using a local VM but it won't work forever on MacOS).

In any case, I unsubscribed here and kindly ask to ping me once CI is green, just in case I don't get notified if the PR is set to 'ready for review'.

@Jake-Shadle
Copy link
Contributor Author

That's the problem, I can't repro any of the errors on a Windows VM, they only occur in the CI. 😦

@Jake-Shadle
Copy link
Contributor Author

@Byron sorry about the churn, had a flipped logic check that didn't matter locally but did in CI. 😓

@Byron
Copy link
Owner

Byron commented Jan 18, 2024

Oh shoot 😁! I am glad at least that you didn't have to create this implementation with CI-feedback only.

And I didn't think I'd say that, but I really like this change! Overall it seems to be more correct now and it's easier to reason about it as the sys API doesn't try to hide anything. It's just plain FFI which also will be stable, as opposed to the windows crate which tends to break every know and then.

Thanks again - I think I will also go for windows-sys in future, while also hoping that I never have to use it again anyway.

Closing, as the squashed version of this PR was just merged into main.

@Byron Byron closed this Jan 18, 2024
@Jake-Shadle
Copy link
Contributor Author

Personally I don't even like windows-sys, but most maintainers seem to shy away from doing minimalistic bindings.

@Byron
Copy link
Owner

Byron commented Jan 18, 2024

Thanks for sharing, it's good to have seen what's possible!

I am definitely among those who avoid any direct FFI usage 😅.

@NobodyXu
Copy link
Contributor

There's also cc, which uses windows-bindgen to generate windows-sys style bindings

It used to have FFI definition directly though

@Byron
Copy link
Owner

Byron commented Jan 19, 2024

This is fantastic! If I had to ever create bindings, a binding-generator would also be my choice. There is so much to know about this for me unexplored aspect of Rust, maybe there is even a book about it.

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

3 participants