-
Notifications
You must be signed in to change notification settings - Fork 204
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
Add Windows UWP support #69
Conversation
I've added a correct processing of NTSTATUS codes. Now only codes which start with |
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.
This looks good, I don't know if you think bothering with the info/warning codes is worth it, but I like shifting the error codes into the OS error space.
src/lib.rs
Outdated
target = "x86_64-uwp-windows-gnu", | ||
target = "aarch64-uwp-windows-msvc", | ||
target = "x86_64-uwp-windows-msvc", | ||
target = "i686-uwp-windows-msvc", |
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.
Looks like this will not work, see rust-lang/rust#63217. :/ So our options are:
- Do not support UWP
- Bump MSRV to Rust 1.33
- Add
uwp
orrust_1_33
feature. - Remove support for Windows XP and Vista.
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.
I think we should bump the MSRV to 1.33 (and release 0.1.8), Rust 1.38 is coming out in a few days (Aug 12 I think), so bumping to 1.33 then seems reasonable.
Users still on 1.32 can still use 0.1.7
without any problems.
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.
Personally until something like rust-lang/rfcs#2495 lands I prefer a conservative position stating that MSRV bump should be considered a breaking change.
I think we also could move UWP check to the Windows branch, so MSRV bump will affect only Windows users, but I would prefer to drop support for Windows XP and Vista instead, but I guess it would require writing an RFC if we want getrandom
to be used as part of std
.
@dhardy
What dou you think?
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.
After looking at #75 and rust-lang/rfcs#2495, I think you're right. Incrementing the minimum supported version should be considered a breaking change. However, we can support UWP without bumping the MSRV, adding a feature, or dropping support for XP/Vista. We can just check in the build script.
libc
does this to maintain a comically low MSRV (Rust 1.13). They just check for the version in build.rs
and then set cfg
s appropriately.
So we would do the following:
// build.rs
...
if target.contains("uwp") {
println!("cargo:rustc-cfg=getrandom_uwp");
// for BCryptGenRandom
println!("cargo:rustc-link-lib=bcrypt");
...
// src/lib.rs
...
} else if #[cfg(all(windows, getrandom_uwp))] {
#[path = "windows_uwp.rs"] mod imp;
} else if #[cfg(windows)] {
#[path = "windows.rs"] mod imp;
}
...
This prevents exposing any of the details in getrandom
's public interface.
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.
Good workaround!
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.
Using a build.rs file seems appropriate in this case.
So I think we are good to merge this? |
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.
So I think we are good to merge this?
Yup, the changes look good to me. My only concern is that we don't have a way to run this in the CI yet, but that can be fixed in a followup PR.
BTW despite the request for review I would prefer to stay out of this one — I know little about the APIs. |
@dhardy |
Assuming none of those targets require nightly features, I think it would be more appropriate to use stable and/or beta for some of these tests? Otherwise looks fine (though I'm not very familiar with appveyor). |
In my understanding nightly CI tests are mainly used to catch potential regressions in Rust early to have a chance to fix them before they will hit beta/stable. |
RtlGenRandom
is not accessible on UWP targets, so we have to handle them separately right now.cc @chouquette