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

Bump ntapi to 0.4 to get rid of rejected code #1707

Closed
wants to merge 1 commit into from

Conversation

FrankenApps
Copy link

ntapi version 0.3.7 cotains code which is rejected using the latest version of rust.
I am unable to upgrade mio to 0.8 at the moment so it would be great if it was possible to release another 0.7.x version with this dependency upgrade.

For reference, here is the related issue in ntapi.

@Thomasdezeeuw
Copy link
Collaborator

I'm not sure this will work with our MSRV, but the CI will tell us.

@FrankenApps
Copy link
Author

FrankenApps commented Aug 12, 2023

I am not sure I understand this output correctly, but it seems that log version 0.4.20 is causing the current failure?

Maybe it needs to be pinned to v.0.4.8?

Edit

I did some additional digging and it seems v0.4.17 is fine (e.g. MSRV: 1.31.0+), but since v0.4.18 their MSRV is 1.60.0+. So we'd probably need to pin log to v0.4.17?

@Thomasdezeeuw
Copy link
Collaborator

I am not sure I understand this output correctly, but it seems that log version 0.4.20 is causing the current failure?

Maybe it needs to be pinned to v.0.4.8?

Edit

I did some additional digging and it seems v0.4.17 is fine (e.g. MSRV: 1.31.0+), but since v0.4.18 their MSRV is 1.60.0+. So we'd probably need to pin log to v0.4.17?

Yes, this is resolved in #1673, also see #1666.

I think we need to add --no-default-features to the CI, I'll do that in another pr.

@Thomasdezeeuw
Copy link
Collaborator

@FrankenApps can you rebase this on the v0.7.x branch? I've expanded to MSRV check of the CI to include Windows.

@FrankenApps
Copy link
Author

@Thomasdezeeuw Done. Thanks for getting all that other stuff in.
Hopefully CI will be happy...

@Thomasdezeeuw
Copy link
Collaborator

Ah see I was afraid of that ntapi v0.4 doesn't work with rustc 1.46. So, now we have a bit of a problem. Newer versions of rustc can't compile ntapi v0.3, but older versions can't compile v0.4...

@Darksonn
Copy link
Contributor

If you don't want to bump the MSRV of v0.7, then you could vendor the parts of ntapi you need. It's not very much:

$ rg ntapi
Cargo.toml
61:ntapi  = "0.3"

src/sys/windows/io_status_block.rs
4:use ntapi::ntioapi::IO_STATUS_BLOCK;
9:    use ntapi::ntioapi::IO_STATUS_BLOCK_u;

src/sys/windows/afd.rs
1:use ntapi::ntioapi::{IO_STATUS_BLOCK_u, IO_STATUS_BLOCK};
2:use ntapi::ntioapi::{NtCancelIoFileEx, NtDeviceIoControlFile};
3:use ntapi::ntrtl::RtlNtStatusToDosError;
120:    use ntapi::ntioapi::{NtCreateFile, FILE_OPEN};

@Thomasdezeeuw
Copy link
Collaborator

Or we can fix ntapi v0.3?

@FrankenApps
Copy link
Author

What is the strategy going to be here? If there is a chance to bump the MSRV for 0.7.x it would probably be the easiest solution. Otherwise I can take a shot at vendoring the needed parts.
Not sure if fixing ntapi 0.3.x is possible easily...

@Thomasdezeeuw
Copy link
Collaborator

What is the strategy going to be here? If there is a chance to bump the MSRV for 0.7.x it would probably be the easiest solution.

Except that's a breaking chance, more or less, there is plenty of discussion around this.

Otherwise I can take a shot at vendoring the needed parts. Not sure if fixing ntapi 0.3.x is possible easily...

I think we have the following options:

  • Fix ntapi v0.3.
  • Implement the parts of ntapi v0.3 ourselves.
  • Drop support for Mio v0.7 on newer rustc versions.

@Darksonn
Copy link
Contributor

I think vendoring is worth a shot.

@FrankenApps
Copy link
Author

I implemented the relevant code from ntapi v.0.3.7 in #1712.

@FrankenApps FrankenApps deleted the ntapi-upgrade branch November 18, 2023 20:39
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.

3 participants