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

AppHost should be disabled by default for macOS like it is for iOS / tvOS / Mac Catalyst / Android #53307

Closed
rolfbjarne opened this issue May 26, 2021 · 11 comments
Milestone

Comments

@rolfbjarne
Copy link
Member

Description

Xamarin.Mac for .NET (the net6.0-macos target framework) are like mobile platforms in that the app host should be disabled by default here:

https:/dotnet/sdk/blob/c53c8ac81e1bcb8118c31bc4f5622396c72e9959/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets#L118-L119

I'm not sure what's the best way to do it though, since the applicable RIDs (osx-x64/osx-arm64) are also used for other target frameworks where the app host shouldn't get the same behavior.

CC @akoeplinger

@dotnet-issue-labeler dotnet-issue-labeler bot added area-Host untriaged New issue has not been triaged by the area owner labels May 26, 2021
@ghost
Copy link

ghost commented May 26, 2021

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

Issue Details

Description

Xamarin.Mac for .NET (the net6.0-macos target framework) are like mobile platforms in that the app host should be disabled by default here:

https:/dotnet/sdk/blob/c53c8ac81e1bcb8118c31bc4f5622396c72e9959/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets#L118-L119

I'm not sure what's the best way to do it though, since the applicable RIDs (osx-x64/osx-arm64) are also used for other target frameworks where the app host shouldn't get the same behavior.

CC @akoeplinger

Author: rolfbjarne
Assignees: -
Labels:

area-Host, untriaged

Milestone: -

@agocke
Copy link
Member

agocke commented May 26, 2021

We're actually planning on enabling the apphost for Mac due to the x64/ARM64 split on Apple Si.

What's the reason why mobile platforms need to have the apphost disabled?

@rolfbjarne
Copy link
Member Author

What's the reason why mobile platforms need to have the apphost disabled?

We don't use it, we host the runtime (either Mono or CoreCLR) ourselves.

@akoeplinger
Copy link
Member

Hm as a workaround you can set _RuntimeIdentifierUsesAppHost=false and UseAppHost=false.

@agocke
Copy link
Member

agocke commented May 26, 2021

Yeah, I'm not sure that's bad as an actual solution -- it seems like we just need to decide on the correct properties to set in the Xamarin targets.

@rolfbjarne
Copy link
Member Author

Hm as a workaround you can set _RuntimeIdentifierUsesAppHost=false and UseAppHost=false.

That can only be set in the csproj, because _RuntimeIdentifierUsesAppHost is evaluated before our WorkloadManifest.targets file is loaded (where we can set properties ourselves).

You can see the Import order here: https://gist.github.com/rolfbjarne/848f5c5fed4f3e1a5314232d269bb670

AutoImports.props is loaded at line 27: https://gist.github.com/rolfbjarne/848f5c5fed4f3e1a5314232d269bb670#file-gistfile1-txt-L27 - this is the first time our code is loaded, and at this point we can't set properties (https:/xamarin/xamarin-macios/blob/85ce924296eb17b4a3860b32cc94d8b4c1bfae00/dotnet/targets/Microsoft.Sdk.DefaultItems.template.props#L5-L16)

Microsoft.NET.RuntimeIdentifierInference.targets is loaded at line 52: https://gist.github.com/rolfbjarne/848f5c5fed4f3e1a5314232d269bb670#file-gistfile1-txt-L52

Our WorkloadManifest.targets is loaded at line 65: https://gist.github.com/rolfbjarne/848f5c5fed4f3e1a5314232d269bb670#file-gistfile1-txt-L65 - this is where we can set properties, but it's too late.

@agocke
Copy link
Member

agocke commented Jun 17, 2021

As we're enabling the AppHost for Mac we may be removing some usages of those properties, so I would be careful about dependencies here. Clearly UseAppHost is public, so there's no problem there, but I'm not sure what will change in our usage of _RuntimeIdentifierUsesAppHost

@steveisok
Copy link
Member

@akoeplinger what do you think the best solution is?

@vitek-karas vitek-karas added this to the 6.0.0 milestone Jul 9, 2021
@vitek-karas vitek-karas removed the untriaged New issue has not been triaged by the area owner label Jul 9, 2021
@vitek-karas
Copy link
Member

I think this should be solved in the SDK specific to Xamarin.Mac - I assume there's such a thing as the core SDK will not produce apps for it. Should we maybe move the issue to the repo which track the work on that SDK?

@akoeplinger
Copy link
Member

akoeplinger commented Jul 9, 2021

With dotnet/sdk#18639 the RuntimeIdentifierInference.targets is now imported after the workload targets so setting _RuntimeIdentifierUsesAppHost=false and UseAppHost=false should work now as a workaround.

Ideally we shouldn't need to rely on a "private" msbuild property though, an explicit UseAppHost=false setting should be enough to disable the apphost completely.

We only need the _RuntimeIdentifierUsesAppHost=false to disable this error check here: https:/dotnet/sdk/blob/e2687a3e6e5259d2167173977e64f1e9ab91f51b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets#L148-L149

@agocke sounds like if you touch this code anyway, can we just relax that check?

@agocke
Copy link
Member

agocke commented Aug 2, 2021

I think the right approach for now is to use _RuntimeIdentifierUsesAppHost. I don't think disabling that check is the right thing -- it's an important safe guard for later passes that may require the apphost.

It seems better to make _RuntimeIdentifierUsesAppHost a public property, since it's being used like one. I'm going to close this issue for now since it's been resolved (at least temporarily). If we want to come back to this, the SDK is the right repo for a new issue.

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

No branches or pull requests

5 participants