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

Set windows-implement min rust-version #1773

Merged
merged 1 commit into from
May 20, 2022

Conversation

wravery
Copy link
Collaborator

@wravery wravery commented May 20, 2022

Both the #[implement] macro and the implement feature in windows require newly stabilized features in rustc 1.61 (which just hit stable 馃帀). We can specify it on the macro crate so anyone who wants to use the macro knows they need to update their toolchain and gets a clear error message if they don't.

I don't think you can specify a conditional rust-version which depends on the feature, but if there's a way to do that it would make sense to add it to the windows crate as well.

@kennykerr
Copy link
Collaborator

As part of #1771 I planned to remove the nightly restriction for testing. It is not sufficient to set the rust-version (although definitely a good move) as we also need to ensure that the yml scripts actually test that version. It's probably simplest to set the min version for the windows crate as a whole and test that via the yml scripts. The yml scripts are currently only testing 1.59 and nightly. The yml scripts are generated here:

https://github.com/microsoft/windows-rs/blob/master/crates/tools/yml/src/main.rs

@wravery
Copy link
Collaborator Author

wravery commented May 20, 2022

As part of #1771 I planned to remove the nightly restriction for testing. It is not sufficient to set the rust-version (although definitely a good move) as we also need to ensure that the yml scripts actually test that version. It's probably simplest to set the min version for the windows crate as a whole and test that via the yml scripts. The yml scripts are currently only testing 1.59 and nightly. The yml scripts are generated here:

https://github.com/microsoft/windows-rs/blob/master/crates/tools/yml/src/main.rs

OK, do you mean that you would like me to add rust-version to windows and update the yml script (presumably to update stable and nightly, and drop/replace 1.59 with stable) in the same PR? Or is that more of a forward-looking suggestion for another PR?

@kennykerr
Copy link
Collaborator

Updating the yml generation is a much bigger job - happy for you to do it if you're in a hurry - but equally happy for you to wait until I've wrapped that up in a few days. I'm just not comfortable updating the rust-version without testing. I hope to get that done next week.

@wravery
Copy link
Collaborator Author

wravery commented May 20, 2022

Updating the yml generation is a much bigger job - happy for you to do it if you're in a hurry - but equally happy for you to wait until I've wrapped that up in a few days. I'm just not comfortable updating the rust-version without testing. I hope to get that done next week.

OK, what about updating just windows-implement in this PR first, then? It looks like it passed all the CI checks, but I guess that means they aren't testing the macro crate (or only testing it on nightly)?

@kennykerr
Copy link
Collaborator

Yep, it's only testing with nightly. I think the change is harmless now that 1.61 is stable, so I'll take it but yeah there's more work to be done.

@kennykerr kennykerr merged commit f30c143 into microsoft:master May 20, 2022
@wravery wravery deleted the implement-msrv branch May 20, 2022 20:07
@wravery
Copy link
Collaborator Author

wravery commented May 20, 2022

I think this may have already made the conditional MSRV work. When you add the implement feature to windows, it adds windows-implement as a dependency, right? When it tries to build windows-implement, it should already error out for its greater MSRV on earlier toolchains. 馃槃

@kennykerr
Copy link
Collaborator

Yep... 馃槃 Although testing that would mean having to have different runs for the different min versions.

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

2 participants