-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Unpin cc
and upgrade to the latest version
#131070
Conversation
`cc` was previously pinned because version 1.1.106 dropped support for Visual Studio 12 (2013), and we wanted to decouple that from the rest of the automated updates. As noted in [2], there is no longer anything indicating we support VS2013, so it should be okay to unpin it. `cc` 1.1.22 contains a fix that may help improve the high MSVC CI failure rate [3], so we also have motivation to update to that point. [1]: rust-lang#129307 [2]: rust-lang#129307 (comment) [3]: rust-lang#127883
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
Unpin `cc` and upgrade to the latest version `cc` was previously pinned because 1.1.106 dropped support for Visual Studio 12 (2013), and we wanted to decouple that from the rest of the automated updates. As noted in [2], there is no longer anything indicating we support VS2013, so it should be okay to unpin it. `cc` 1.1.22 contains a fix that may help improve the high MSVC CI failure rate [3], so we also have motivation to update to that point. [1]: rust-lang#129307 [2]: rust-lang#129307 (comment) [3]: rust-lang#127883 try-job: x86_64-msvc-ext
Probably @bors rollup=never actually... |
👍 from me but this really should have a T-compiler reviewer |
Yeah you're right, thanks for confirming r? compiler |
☀️ Try build successful - checks-actions |
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.
Tagging as relnote but as VS 2013 has been out of support for months-to-years (depending on whether you count extended support or not), I don't think this will be a big deal for users.
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (1d71891): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 1.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary -2.6%, secondary -2.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 770.288s -> 770.698s (0.05%) |
I've recently tried to update the nightly we use in Wasmtime's CI and it's failing. After that I minimized the CI failure and ran a "bisection on github" which yielded this regression range. Within that range is this PR updating the
I unfortunately can't reproduce this locally and I think it's likely relatively specific to the setup of CI. I for example don't know what toolchain components are installed on github actions on Windows for the various bits and pieces here. All that's to say that I think this PR's update is the likely cause of the build issue we're running into. I certainly won't go so far as to say that this is the component at fault, it's probably just uncovering something about how we're configuring CI wrong or something like that. Specifically the shape of the issue is:
If I had to hazard a guess what I would suspect is happening is that MSBuild doesn't know it's doing a cross compile (I have no idea how one would configure it to do so if this is the case) and so it's setting some env vars which this Apologies for the long explanation here, but what I wanted to ask here is (a) if this is a known "change" that might happen with updating |
@alexcrichton are you familiar with the issue at #127883? We have been seeing issues with link.exe and others that only happen in CI (no local reproduction) for a few months now. There was a problem in a specific version of Probably worth bringing up in the Zulip thread, https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/Spurious.20CI.20errors.20on.20x86_64-msvc-ext |
Oh I don't think this is related to that in that this isn't a spurious error it's a deterministic error from what I've seen so far. This PR being the "cause" is I presume due to some change between 1.0.105 and 1.1.23 for |
We actually did have something similar where it was failing 100% of the time bumping 1.0.99 to 1.1.22, in #130720, which was bisected to a caching change in 1.0.101. This PR just includes a fix for that. But there were a lot of other things updated for MSVC within bumped versions rust-lang/cc-rs@cc-v1.0.105...cc-v1.1.23. Any chance you are able to adjust cc's version and see how that changes things? That is more or less what I did to narrow down this bad cc version in #130908. @ChrisDenton might have other ideas. There is definitely something weird going on with cc+GHA+MSVC... |
@alexcrichton This looks like a real error to me and I suspect this is a case where we have to make someone unhappy. cc-rs tries to respect external build systems (such as cmake and msbuild) because people often don't like us clobbering them. This complicates things and is made harder when build scripts are involved, as obviously they are compiled for the host rather than the target. From what I can tell, it looks like the environment is set up to target Otherwise you may want to open an issue on the cc-rs repo to figure out what's going on in your build and if we can make things work again without breaking other people or if we need to reevaluate some trade-offs that have been made. |
Ah ok that makes sense, thanks for explaining! I think bisection of I'll try to poke around and see if there's an "official" way to tell CMake that we're cross-compiling so it would hopefully forward different flags to the MSBuild generator. Either that or I'll probably just switch to Ninja which I predict won't have this issue. Failing all that I'll test out Thanks again for the info! |
Updating CMake to use Ninja did the trick so it looks like it was indeed MSBuild setting variables since MSBuild didn't think it was cross-compiling. |
cc
was previously pinned because 1.1.106 dropped support for Visual Studio 12 (2013), and we wanted to decouple that from the rest of the automated updates. As noted in 2, there is no longer anything indicating we support VS2013, so it should be okay to unpin it.cc
1.1.22 contains a fix that may help improve the high MSVC CI failure rate 3, so we also have motivation to update to that point.try-job: x86_64-msvc-ext
r? @ChrisDenton
cc @dpaoliello
cc @Mark-Simulacrum (from #128722 (comment))