Skip to content
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

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

Closed
mikem8361 opened this issue Nov 7, 2018 · 34 comments
Closed

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

mikem8361 opened this issue Nov 7, 2018 · 34 comments
Assignees

Comments

@mikem8361
Copy link
Member

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
Copy link
Member

tmat commented Nov 8, 2018

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
Copy link
Member Author

mikem8361 commented Nov 8, 2018 via email

@markwilkie
Copy link
Member

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

@mikem8361
Copy link
Member Author

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
Copy link
Member Author

I think this PR will address this issue: #1958

/cc: @tmat

@vatsan-madhavan
Copy link
Member

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
Copy link
Member

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

@tmat
Copy link
Member

tmat commented Apr 19, 2019

This will be also needed to support 32bit core testing: #2343

@markwilkie
Copy link
Member

Ah yes, good point @tmat

@vatsan-madhavan
Copy link
Member

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.

  • Add -platform parameter to eng\build.ps
  • Iniitally support [x86, x64]
  • Supply /p:Platform=x86 or /p:Platofrm=x64 to msbuild
  • When x86 is specified, respwan the build process if necessary as a WOW process.

Future -

  • Improve where we place build.binlog - put it under a location that is both configuration and platform specific.

Thoughts?

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

@vatsan-madhavan
Copy link
Member

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
Copy link
Member

tmat commented May 15, 2019

@tmat
Copy link
Member

tmat commented May 15, 2019

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

I don't think this is necessary.

@vatsan-madhavan
Copy link
Member

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

@tmat
Copy link
Member

tmat commented May 15, 2019

yes, almost everything is :)

@tmat
Copy link
Member

tmat commented May 15, 2019

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
Copy link
Member

@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
Copy link
Member

tmat commented May 16, 2019

Having both should be possible once #1291 (comment) 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
Copy link
Member

chcosta commented May 16, 2019

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

@chcosta
Copy link
Member

chcosta commented May 16, 2019

@natemcmaster , any concerns with #1291 (comment) as an evolution of multi-framework?

@vatsan-madhavan
Copy link
Member

vatsan-madhavan commented May 16, 2019

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: bb5a580

@tmat
Copy link
Member

tmat commented May 16, 2019

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
Copy link
Contributor

natemcmaster commented May 16, 2019

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
Copy link
Member

tmat commented May 16, 2019

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

I'd leave that for another PR. See #1293

@natemcmaster
Copy link
Contributor

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
Copy link
Member

tmat commented May 16, 2019

As you mentioned, any change here will...

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

@tmat
Copy link
Member

tmat commented May 16, 2019

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

@markwilkie
Copy link
Member

Looks like @dsplaisted has a prototype for #1293?

@natemcmaster
Copy link
Contributor

natemcmaster commented May 16, 2019

@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
Copy link
Member

tmat commented May 16, 2019

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

@tmat
Copy link
Member

tmat commented May 16, 2019

@natemcmaster I see what you meant. Misunderstood.

@vatsan-madhavan
Copy link
Member

I started a PR for this at #2815

@vatsan-madhavan
Copy link
Member

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

@mikem8361
Copy link
Member Author

Yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants