-
-
Notifications
You must be signed in to change notification settings - Fork 317
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
Conversation
There was a problem hiding this 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.
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. |
55eab40
to
b78a355
Compare
I truly recommend to get access to a Windows machine for local debugging, triggering CI for each run is painful in so many ways. 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'. |
That's the problem, I can't repro any of the errors on a Windows VM, they only occur in the CI. 😦 |
968f211
to
d223aee
Compare
@Byron sorry about the churn, had a flipped logic check that didn't matter locally but did in CI. 😓 |
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 Thanks again - I think I will also go for Closing, as the squashed version of this PR was just merged into |
Personally I don't even like windows-sys, but most maintainers seem to shy away from doing minimalistic bindings. |
Thanks for sharing, it's good to have seen what's possible! I am definitely among those who avoid any direct FFI usage 😅. |
There's also cc, which uses It used to have FFI definition directly though |
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. |
I happened to notice that
gix-sec
was the sole reason that thewindows
crate was being used incargo-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 forgix-sec
.