dotnet / arcade

Tools that provide common build infrastructure for multiple .NET Foundation projects.
MIT License
673 stars 347 forks source link

Arcade limits the TestArchitectures to x64 on .NET Core #1291

Closed mikem8361 closed 5 years ago

mikem8361 commented 6 years ago

In our dotnet-diagnostictests repo conversion to Arcade I’ve been working on, we want to run the xunit tests on Windows as x86, why does Arcade error on it?

In Test.targets line 39 (Arcade version 1.0.0-beta.18516.5) remove the <Error Text=... below:

    <PropertyGroup>
      <_TestArchitecture>%(_TestArchitectureItems.Identity)</_TestArchitecture>
      <_TestOutPathNoExt>$(ArtifactsTestResultsDir)$(MSBuildProjectName)_$(TargetFramework)_$(_TestArchitecture)</_TestOutPathNoExt>
    </PropertyGroup>

    <Error Text="Architecture specified in TestArchitectures is not supported: '$(_TestArchitecture)'" File="XUnit"
           Condition="'$(_TestArchitecture)' != 'x64' and ('$(_TestArchitecture)' != 'x86' or '$(TargetFrameworkIdentifier)' == '.NETCoreApp')"/>

    <ItemGroup>
      <TestToRun Include="$(TargetPath)">
        <TargetFramework>$(TargetFramework)</TargetFramework>
tmat commented 6 years ago

This is because Arcade SDK assumes only a single .NET Core installation, which is 64-bit. We would need to do some work to support 32-bit and 64-bit. I'm thinking about removing TestArchitectures and instead enabling running 32/64 bit tests by setting the DotNetTool path to the corresponding dotnet.exe.

mikem8361 commented 6 years ago

For the diagnostics and dotnet-diagnostictests repos we need to build and run tests on x86 which this check prevents. I have also tweaked the common build.ps1 to allow an x86 cli/runtime to be installed by passing the architecture through build.ps1 to dotnet-install.ps1. I created an arcade issue and will get these minor change into arcade too.

So I think this error text line should be removed. Without it I can run the debugger tests on x86.

markwilkie commented 5 years ago

@mikem8361 - Any thoughts on next steps? (I saw the PR.....)

mikem8361 commented 5 years ago

I'm not sure exactly anymore. I abandoned the PR with dotnet install architecture changes. Tomas pointed out some alternatives in the newer Arcade that can be made to work.

But the "error text line" in test.targets should still be removed I think. I'll need some time to see if all of this will work for our diagnostic testing; I'm busy with coreclr repo engineering right now.

mikem8361 commented 5 years ago

I think this PR will address this issue: https://github.com/dotnet/arcade/pull/1958

/cc: @tmat

vatsan-madhavan commented 5 years ago

WPF will be affected by this as well - our tests are built and run separately for x86 vs x64.

/cc: @miguep, @ojhad , @rladuca, @stevenbrix

markwilkie commented 5 years ago

@vatsan-madhavan - do you know if @mikem8361 PR #1958 fixes your scenario?

tmat commented 5 years ago

This will be also needed to support 32bit core testing: https://github.com/dotnet/arcade/pull/2343

markwilkie commented 5 years ago

Ah yes, good point @tmat

vatsan-madhavan commented 5 years ago

I finally got to try out @chcosta's MultiFramework changes and found that #1958 comes in very handy.

For our tests, we need to match the bitness of dotnet.exe/test runner for 32-bit vs 64 bit - it's not enough to have the right arch of the SDK. In effect, I'm still not able to get our tests to run effectively and might need additional tweaks before we get things working.

@chcosta suggested that perhaps we should be running the whole 32-bit build in a WOW process (when running on x64 build machines) - this would have the effect of installing 32-bit sdk (or using x86 global sdk), and running tests using 32-bit runners etc. This seems like a reasonable approach at first glance - I'm wondering if we can add -platform to eng\build.ps1 analogous to -configuration.

Future -

Thoughts?

/cc @tmat /cc @rladuca, @miguep, @ojhad

vatsan-madhavan commented 5 years ago

Bitness of the test runner is generally coupled with the bitness of the tests, which are in turn coupled with the bitnesss of the build itself. At least this is how WPF builds work. Are there other repos that actually decouple /p:Platform and the test-runner bitness into truly separate concerns?

tmat commented 5 years ago

Yes, we can run 32bit and 64bit tests against .NET Framework: https://github.com/dotnet/arcade/blob/master/Documentation/ArcadeSdk.md#testarchitectures-list-of-strings

tmat commented 5 years ago

which are in turn coupled with the bitnesss of the build itself

I don't think this is necessary.

vatsan-madhavan commented 5 years ago

@tmat - oh right, i should remember that most of the universe is made up of AnyCPU :)

tmat commented 5 years ago

yes, almost everything is :)

tmat commented 5 years ago

