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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt official windows-rs API #5

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Adopt official windows-rs API #5

wants to merge 5 commits into from

Conversation

spikespaz
Copy link

I do not know if this works. I am fairly new to Rust, but I did my best. I did this because I do not want both winapi and windows-rs as dependencies in my future project. Please see rust-windowing/winit#2057, as my contribution is based off of this.

Please leave comments on any problems with the changes, and I will be sure to fix them. I do not know how you are testing this, I may wish to implement tests for you. 馃槃

@bbqsrc
Copy link
Owner

bbqsrc commented Nov 18, 2021

Very interesting. I was thinking about investigating this recently, so I'm very happy that you've done that for me haha.

How does this impact build times?

@spikespaz
Copy link
Author

spikespaz commented Nov 18, 2021

4.38s --> 7.66s Edit: May not be an accurate measurement. Best try it yourself.

There's discussion about the speed (or lack thereof) compiling windows-rs in the PR I linked above.

@bbqsrc
Copy link
Owner

bbqsrc commented Nov 18, 2021

Yeah it's not a strict science, but ballparks are relevant. Will have a think on this for a bit and get back to you. 馃槃

@U-C-S
Copy link

U-C-S commented Apr 9, 2022

@bbqsrc any update on this ? Would love to have this merged....

Since, The PR repo is deleted, Is there anyway I could least use this PR as a dependency ??


registry = { git = "https://github.com/bbqsrc/registry-rs", tag = "d912b165e826add5d484ff21683a044585cb591f" }

is not working well

EDIT:

registry = { git = "https://github.com/bbqsrc/registry-rs", rev = "refs/pull/5/head" }

is working good. Thanks to this.

@spikespaz
Copy link
Author

I just ran across this again, and wanted to close it because it seems stale. Just checking before I do, to make sure this change isn't wanted.

@MarijnS95
Copy link

@spikespaz it looks like everyone is slowly moving away from winapi towards the official and maintained windows bindings, in various forms. Though this PR needs a rather large overhaul as the bindings are now much further ahead and in a much better place than when these changes were made.

If the build times are still an issue on the latest version, and if these types aren't leaking into the public API, you may look into the recently released windows-core crate together with windows-bindgen which generates a more modest private set of bindings limited to "just the ones you need" (unfortunately currently with all the types (in)directly referenced by functions in a module/type).

An up-to-date example is available at strawlab/iana-time-zone#117; let me know if you need assistance integrating/testing this for registry-rs!


And landing this will be a "Fixes #9" 馃槵

@spikespaz
Copy link
Author

spikespaz commented Aug 27, 2023

It looks like the Cargo.lock is in the .gitignore. It shouldn't be, the lock files (for almost any build system) are intended to be included in VCS for reproducibility.

Can I fix that?

@spikespaz
Copy link
Author

It looks like the Cargo.lock is in the .gitignore. It shouldn't be, the lock files (for almost any build system) are intended to be included in VCS for reproducibility.

Can I fix that?

I am wrong, that is not the case for libraries.

@stefnotch
Copy link

@spikespaz I'm sure you've already heard this, but the official Rust guidance these days is: It depends on the project's needs, with the default being that the Cargo.lock is included.

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

5 participants