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

Overeager check for RuntimeIdentifier when PublishAot=true #33414

Closed
Tracked by #80905
rolfbjarne opened this issue Jun 20, 2023 · 2 comments · Fixed by #35366
Closed
Tracked by #80905

Overeager check for RuntimeIdentifier when PublishAot=true #33414

rolfbjarne opened this issue Jun 20, 2023 · 2 comments · Fixed by #35366
Assignees

Comments

@rolfbjarne
Copy link
Member

There's a check to verify that RuntimeIdentifier is set when PublishAot=true:

<NETSdkError Condition="'$(PublishAot)' == 'true' and '$(RuntimeIdentifier)' == '' and '$(_IsPublishing)' == 'true'"
ResourceName="ImplicitRuntimeIdentifierResolutionForPublishPropertyFailed"
FormatArguments="PublishAot"/>

Unfortunately this check is not valid when building universal apps for macOS and Mac Catalyst (the netX-macos and netX-maccatalyst target frameworks).

When building a universal app, the project file will set RuntimeIdentifiers (plural):

<TargetFramework>net8.0-macos</TargetFramework>
<RuntimeIdentifiers>osx-x64;osx-arm</RuntimeIdentifiers>

and then during the build, the macOS SDK will run two inner builds, with RuntimeIdentifiers unset, and RuntimeIdentifier set to each of the rids.

The problem is that the outer build, where RuntimeIdentifiers is set, but RuntimeIdentifier isn't, triggers the above-mentioned verification, and that fails the build.

I attempted to unset PublishAot for the outer build, but that doesn't work for two reasons:

  1. It can't be unset if customers pass /p:PublishAot=true on the command line.
  2. The package restore won't restore the NativeAOT packages nor include them in the build, and thus effectively NativeAOT is turned off for the inner builds as well, even if we set PublishAot=true for them.

This workaround worked around the second point:

<Target Name="EnablePublishAot" BeforeTargets="ProcessFrameworkReferences">
    <PropertyGroup>
        <_OriginalPublishAot>$(PublishAot)</_OriginalPublishAot>
        <PublishAot>true</PublishAot>
    </PropertyGroup>
</Target>

<Target Name="RestorePublishAot" AfterTargets="ProcessFrameworkReferences">
    <PropertyGroup>
        <PublishAot>$(_OriginalPublishAot)</PublishAot>
    </PropertyGroup>
</Target>

but it's on the uglier side of ugly, and not a complete solution anyways (point 1 is still a problem).

I see two potential fixes:

a. Add a property to skip this check, and set this property in the macOS SDK:

    <NETSdkError Condition="'$(PublishAot)' == 'true' and '$(RuntimeIdentifier)' == '' and '$(_IsPublishing)' == 'true' and '$(AllowPublishAotWithoutRuntimeIdentifier)' != 'true'"
                ResourceName="ImplicitRuntimeIdentifierResolutionForPublishPropertyFailed"
                FormatArguments="PublishAot"/>

b. Check for RuntimeIdentifiers (plural) as well:

    <NETSdkError Condition="'$(PublishAot)' == 'true' and '$(RuntimeIdentifier)' == '' and '$(RuntimeIdentifiers)' != 'true' and '$(_IsPublishing)' == 'true'"
                ResourceName="ImplicitRuntimeIdentifierResolutionForPublishPropertyFailed"
                FormatArguments="PublishAot"/>
@rolfbjarne
Copy link
Member Author

@nagilson I've created a PR with option a): #35366

@nagilson
Copy link
Member

Thank you for this well written issue and sorry I never got to it, it's in a list of many issues I still need to look at. Your PR seems like the best approach.

It actually used to work like #2 but that caused unexpected issues, so #1 is definitively the best choice.

@nagilson nagilson removed the untriaged Request triage from a team member label Sep 18, 2023
dsplaisted pushed a commit that referenced this issue Sep 18, 2023
This adds an escape hatch for a sanity check when trying to use PublishAot without a RuntimeIdentifier, because the check is not valid when building universal apps for macOS and Mac Catalyst (the netX-macos and netX-maccatalyst target frameworks).

When building such an app, the project file will set RuntimeIdentifiers (plural):

    <TargetFramework>net8.0-macos</TargetFramework>
    <RuntimeIdentifiers>osx-x64;osx-arm</RuntimeIdentifiers>

and then during the build, the macOS SDK will run two inner builds, with RuntimeIdentifiers unset, and RuntimeIdentifier set to each of the rids (and at the end merge the result into a single app).

The problem is that the outer build, where RuntimeIdentifiers is set, but RuntimeIdentifier isn't, triggers the sanity check, and that fails the build.

I attempted to unset PublishAot for the outer build, but that doesn't work for two reasons:

* It can't be unset if customers pass /p:PublishAot=true on the command line.
* The package restore won't restore the NativeAOT packages nor include them in the build, and thus effectively NativeAOT is turned off for the inner builds as well, even if we set PublishAot=true for them.

So instead add an escape hatch for the sanity check.

