Skip to content

Conversation

@mateoatr
Copy link
Contributor

@mateoatr mateoatr commented Oct 2, 2021

Should fix #59473.

@ghost ghost added the area-Host label Oct 2, 2021
@ghost
Copy link

ghost commented Oct 2, 2021

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

Issue Details

Should fix #59473.

Author: mateoatr
Assignees: -
Labels:

area-Host

Milestone: -

@mateoatr
Copy link
Contributor Author

mateoatr commented Oct 2, 2021

@ericstj could you please give this a shot? I did some validation but best I have now is x86 emulation on a win-arm64 machine, we should try the real thing (running x64 on a windows arm64 machine).

No need unloading kernel32 dll
Remove unnecessary variable
Remove unnecessary else-if branch
@ericstj
Copy link
Member

ericstj commented Oct 6, 2021

Do you also plan to fix the mac side of this? It should also have different defaults when run translated. If you think about both together, would it make sense to make this part of the PAL?

}

// Check if we are running an x64 process on a non-x64 windows machine.
return pProcessMachine != pNativeMachine && pProcessMachine == IMAGE_FILE_MACHINE_AMD64;
Copy link
Member

Choose a reason for hiding this comment

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

One thing to be careful about is that pProcessMachine will be IMAGE_FILE_MACHINE_UNKNOWN if this process is not being run as WOW (eg: process = native = x64). That doesn't impact what this method returns, since it intends to communicate not native architecture but I can see that being a place of confusion in the future if this method was refactored. It be more future proof to just check pNativeMachine, this is what we do in the installer.

The reason being, that we've established that the dotnet folder is for the native architecture. Any future non-native architecture will get its own subfolder (if it isn't redirected by the OS).

Use sysctlbyname on macOS
@agocke
Copy link
Member

agocke commented Oct 19, 2021

@vitek-karas Anything else we need here?

@mateoatr mateoatr merged commit 26c9b28 into dotnet:main Oct 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 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.

Apphost defaults to wrong install location on multi-arch systems

5 participants