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

vstest has a hardcoded list of architectures which makes porting to new architectures harder #4034

Closed
omajid opened this issue Sep 29, 2022 · 10 comments

Comments

@omajid
Copy link
Contributor

omajid commented Sep 29, 2022

Description

vstest has a hard-coded list of architectures that is supported by the .NET runtime.

public enum Architecture
{
Default,
X86,
X64,
ARM,
AnyCPU,
ARM64,
S390x
}

This hardcoded list means that everytime someone ports .NET to a new architecture, they need to come to this repo and update vstest to get dotnet new xunit && dotnet test to work. If they forget that, the dotnet test command fails with strange errors for their users.

Since this repo is not part of the runtime/SDK and ships out of band, all users also have to update the xunit/vstest versions in their project files to pick up the latest release of vstest with the support enabled. For example, dotnet new xunit && dotnet test still fails out of the box for .NET 6.0.1xx when running on S390x.

The hardcoded list quickly gets out of date. For example, as of right now, vstest is missing 3 architectures that are supported in the runtime for .NET 7: LoongArch64, Armv6 and Ppc64le: https://github.com/dotnet/runtime/blob/ebaba40a5a9a71da0167b0838d87974240997113/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Architecture.cs#L38-L52

Is there any way to avoid duplicating the list of supported architectures in this repository and somehow get it from the runtime directly?

See #2954 and #4028, but also note the missing support in vstest for LoongArch64, Armv6 here because no one tested that.

Steps to reproduce

Try and use .NET 6 on an IBM Z (s390x) machine running Linux and run dotnet new xunit && dotnet test. Same thing when running .NET 7 for Armv6.

Expected behavior

dotnet new xunit && dotnet test works out of the box on new architectures, assuming it is supported by the .NET runtime

Actual behavior

dotnet new xunit && dotnet test fails to work out of the box on new architectures. The folks porting need to update vstest repository. Users of .NET need to update the xunit versions in their project fils to pick up support for new architectures.

cc @tmds @MichaelSimons

@omajid
Copy link
Contributor Author

omajid commented Sep 29, 2022

cc @Xinlong-Wu for riscv

@janani66
Copy link

Agree with Omair's comment. We are adding support for ppc64le for .NET7 and users will have to upgrade to vstest out of band for support of xunit on this new architecture.

@tmds
Copy link
Contributor

tmds commented Sep 29, 2022

fyi, msbuild also has an enum with architectures: https://github.com/dotnet/msbuild/blob/main/src/Utilities/ProcessorArchitecture.cs

@nohwnd
Copy link
Member

nohwnd commented Sep 30, 2022

We build for .NET Framework as well and can't simply use the enum from .NET runtime. Is the same enum published to some nuget that we can use for .NET Framework?

@tmds
Copy link
Contributor

tmds commented Jan 18, 2023

Rather than throwing for unknown architectures, vstest could use the value returned by RuntimeInformation.OSArchitecture and pass it around as a negative number in its Architecture/PlatformArchitecture enums.

I can look into the implementation.

@nohwnd wdyt?

@nohwnd
Copy link
Member

nohwnd commented Jan 19, 2023

How would this work for our client that is built against the oldest .NET Framework version that is still alive, currently .NET 4.6.2?

@tmds
Copy link
Contributor

tmds commented Jan 19, 2023

How would this work for our client that is built against the oldest .NET Framework version that is still alive, currently .NET 4.6.2?

We'd like vstest to work on a runtime that was ported to a new architecture when that architecture was not yet added to vstest.

The .NET Framework client doesn't need this logic because it is not used on these new ports.

@nohwnd
Copy link
Member

nohwnd commented Jan 24, 2023

We discussed this internally and would like to keep the architectures in a list that we control for now, because more often than not a new platform means more work than just adding item to enum on our side. Thanks for the PR offer but we would not accept it.

@nohwnd nohwnd closed this as completed Jan 24, 2023
@tmds
Copy link
Contributor

tmds commented Jan 24, 2023

Looking at the PR that added the ppc64le architecture: #4028, vstest doesn't seem to do anything with the architecture besides use it to throw NotSupportedException on the first chance.

Probably adding Unknown as values to these enums would even work.

@tmds
Copy link
Contributor

tmds commented Jan 24, 2023

.NET 7 test templates are being updated so they reference a version of vstest that knows about ppc64le: dotnet/test-templates#278.

If vstest wouldn't throw for unknown architectures, the existing packages would just work.

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

4 participants