I think we should: 1) Modify the dotnet installer to install x86 to .dotnet\x86. 2) Set build property DotNetToolX86 = $(DotNetRoot)x86\dotnet.exe 3) Update the test runner to use DotNetToolX86 when TestArchitectures is x86.

Then we can also add -platform which just sets /p:Platform. Since TestArchitectures is initialized from Platform if specified it would pick x86 atuomatically. You could also specify TestArchitectures="x64;x86" and run both at the same time if the test is built as AnyCPU.

vatsan-madhavan commented 5 years ago

@tmat, @chcosta, is there a way to specify both x86 and x64 runtimes in global.json so both architectures are made available ?

When I try something like this, I get an error;

    "runtimes": {
      "dotnet/x86": [
        "2.1.7",
        "$(MicrosoftNETCoreAppVersion)"
      ],
      "dotnet/x64": [
        "2.1.7",
        "$(MicrosoftNETCoreAppVersion)"
      ]
C:\Users\username\.nuget\packages\microsoft.dotnet.arcade.sdk\1.0.0-beta.19266.3\tools\InstallDot NetCore.targets(15,5): error MSB4018: The "InstallDotNetCore" task failed unexpectedly. [C:\Users \username\.nuget\packages\microsoft.dotnet.arcade.sdk\1.0.0-beta.19266.3\tools\Tools.proj]
C:\Users\username\.nuget\packages\microsoft.dotnet.arcade.sdk\1.0.0-beta.19266.3\tools\InstallDot NetCore.targets(15,5): error MSB4018: System.ArgumentException: An item with the same key has already been added.
tmat commented 5 years ago

Having both should be possible once https://github.com/dotnet/arcade/issues/1291#issuecomment-492856534 is implemented.

Also, InstallDotNet task could take Platform as a parameter. If specified and is not AnyCPU it would use it as a default platform for runtime with unspecified architecture. This would be useful for (common) case when you have separate CI leg for each platform. Each leg would just download the architecture it needs, and not both.

chcosta commented 5 years ago

Agreed. That wasn't a scenario initially discussed but tmat's proposal should address it.

chcosta commented 5 years ago

@natemcmaster , any concerns with https://github.com/dotnet/arcade/issues/1291#issuecomment-492856534 as an evolution of multi-framework?

vatsan-madhavan commented 5 years ago

It might be simpler to install arch-specific (either x86, or x64) under its own folder all the time. It would work nicely in the future when arm becomes more of a concern.

Please take a look at this candidate fix: https://github.com/dotnet/arcade/commit/bb5a5805ad219c909d2c6aaa70e8c75adb7ac32a

tmat commented 5 years ago

It would require fixing up existing code that expects dotnet.exe to be in .dotnet and most repos do not care about x86 at all.

Also, we have a precendent for this pattern: https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.Arcade.Sdk/tools/ProjectLayout.props#L14-L15

natemcmaster commented 5 years ago

It might be simpler to install arch-specific (either x86, or x64) under its own folder all the time

+1. AspNetCore already installs to .dotnet/x64 and .dotnet/x86 as we need both architectures for testing.

As long as we're considering this, could we make the installation include the OS name? .dotnet/win-x86/ I regularly develop for linux using WSL, and it's annoying to me that I have to continually delete my .dotnet folder to switch between windows and linux versions of .NET Core.

tmat commented 5 years ago

As long as we're considering this, could we make the installation include the OS name?

I'd leave that for another PR. See https://github.com/dotnet/arcade/issues/1293

natemcmaster commented 5 years ago

If we agree adding OS name is something that has value, I figure we should do this now. Might as well not break people twice, right? As you mentioned, any change here will...

require fixing up existing code that expects dotnet.exe to be in .dotnet

tmat commented 5 years ago

As you mentioned, any change here will...

Not any change. Not if we keep 64bit where it is now.

tmat commented 5 years ago

@natemcmaster What would be the OS names for various Linux distros?

markwilkie commented 5 years ago

Looks like @dsplaisted has a prototype for #1293?

natemcmaster commented 5 years ago

@tmat I think we're saying the same thing. Emphasis on IF

It might be simpler to install arch-specific (either x86, or x64) under its own folder all the time

👉 As long as we're considering this, could we make the installation include the OS name?

I don't know if changing to side-by-side x64/x86 folders is worth the break. Nesting a new .NET Core installation inside DOTNET_ROOT seems like something that might be fragile, but I don't have a concrete example.

tmat commented 5 years ago

@markwilkie Yes, but I think we should address the build outputs (artifacts) naming differently than dotnet dir.

tmat commented 5 years ago

@natemcmaster I see what you meant. Misunderstood.

vatsan-madhavan commented 5 years ago

I started a PR for this at https://github.com/dotnet/arcade/pull/2815

vatsan-madhavan commented 5 years ago

@mikem8361 this should be fixed now. ok to close?

mikem8361 commented 5 years ago

Yes.