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

Enable win registry install location for all architectures #54698

Merged
merged 6 commits into from
Aug 20, 2021

Conversation

mateoatr
Copy link
Contributor

Follow-up of #53763

Enable the multi-architecture install locations on Windows Registry. The registration mechanism works the same way, we are only lifting the requirement of having to be running on x64/x86 to use the registry.

@ghost
Copy link

ghost commented Jun 24, 2021

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

Follow-up of #53763

Enable the multi-architecture install locations on Windows Registry. The registration mechanism works the same way, we are only lifting the requirement of having to be running on x64/x86 to use the registry.

Author: mateoatr
Assignees: -
Labels:

area-Host

Milestone: -

Comment on lines -327 to -329
#if !defined(TARGET_AMD64) && !defined(TARGET_X86)
return false;
#else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that you made basically no changes to existing tests - does it mean that we don't run the tests on Windows ARM32 or ARM64 at all?

Or does it mean that we didn't have any tests validating the current behavior?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently the host tests don't run on arm or arm64 at all (Windows or not):

- name: SkipTests
value: ${{ or(
not(in(parameters.archType, 'x64', 'x86')),
eq(parameters.runtimeFlavor, 'mono'),
eq(parameters.isOfficialBuild, true),
eq(parameters.crossBuild, true),
eq(parameters.pgoType, 'PGO')) }}

Copy link
Contributor Author

@mateoatr mateoatr Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll enable them on a separate PR (which should be merged before this one) -- curious if this will cause existing tests to fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair warning - I expect enabling them may be a bit involved. I think the main reason the arm/arm64 tests are currently skipped is that the hosting tests haven't been set up to send tests to helix, so they just run on the build machine (which works fine for x64/x86).

src/native/corehost/common.cmake Show resolved Hide resolved
Comment on lines -327 to -329
#if !defined(TARGET_AMD64) && !defined(TARGET_X86)
return false;
#else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently the host tests don't run on arm or arm64 at all (Windows or not):

- name: SkipTests
value: ${{ or(
not(in(parameters.archType, 'x64', 'x86')),
eq(parameters.runtimeFlavor, 'mono'),
eq(parameters.isOfficialBuild, true),
eq(parameters.crossBuild, true),
eq(parameters.pgoType, 'PGO')) }}

src/native/corehost/hostmisc/pal.h Outdated Show resolved Hide resolved
Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be able to merge this - assuming we did some manual testing of it.

@agocke
Copy link
Member

agocke commented Aug 19, 2021

@mateoatr Is this ready to go?

@mateoatr
Copy link
Contributor Author

@mateoatr Is this ready to go?

Yup. I did some local validation on a win-arm64 machine: arm apps work as expected, running an x86 apphost works fine as well. The dotnet run scenario is broken, but we are already tracking that and working on a fix in the SDK.

@agocke
Copy link
Member

agocke commented Aug 20, 2021

Do we still need to swap the DOTNET_ROOT ordering, or is that already done?

@mateoatr
Copy link
Contributor Author

Do we still need to swap the DOTNET_ROOT ordering, or is that already done?

Nope. It's already done -- I was having issues with dotnet run but simply running the apphost is good, and the ordering is fine.

@mateoatr mateoatr merged commit 4209a95 into dotnet:main Aug 20, 2021
@agocke
Copy link
Member

agocke commented Aug 20, 2021

/backport to release/6.0-rc1

@github-actions
Copy link
Contributor

Started backporting to release/6.0-rc1: https://github.com/dotnet/runtime/actions/runs/1151559541

@ghost ghost locked as resolved and limited conversation to collaborators Sep 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants