Uh oh!
There was an error while loading. Please reload this page.
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
newpavlov
commented
Aug 1, 2019
I've added a correct processing of NTSTATUS codes. Now only codes which start with |
josephlr
left a comment
There was a problem hiding this comment.
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.
| 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.
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
uwporrust_1_33feature. - Remove support for Windows XP and Vista.
There was a problem hiding this comment.
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.
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.
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 cfgs appropriately.
So we would do the following:
// build.rs
...
if target.contains("uwp"){println!("cargo:rustc-cfg=getrandom_uwp");// for BCryptGenRandomprintln!("cargo:rustc-link-lib=bcrypt");
...// src/lib.rs
...}elseif#[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.
Using a build.rs file seems appropriate in this case.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
newpavlov
commented
Aug 6, 2019
So I think we are good to merge this? |
josephlr
left a comment
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
dhardy
commented
Aug 8, 2019
BTW despite the request for review I would prefer to stay out of this one — I know little about the APIs. |
newpavlov
commented
Aug 8, 2019
@dhardy |
dhardy
commented
Aug 8, 2019
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). |
newpavlov
commented
Aug 8, 2019
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. |
RtlGenRandomis not accessible on UWP targets, so we have to handle them separately right now.cc @chouquette