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

Use windows-core with embedded bindings via windows-bindgen #117

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

MarijnS95
Copy link
Contributor

@MarijnS95 MarijnS95 commented Aug 20, 2023

Closes #116

As discussed in #97 (comment) iana-time-zone might not have to pull in the whole windows crate, but only generate the bindings (one single type and function call) that it needs and use simplified/stripped-down windows components from the (much lighter!) windows-core crate.

As also brought up in #97 (comment) however, this approach currently has as few drawbacks:

  • No cfg around APIs. The whole Windows.Globalization.Calendar type is generated, and a few functions require external types, which also require more types ,leading to a bit of (compile) overhead (and generated file bloat) that wasn't there with the windows crate;
  • MSRV bump to 1.56 ([discuss] increase MSRV #115).

On my machine this appears to be about half a second faster to compile.


Opening this as draft to put some visibility to the changes I've already made, and to have a place to discuss how/where we could improve this; allowing it to be merged (if folks are happy with this) as soon as there is an applicable MSRV bump.

@MarijnS95 MarijnS95 marked this pull request as draft August 20, 2023 19:37
@Kijewski
Copy link
Collaborator

Thank you for working on this! I propose this new CI test (.github/workflow/rust.yml):

  test-windows-api-gen:
    runs-on: ubuntu-latest
    steps:
      - name: Checkout
        uses: actions/checkout@v3
      - name: Install Rust
        run: |
          rustup toolchain install stable --profile minimal --no-self-update
      - run: cargo run --package api_gen
      - run: git diff --exit-code

This way we know if anything has changed upstream, and users can be sure that the code is what it claims to be. :)

@Kijewski Kijewski added Unsafe-Proposed `unsafe` code is added or changed Tier-1 Rust Tier-1 platform labels Aug 21, 2023
@Kijewski
Copy link
Collaborator

Kijewski commented Aug 21, 2023

The generated code works with windows-core==0.50.0, too. This version has an msrv of 1.48, so this PR would be an improvement without any downsides.

I would allow a version range as I proposed in #112: windows-core = { version = ">= 0.50, < 0.52" }. We already have tests in place if compiling with -Zminimal-versions works.

@MarijnS95
Copy link
Contributor Author

Thank you for working on this! I propose this new CI test (.github/workflow/rust.yml):

I hadn't yet pushed this from my other windows-bindgen crates, but have done so now. I prefer git status --porcelain because it includes untracked files, in case api_gen starts spitting out more files in future updates/changes but the submitter forgets to check them in.

Not sure what we need most rustup calls for: GitHub's virtual environments already come preinstalled with all that.

The generated code works with windows-core==0.50.0, too. This version has an msrv of 1.48, so this PR would be an improvement without any downsides.
I would allow a version range as I proposed in #112: windows-core = { version = ">= 0.50, < 0.52" }.

Nice catch! I hadn't even thought of trying this because windows-core 0.50 did not have a matching/compatible windows-bindgen release, but for this crate the code generated by windows-bindgen 0.51 (now that we finally have a release...) appears to be compatible with windows-core 0.50 (that isn't the case for my other Windows projects though...) 🎉!

There's a strict downside as users of MSRV 1.48 will now have to make sure to lock windows-core at 0.50, this crate will no longer compile out of the box for them. In that sense:

We already have tests in place if compiling with -Zminimal-versions works.

This will only apply to the wasm tests whose testing matrix currently combines MSRV 1.48 with -Zminimal-versions. The regular test job matrix checks MSRV 1.48 with and without -Zminimal-versions, which should fail to compile (as suggested in the previous paragraph).

@MarijnS95 MarijnS95 marked this pull request as ready for review August 26, 2023 21:14
@MarijnS95
Copy link
Contributor Author

Let's approve/run the CI to confirm the above?

@Kijewski
Copy link
Collaborator

Darn, this will change will increase the MSRV to 1.56 for unrelated platforms, too. Cargo fails if it finds Cargo.toml with editon = "2021" anywhere in its dependencies, regardless if the dependency is used or it. Cargo downloads any and all non-optional packages, even if the cfg(target_os = …) excluded the package from winding up in the resulting compilation.

IMO it's okay to bump the MSRV. Versions < 1.56 are unusable by now, because fundamental crates like syn are edition = 2021 by now.

@MarijnS95 MarijnS95 force-pushed the windows-core branch 2 times, most recently from 11b1cd7 to 968eb9c Compare August 28, 2023 21:01
@MarijnS95
Copy link
Contributor Author

Aplogies, it was me who forgot to replace edition = "2021" with edition = "2018" in api_gen, but even then its windows-bindgen dependency is still 2021. We can remove that crate from the workspace entirely as it's just a tool to generate the bindings, which are checked in and won't change afterwards. The tool itself isn't intended to be distributed with the crate.

I've removed it so that we can see where we stand now - probably still need to downgrade windows-core in the MSRV-1.48 Windows build.

@MarijnS95
Copy link
Contributor Author

Fwiw the -Zminimal-versions detailed above might be too harsh, we could even add a cargo update -p windows-core --precise 0.50.0 to only affect the MSRV of that crate. The rest could remain at latest.

@MarijnS95
Copy link
Contributor Author

@Kijewski where are we at with this?

@Kijewski Kijewski merged commit 791a63e into strawlab:main Oct 17, 2023
54 of 55 checks passed
@Kijewski
Copy link
Collaborator

Thank you for your work! Sorry that it took so long to get merged.

@MarijnS95 MarijnS95 deleted the windows-core branch October 17, 2023 05:46
@MarijnS95
Copy link
Contributor Author

No worries, I just wanted to see the CI run-failure so that I could add something akin to ed6476b, making sure that Windows+MSRV is only validated for -Zminimal-versions (and settling that it's okay that MSRV-compliancy is only there when manually or via -Zminimal-versions downgrading some dependencies) 👍

@MarijnS95 MarijnS95 restored the windows-core branch November 2, 2023 09:50
@MarijnS95 MarijnS95 deleted the windows-core branch November 2, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tier-1 Rust Tier-1 platform Unsafe-Proposed `unsafe` code is added or changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement windows support using windows-bindgen
2 participants