Use dotnet MSBuild for tests #19324
Use dotnet MSBuild for tests #19324
Conversation
|
Verified locally that when running |
| if %__Priority% GTR 0 (set "__PriorityArg=-priority=%__Priority%") | ||
| if %__Priority% GTR 0 ( | ||
| set "__PriorityArg=-priority=%__Priority%" | ||
| set "__PriorityMsbuildArg=/p:CLRTestPriorityToBuild=%__Priority%" |
There was a problem hiding this comment.
The issue was that for MSBuild the correct property is CLRTestPriorityToBuild, not Priority. The priority flag that was previously passed is mapped in the config.json file and converted in the depths of run.exe 😖 unclear why this mapping logic exists.
|
FYI @RussKeldorph with this commit, the work that @chsienki started should be complete. |
|
@chsienki Was there a reason we didn't update |
| <Exec Condition="'$(RunningOnCore)' == 'false'" Command="$(DotnetRestoreCommand) $(RestoreProj) $(PackageVersionArg)" StandardOutputImportance="Low" /> | ||
| <Exec Condition="'$(RunningOnCore)' == 'true'" Command="$(DotnetRestoreCommand) -r $(__DistroRid) $(RestoreProj) $(PackageVersionArg)" StandardOutputImportance="Low" /> | ||
| <Exec Condition="'$(__DistroRid)' == ''" Command="$(DotnetRestoreCommand) $(RestoreProj) $(PackageVersionArg)" StandardOutputImportance="Low" /> | ||
| <Exec Condition="'$(__DistroRid)' != ''" Command="$(DotnetRestoreCommand) -r $(__DistroRid) $(RestoreProj) $(PackageVersionArg)" StandardOutputImportance="Low" /> |
There was a problem hiding this comment.
Dependency restore is one of the pain points in test build. Could we explicitly output more detailed info on restore process? It would be sufficient to hook above commands log output at normal verbosity to logs and have a StandardOutputImportance console output set to High with minimal verbosity for console output set in restore command.
There was a problem hiding this comment.
What exactly is the pain point? Does it fail? Does it not properly fully restore? I am more than willing to add logging, but I would like to understand pain points because there may be a more general fix.
There was a problem hiding this comment.
The problem is that there are multiple already attempts at fixing dependencies in particular for performance tests where majority of them failed from different reasons. See #16647 #9281 #16126. Discussion related to the problem was in some comments of the https://github.com/dotnet/corefx/issues/31536#issuecomment-409635103 issue.
In general build-test configuration causes download of roughly 2.5 GB of assemblies out of which only about 500 - 600 are necessary. There is a possibility of reducing that amount by careful management of dependencies. This will lead to deterministic build which should be made against built product and chosen CoreFX or netstandard assemblies. Detailed information on dependency graph resolution would make any feature test dependencies updates easier.
Part of the above goal can be achieved by introducing new features into NuGet which would allow to override resolution of inbox assemblies with latest used version.
Solution which will not impact standard builds could be based on verbosity parameter passed to build-test.(cmd|sh) which would be used in the above code to set logging verbosity for restore and visibility of command line messages in console output by changing StandardOutputImportance
There was a problem hiding this comment.
That is indeed a valuable goal. In this case, however providing a new knob to configure logging for the update isn't the purpose of this PR. It is for a long standing work item related to consolidating usage of dotnet's MSBuild version. I think adding that support is very valid and should be done.
|
@AaronRobinsonMSFT My understanding is that using build-test.sh eventually builds using dotnet msbuild, whereas our build-test.cmd ends up with desktop We could theoretically edit the config.json to make it flip between desktop and core msbuilds on windows as needed, but my understanding is that a lot of this stuff will go away with ARCADE, so we're not updating it in buildtools, but going directly to msbuild ourselves (that's what @sbomer did when he updated the xunit wrappers anyway, so I just copied him :)) |
|
@chsienki Sounds good, thanks. |
|
@BruceForstall or @jashook Can you kick off the ARM tests for validation? Thanks. |
|
@dotnet-bot test Windows_NT x86 Checked Build and Test |
|
@dotnet-bot test Windows_NT arm64 Cross Checked normal Build and Test |
…orTests # Conflicts: # build-test.cmd
Revert the revert of #19254
cc @BruceForstall @chsienki