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

Drop support for Visual Studio 11 (2012) #981

Merged
merged 1 commit into from Mar 6, 2024
Merged

Conversation

dpaoliello
Copy link
Contributor

@dpaoliello dpaoliello commented Feb 26, 2024

@Be-ing
Copy link
Contributor

Be-ing commented Feb 28, 2024

the Rust toolchain no longer supports as tier 1 as of 1.76

The rust-version of cc is currently 1.53. Is keeping this code a maintenance burden? I think it may be a good idea to wait until the MSRV gets bumped to >= 1.76 for some other reason before removing this.

@dpaoliello
Copy link
Contributor Author

the Rust toolchain no longer supports as tier 1 as of 1.76

The rust-version of cc is currently 1.53. Is keeping this code a maintenance burden? I think it may be a good idea to wait until the MSRV gets bumped to >= 1.76 for some other reason before removing this.

Keeping this code is not a huge burden: it's small and self-contained.

It's more a policy question: if a vendor no longer supports a toolchain and OS, then should cc-rs continue to support it?

@Be-ing
Copy link
Contributor

Be-ing commented Feb 29, 2024

Maybe instead of removing the code, add a TODO comment to remove it when the MSRV is bumped to >= 1.76?

@dpaoliello
Copy link
Contributor Author

I'm removing the comment about OS support, since it isn't actually relevant: you can use a later toolchain and Windows SDK to build a binary that works on older versions of Windows (you just need to be careful not to use an API that was added later). So the fact that VS 2012 was tied to the Windows 8 SDK doesn't mean that cc-rs can't build a Windows 8 compatible binary using a later toolchain and SDK.

As for maintenance burden, I've realized that there is large burden that currently isn't being paid: testing compatibility. Although the code to find VS 2012 is small and unlikely to change, there is no guarantee that cc-rs and the Rust compiler won't start using a new feature in either cl or link that doesn't exist in VS 2012. Currently there is no testing for these older toolchains, and the fact that cc-rs can find and attempt to use an older toolchain creates an implicit contract that cc-rs (and, thus, the Rust compiler), by default, will work with these old toolchains.

It seems reasonable to me that cc-rs should have the policy to drop support for a toolchain once the vendor no longer supports it since testing that older toolchain and attempting to reproduct issues will become increasingly difficult.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Mar 1, 2024

It does seem make sense for me, though I would also like another maintainer to approve this.

cc @Amanieu

@NobodyXu NobodyXu requested a review from Amanieu March 2, 2024 04:16
@Amanieu
Copy link
Member

Amanieu commented Mar 5, 2024

I feel ambivalent about this and leave it up to the maintainer's discretion: there's no strong motivation to remove this now, but neither is there any reason to keep it.

@dpaoliello
Copy link
Contributor Author

@ChrisDenton thoughts on dropping unsupported versions of VS?

@ChrisDenton
Copy link
Contributor

ChrisDenton commented Mar 6, 2024

To my mind "support" is something we actively provide rather than passively have. There needs to be some motivation behind our support.

If someone did have a serious problem with VS 2012 that is hard to fix or required tough trade-offs, what would our response be? I think it would be to just drop support (even if only through inaction). In that case I don't think we can consider it supported to begin with.

And yes testing. This is already a problem for this crate and having a large number of versions to support does increase the burden on contributors and maintainers, especially when some of those versions are over a decade old. Unless we're happy with letting it break. In which case, see my previous paragraph.

tl;dr I'm not certain it is currently supported in practice and see no motivation to keep it, so yes I support removal.

@NobodyXu NobodyXu merged commit 9ac1dd8 into rust-lang:main Mar 6, 2024
21 checks passed
@NobodyXu
Copy link
Collaborator

NobodyXu commented Mar 6, 2024

Thanks @dpaoliello @ChrisDenton @Amanieu !

I agree with @ChrisDenton and decide to merge this PR.

BTW, I plan on increasing MSRV to be the same of jobserver (1.63) so that I can use new APIs introduced in rust-lang/jobserver-rs#73

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