-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Hardcode LKG version of ucrtbased.dll #25444
Conversation
Crap, formatting is still failing. Looks like they get UCRT a different way. looking. |
|
Formatting doesn't happen in official builds, so if this passes everything else, it may be best to merge it (to unblock |
@hoyosjs Can you help with this? You mentioned something about ucrt earlier. |
OSX failure may be unrelated, there's nothing about ucrt in there. It was the Windows jobs that were failing previously.
|
I missed this before (it's in the logs of other failing PRs):
This shows up for every test project. |
@wtgodbe It looks like the windows jobs have gotten past the Build Tests phase this time, so it probably is something about certain build machines. |
If the OSX failure is unrelated, then this is good enough as a workaround for now. |
My main question here is will this require every dev on the team to install the given version of the package. (For example, if Dev16 is detected on the box, it gets used as the default. And how will this translate for people who already nuked Dev15) That being said, I'm fine with this as a stop gap solution only if you can ensure this one is always installed in the machines we build on. |
Yes, @MattGal and I confirmed that the hardcoded version is installed on the build machines we use. We can remove this workaround once either the VS bug is fixed, or we move up to the next dot version. |
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.
Cool, can you please open an issue to track it?
So does this mean that close to everybody building CoreCLR locally will get a build break, will need to trace down this issue, and apply a workaround (what is the workaround exactly?) somehow? This is major time sink and productivity loss across both our team and our community. Can we do better? E.g. apply this workaround conditionally? |
Best I can tell, this would be "only people who keep their VS 2017 fully patched," and possibly actually "only people who recently have done a clean install of VS 2017" |
Good idea. @wtgodbe can we try: <None Include="$(ExtensionSdkDir)/Microsoft.UniversalCRT.Debug/$(UCRTVersion)/Redist/Debug/$(Platform)/ucrtbased.dll" CopyToOutputDirectory="Always"
Condition="Exists($(ExtensionSdkDir)/Microsoft.UniversalCRT.Debug/$(UCRTVersion)/Redist/Debug/$(Platform)/ucrtbased.dll)" />
<!-- There's a bug in VS that causes UCRTVersion env var to get set to a version that isn't actually installed on the machine. -->
<!-- Until that gets resolved, we need this workaround to grab the last "known" good version of ucrtbased.dll -->
<None Include="$(ExtensionSdkDir)/Microsoft.UniversalCRT.Debug/10.0.17763.0/Redist/Debug/$(Platform)/ucrtbased.dll" CopyToOutputDirectory="Always"
Condition="!Exists($(ExtensionSdkDir)/Microsoft.UniversalCRT.Debug/$(UCRTVersion)/Redist/Debug/$(Platform)/ucrtbased.dll)" /> |
Good call, I've pushed your suggestion @hoyosjs |
Looks like I forgot to quote the string in the exists condition for the snippet I sent. |
Windows failure is a test timeout
|
* Hardcode LKG version of ucrtbased.dll * Only use hardcoded UCRT version when search path fails * Add missing single-quotes
@wtgodbe Are you working on fixing the Formatting job failures too? |
I haven't taken a look at the formatting job, as it isn't blocking the official builds. It's likely related to the |
Sure, here's the default Lib paths from the 15.9.12 machine:
... appending the relevant 17763 ones to the existing lib should work, or overriding the value entirely as a workaround if files that should be on those paths cannot be located. |
This reverts commit aa03833.
I have opened https://github.com/dotnet/coreclr/issues/25499 to have this tracked. I tried to find the right place where to add this workaround, but I was not able to figure it out. The python formatting script fails on my local machine much sooner due number of other problem that makes diagnosing this hard. It would be great if somebody more familiar with this can take a look. |
Auto-detection of winsdk version proved to be fragile so this change sets it explicitly. Undo the workaround for UCRTVersion (dotnet#25444). Re-enable Windows formatting job (dotnet#25507). Fixes #25447, #25499.
Auto-detection of winsdk version proved to be fragile so this change sets it explicitly. Undo the workaround for UCRTVersion (dotnet#25444). Re-enable Windows formatting job (dotnet#25507). Fixes #25447, #25499.
Auto-detection of winsdk version proved to be fragile so this change sets it explicitly. Undo the workaround for UCRTVersion (dotnet#25444). Re-enable Windows formatting job (dotnet#25507). Fixes #25447, #25499.
VS2017 VsDevCmd had a bug in setting of UCRTVersion environment variable. That was affecting Interop tests and Windows formatting jobs. We added a workaround for the former (dotnet#25444) and disabled Windows formatting jobs (dotnet#25507, dotnet#25902). The bug has been fixed in VS2019. Since we switched to VS2019 pool we can remove the workaround and re-enable Windows formatting jobs. Fixes #25447, #25499.
…27514) VS2017 VsDevCmd had a bug in setting of UCRTVersion environment variable. That was affecting Interop tests and Windows formatting jobs. We added a workaround for the former (#25444) and disabled Windows formatting jobs (#25507, #25902). The bug has been fixed in VS2019. Since we switched to VS2019 pool we can remove the workaround and re-enable Windows formatting jobs. Fixes #25447, #25499.
* Hardcode LKG version of ucrtbased.dll * Only use hardcoded UCRT version when search path fails * Add missing single-quotes Commit migrated from dotnet/coreclr@aa03833
Should unblock CI errors that look like:
@MattGal and I investigated this, it appears to be a bug with the new version of VS that incorrectly sets
UCRTVersion
to a version that isn't installed on the machine. This workaround hard-codes the LKG version until the bug gets resolved (or permanently, if we decide that we don't want to risk getting broken like this again).@MattGal @hoyosjs @BruceForstall @jkoritzinsky @AaronRobinsonMSFT PTAL