Fixes #33414.
github-actions bot pushed a commit that referenced this issue Sep 18, 2023
This adds an escape hatch for a sanity check when trying to use PublishAot without a RuntimeIdentifier, because the check is not valid when building universal apps for macOS and Mac Catalyst (the netX-macos and netX-maccatalyst target frameworks).

When building such an app, the project file will set RuntimeIdentifiers (plural):

    <TargetFramework>net8.0-macos</TargetFramework>
    <RuntimeIdentifiers>osx-x64;osx-arm</RuntimeIdentifiers>

and then during the build, the macOS SDK will run two inner builds, with RuntimeIdentifiers unset, and RuntimeIdentifier set to each of the rids (and at the end merge the result into a single app).

The problem is that the outer build, where RuntimeIdentifiers is set, but RuntimeIdentifier isn't, triggers the sanity check, and that fails the build.

I attempted to unset PublishAot for the outer build, but that doesn't work for two reasons:

* It can't be unset if customers pass /p:PublishAot=true on the command line.
* The package restore won't restore the NativeAOT packages nor include them in the build, and thus effectively NativeAOT is turned off for the inner builds as well, even if we set PublishAot=true for them.

So instead add an escape hatch for the sanity check.

Fixes #33414.
rolfbjarne added a commit to rolfbjarne/sdk that referenced this issue Oct 12, 2023
…meIdentifier.

This is identical to issue dotnet#33414 (allow PublishAot=true without RuntimeIdentifier), except for the SelfContained property instead of the PublishAot property.

This adds an escape hatch for a sanity check when trying to publish with SelfContained=true, but without a RuntimeIdentifier, because the check is not valid when building universal apps for macOS and Mac Catalyst (the netX-macos and netX-maccatalyst target frameworks).

When building such an app, the project file will set RuntimeIdentifiers (plural):

    <TargetFramework>net8.0-macos</TargetFramework>
    <RuntimeIdentifiers>osx-x64;osx-arm</RuntimeIdentifiers>

and then during the build, the macOS SDK will run two inner builds, with RuntimeIdentifiers unset, and RuntimeIdentifier set to each of the rids (and at the end merge the result into a single app).

The problem is that the outer build, where RuntimeIdentifiers is set, but RuntimeIdentifier isn't, triggers the sanity check if the developer sets SelfContained=true, and that fails the build.

Note that SelfContained defaults to true if PublishTrimmed=true, which means that just setting PublishTrimmed=true also triggers the sanity check.

Ref: xamarin/xamarin-macios#19142
github-actions bot pushed a commit that referenced this issue Oct 12, 2023
…meIdentifier.

This is identical to issue #33414 (allow PublishAot=true without RuntimeIdentifier), except for the SelfContained property instead of the PublishAot property.

This adds an escape hatch for a sanity check when trying to publish with SelfContained=true, but without a RuntimeIdentifier, because the check is not valid when building universal apps for macOS and Mac Catalyst (the netX-macos and netX-maccatalyst target frameworks).

When building such an app, the project file will set RuntimeIdentifiers (plural):

    <TargetFramework>net8.0-macos</TargetFramework>
    <RuntimeIdentifiers>osx-x64;osx-arm</RuntimeIdentifiers>

and then during the build, the macOS SDK will run two inner builds, with RuntimeIdentifiers unset, and RuntimeIdentifier set to each of the rids (and at the end merge the result into a single app).

The problem is that the outer build, where RuntimeIdentifiers is set, but RuntimeIdentifier isn't, triggers the sanity check if the developer sets SelfContained=true, and that fails the build.

Note that SelfContained defaults to true if PublishTrimmed=true, which means that just setting PublishTrimmed=true also triggers the sanity check.

Ref: xamarin/xamarin-macios#19142
github-actions bot pushed a commit that referenced this issue Dec 6, 2023
…meIdentifier.

This is identical to issue #33414 (allow PublishAot=true without RuntimeIdentifier), except for the SelfContained property instead of the PublishAot property.

This adds an escape hatch for a sanity check when trying to publish with SelfContained=true, but without a RuntimeIdentifier, because the check is not valid when building universal apps for macOS and Mac Catalyst (the netX-macos and netX-maccatalyst target frameworks).

When building such an app, the project file will set RuntimeIdentifiers (plural):

    <TargetFramework>net8.0-macos</TargetFramework>
    <RuntimeIdentifiers>osx-x64;osx-arm</RuntimeIdentifiers>

and then during the build, the macOS SDK will run two inner builds, with RuntimeIdentifiers unset, and RuntimeIdentifier set to each of the rids (and at the end merge the result into a single app).

The problem is that the outer build, where RuntimeIdentifiers is set, but RuntimeIdentifier isn't, triggers the sanity check if the developer sets SelfContained=true, and that fails the build.

Note that SelfContained defaults to true if PublishTrimmed=true, which means that just setting PublishTrimmed=true also triggers the sanity check.

Ref: xamarin/xamarin-macios#19142
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants