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 sys with manual bindings #90

Closed
wants to merge 14 commits into from

Conversation

Jake-Shadle
Copy link

This removes the dependency on windows-sys in favor of just having manual binding to only the types and functions actually needed by schannel, to avoid future issues due to version churn. microsoft/windows-rs#1720

I realize this is a large change, but functionally it should just work, though it appears as if tests are currently broken at HEAD. #89 (comment)

Closes #89

@GamePad64
Copy link

There is nothing wrong of having a crate installed multiple times. But there is a problem of having to write the bindings by yourself.
Just imagine, that you want to use some new function from WinAPI, that you have never used before in your project. So, you have to create a binding by yourself, and then test it by hand. So you have to think about the correctness of boilerplate code instead of business logic issues.
Also, the willingness of new contributions by the community would be much lower, bc no one likes writing boilerplate code by themselves :)

windows-sys:
Positive:

  • Reduces boilerplate code
  • Reduces TTM value
  • Ensures correctness, by being autogenerated from win32metadata, which is used in most win32 bindings by Microsoft.
  • Actively maintained
    Negative:
  • About 2MB of gzipped code (srsly?)
  • Can have breaking changes between versions (0.* version numeration)

I strongly advise against pulling the bindings in the project code and taking the burden of maintaining the bindings by the project contributors.

@nox
Copy link

nox commented Mar 28, 2023

@GamePad64 Note that breaking changes between 0.1 and 0.2 is perfectly acceptable with crates.io rules as long as 0.1.1 is compatible with 0.1.0. Just because the first number is 0 doesn't mean there are no backwards compatibility promises being made.

@Jake-Shadle Jake-Shadle force-pushed the remove-windows-sys branch 22 times, most recently from ccfd221 to 9ae9596 Compare March 28, 2023 14:12
@Jake-Shadle
Copy link
Author

Jake-Shadle commented Mar 28, 2023

I've added a Github Actions workflow that mimics the appveyor one just for iteration purposes since appveyor is extremely slow and unresponsive, but I can delete it if that hampers merging

See https://github.com/EmbarkStudios/schannel/actions/runs/4544087046 for a run.

@Jake-Shadle
Copy link
Author

The test failure is a sporadic failure, ironically concerning SSL/TLS.

@Jake-Shadle
Copy link
Author

The bindings are now generated with https://github.com/microsoft/windows-rs#windows-bindgen instead of my own tool since people apparently care a lot about that.

src/bindings.rs Outdated
pub(crate) mod cryptography;
pub(crate) mod identity;

// Uncomment this to regenerate bindings

Choose a reason for hiding this comment

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

I'd recommend running this test unconditionally. Then you can ensure that the bindings are always up to date and regressions haven't snuck in. You can use a diff check via CI to ensure the generated bindings haven't diverged from what's committed to your repo.

@Jake-Shadle
Copy link
Author

Note I had to update the self-signed.badssl.com cert/SHA-1 fingerprint since it seems it changed again.

@steffengy
Copy link
Owner

steffengy commented Mar 31, 2023

Note I had to update the self-signed.badssl.com cert/SHA-1 fingerprint since it seems it changed again.

@Jake-Shadle Yeah I took time to automate that finally now, as you've noticed. Doesnt help appveyor spuriousity only has gone up with one of their workers I presume having spurious network instability.
I'll have a look at the github actions eventually and use that or merge as-is.
Only advantage of AppVeyor test compared to Github might be it tests/confirms working on an older windows version, might limit that in the future to only happen on an "appveyor" branch or sth since it seems to have deteriorated.

Have given this only a quick scrollover besides that, and generally also sharing a similar sentiment as @kennykerr : not necessarily thinking CI (diff) tests are necessary, but not a fan of having to uncomment various things to generate. E.g. maybe hide instead of a "hidden/internal" cargo feature or sth like that to trigger regeneration.

@kennykerr
Copy link

You could just let the test run and ignore the diff. Another option is to mark the test with #[ignore] and then use --ignored when you want to rerun the test.

@Jake-Shadle
Copy link
Author

Yeah I took time to automate that finally now, as you've noticed. Doesnt help appveyor spuriousity only has gone up with one of their workers I presume having spurious network instability.

Yah, my pushes were pretty consistently failing with network/SSL errors, I haven't seen appveyor being used in ages so I didn't know if it was just bad luck or things haven't been great on it in a while.

I'll have a look at the github actions eventually and use that or merge as-is.

I can split it out into a separate PR for convenience if you want, it's basically the exact same as the appveyor, with the one rather annoying caveat that while the Github Windows runner has mingw64 installed, it doesn't have i686 installed, so that job runs a lot slower since it needs to download and install the compiler/libs to compile and link.

Only advantage of AppVeyor test compared to Github might be it tests/confirms working on an older windows version, might limit that in the future to only happen on an "appveyor" branch or sth since it seems to have deteriorated.

Yup, Github actions only have 2019 or 2022 servers to choose from, so might be worth keeping appveyor around if you care about older Windows versions.

Have given this only a quick scrollover besides that, and generally also sharing a similar sentiment as @kennykerr : not necessarily thinking CI (diff) tests are necessary, but not a fan of having to uncomment various things to generate. E.g. maybe hide instead of a "hidden/internal" cargo feature or sth like that to trigger regeneration.

Yah, that might work, I just wanted to get the PR green before a review, I'm open to doing any changes you want for it to be merged.

The previous version required manual fixup of a couple things, this
needs no manual fixup, but is...much more verbose
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.

5 participants