Use new CoreDisTools package #44490
Use new CoreDisTools package #44490
Conversation
…ersion in eng/Versions.props
…n src/coreclr/src/tools/r2rdump/R2RDump.csproj
…ests/Common/Directory.Build.targets
…uild.cmd and src/tests/build.sh
|
I believe this should be ready for review - I ran gcstress pipeline and it shows the same behaviour as last Sunday run (https://dev.azure.com/dnceng/public/_build/results?buildId=879006) - it took the same time to finish and produces the same tests failures (that I believe should now be fixed). PTAL @dotnet/jit-contrib @dotnet/crossgen-contrib @dotnet/runtime-infrastructure I would like to have three sign-offs from a JIT team member (GCStress), an AOT team member (R2RDump) and someone who knows about the MSBuild infrastructure. |
|
I can see the R2RDumpTests is failing on all platforms with timeout in the CI tests. |
|
R2RDumpTests is a recently introduced test that I only earlier this week fixed to pass in Crossgen2 runs; after the fix I noticed yesterday that it still fails in the runtime-coreclr crossgen2 composite gcstress pipeline making me believe that the failure is only specific to GC stress testing. I'll go ahead and mark the test as GC stress-incompatible because its intent is not to stress the runtime - in fact the test is kind of "lengthy" in terms of CoreCLR unit tests as it disassembles the entire SPC assembly; I can easily imagine that the slowdown by an order of magnitude or more incurred by the GC stress-instrumented code can easily push it past the timeout. The test also runs in all CoreCLR test legs of the "runtime" pipeline that seem to be passing just fine. |
eng/coredistools.targets
Outdated
| @@ -0,0 +1,15 @@ | |||
| <!-- Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. --> | |||
| <Project> | |||
| <ItemGroup Condition="'$(DotNetBuildFromSource)' != 'true'"> | |||
There was a problem hiding this comment.
Should the whole file import instead be conditioned as the properties below won't work without the PackageReference.
There was a problem hiding this comment.
I can move this condition to Import element in R2RDump.csproj.
Would I need the same condition for src/tests/Common/Directory.Build.targets ? As far as I understand, we don't build tests during source build, so the condition will always be true.
There was a problem hiding this comment.
Yeah please add it to both imports. You are right that we don't build these currently in source build we might at some point.
There was a problem hiding this comment.
Thanks @ViktorHofer - I addressed both of your suggestions.
|
I've merged in the change disabling R2RDumpTests in GC stress mode; this should make the "runtime-coreclr gcstress0x3-gcstress0xc" legs green. |
1. Use $(RepositoryEngineeringDir) 2. Remove condition on ItemGroup in coredistools.targets and 3. Instead use the condition on Import elements in R2RDump.csproj and tests *.targets
Thank you Tomas, I merged master on top of the PR changes and will re-start gcstress pipelines |
| </ItemGroup> | ||
|
|
||
| <PropertyGroup> | ||
| <CoreDisToolsLibrary Condition="'$(TargetOS)' == 'Windows'">$(PkgMicrosoft_NETCore_CoreDisTools)\runtimes\win-$(TargetArchitecture)\native\coredistools.dll</CoreDisToolsLibrary> |
There was a problem hiding this comment.
@Anipik TargetOS for Windows should be lowercase windows, right?
There was a problem hiding this comment.
Let me follow up on this
There was a problem hiding this comment.
Thanks. I saw this in another place in your PR as well. It isn't causing a problem right now as msbuild is case insensitive but it should be cleaned up.
There was a problem hiding this comment.
Should be fixed in #44845
There was a problem hiding this comment.
What was the reason to have Linux and OSX uppercased but not Windows?
ghost
locked as resolved and limited conversation to collaborators
After the change R2RDump and GCStress will use a newer version of coredistools produced from https://github.com/dotnet/jitutils