Allow building with PublishAot set by agocke · Pull Request #28495 · dotnet/sdk

agocke · 2022-10-12T05:20:43Z

The current PublishAot behavior is to attempt to restore the runtime pack for NativeAOT immediately. This doesn't work for osx-arm64, since there's no runtime pack for that RID. This causes even restore to fail, when it should succeed (as should build).

The new behavior is to only attempt to restore the runtime pack when publish is actually being run. An error message is also printed if an unsupported RID is used, although an "escape hatch" property is also provided to skip that error.

ghost · 2022-10-12T05:20:52Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

LakshanF

LGTM! ITs good that users can bypass the rid check in the SDK if needed when ILC packages with new rids get added

sbomer · 2022-10-13T16:38:40Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets

  </Target>

+  <PropertyGroup>
+    <_AotSupportedRids>linux-x64;linux-arm64;linux-musl-x64;linux-musl-arm64;windows-x64;windows-arm64</_AotSupportedRids>


There's a similar list here: https://github.com/dotnet/installer/blob/main/src/redist/targets/GenerateBundledVersions.targets#L262-L263 - I think we should keep the list in just one place.

That list isn't correct -- it includes the macos RIDs. I need the real list here, as I want to provide an error for mac, not just let it go.

Right - I meant that we should update that list instead of adding another one.

OK, this logic is getting convoluted, but I think this split is what we want now.

We want the full list in restore, so that restore will try to grab it.

But we want the restricted list here, so that you get a clear error when you're doing something unsupported.

I might be missing something though.

Why do we want restore to try to grab it if it's unsupported? I'd still rather have one source of truth for the supported rids, and provide a way to disable that check if needed.

So, the experience I think we want (based on previous changes in this code) is:

PublishAot on Mac = error unsupported.

PublishAot on Mac w/ explicit bypass = works.

If that's true, the list of acceptable RIDs in (1) does not include osx, but it does in (2). I don't see how those could be the same list.

I don't quite follow - what's to prevent us from having a single list just for (1), and then not check against any list in (2)?

sbomer · 2022-10-13T16:40:24Z

...sks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets


    <PropertyGroup>
      <_NuGetRestoreSupported Condition="('$(Language)' == 'C++' and '$(_EnablePackageReferencesInVCProjects)' != 'true')">false</_NuGetRestoreSupported>
+      <_AotEnabled Condition="('$(PublishAot)' == 'true') and ('$(_IsPublishing)' == 'true')">true</_AotEnabled>


What happens if you do a dotnet restore with a supported RID? Does that still download the runtime pack that would be required to publish?

FYI _IsPublishing is only for CLI scenarios ATM, we are discussing if we can get it to work in VS Publishing today. We don't think we can get it to work for t:/Publish and plan on evaluating it in a Monday partner sync or in a separate call, I was planning on adding you @agocke

cc @rainersigwald

Recommendation on what I should use instead?

I'm sorry to say I don't have any immediate recommendations, I am curious as to how PublishTrimmed, single file, aot, etc currently work. The dilemma we're facing right now is detecting if we are publishing early in evaluation seems to be incompatible with MSBuild as it currently exists, because of custom Publish targets that call publish later on, --no-build publishes, etc. It seems like it should be so simple and something we really want; we have been going back and forth on this for a few months now!

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets

agocke · 2022-10-13T18:51:27Z

Talking with @sbomer offline, maybe the right answer is just to not fail restore at all. We can give a better error message later.

src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs

sbomer · 2022-10-13T20:13:03Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets

  </Target>

+  <PropertyGroup>
+    <_AotSupportedRids>linux-x64;linux-arm64;linux-musl-x64;linux-musl-arm64;windows-x64;windows-arm64</_AotSupportedRids>


Why do we want restore to try to grab it if it's unsupported? I'd still rather have one source of truth for the supported rids, and provide a way to disable that check if needed.

sbomer · 2022-10-13T20:18:36Z

I wanted to write down the scenario we discussed where not erroring during restore could be unexpected:

If I do a RID-specific restore with PublishAot set in my project, and it succeeds, then I expect to be able to do an offline dotnet publish --no-restore later. I can't say with certainty that this is an intentionally supported scenario, but I do see some value in producing the error up-front instead of waiting until publish.

…t-app

Also upgrades global.json as the build pulls the BundledVersion file from the toolset SDK.

…st anyway

agocke · 2022-10-14T19:09:41Z

Yup, the decision in this PR is that restore will never fail, even if the eventual publish certainly will.

I think this PR now represents a workable solution. Publish will, by default, check that the RID is supported and error if it is not.

If the check is bypassed, publish will attempt to use a downloaded package (which will be downloaded during restore if we've added it to the list of available packages, or if the user adds a new KnownILCompilerPack).

jkotas · 2022-10-20T04:00:59Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets

                            '$(SelfContained)' != 'true'"
                 ResourceName="CompressionInSingleFileRequiresSelfContained" />
+    <NETSdkError Condition="'$(PublishAot)' == 'true' And
+                            '$(_SkipAotSupportedRidCheck)' != 'true' And


We have similar property in
https://github.com/dotnet/runtime/blob/cb6fb60dfc253bc13655fafa15686cb864fd0ea2/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Publish.targets#L58 .

Would it make sense to use one property to suppress all these supported checks?

Rename _SkipAotSupportedRidCheck -> SkipAotSupportedCheck here

Rename DisableUnsupportedError to SkipAotSupportedCheck in dotnet/runtime.

That makes sense, but I'd like to do that as a separate change, as I intend for this to go into .NET 7 as a servicing fix.

If you agree that we should use the same property name in all places, the property should be renamed in this servicing fix.

agocke · 2022-10-20T17:20:42Z

@sbomer Anything else here?

dsplaisted · 2022-10-20T17:46:48Z

src/Layout/toolset-tasks/OverrideAndCreateBundledNETCoreAppPackageVersion.cs

+            // produce, as if don't try to download a missing package then restore will succeed and
+            // we can give a better error during publish that the given RID is not supported.
+            var ilcompilerRids = itemGroup.Elements(ns + "KnownILCompilerPack").First().Attribute("ILCompilerRuntimeIdentifiers");
+            ilcompilerRids.Value = "linux-x64;linux-musl-x64;linux-arm64;linux-musl-arm64;win-x64;win-arm64;osx-x64";


Ideally we shouldn't hard-code the supported RuntimeIdentifiers in a C# task. What's the reason for the mismatch between the values coming from the item and what's actually supported?

The mismatch is that we were trying to avoid blocking people who wanted to use unsupported, but available packages.

But being too broad here actually causes problems because attempting to download a missing package blocks restore/build.

We may want to have browser-wasm here for NativeAOT-LLVM support.

dsplaisted · 2022-10-20T17:49:50Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets

  </Target>

+  <PropertyGroup>
+    <_AotSupportedRids>;linux-x64;linux-arm64;linux-musl-x64;linux-musl-arm64;win-x64;win-arm64;</_AotSupportedRids>


Usually when we add support for new RuntimeIdentifiers that ends up getting authored in the installer repo and flowing into items such as the KnownFrameworkReference or other items in Microsoft.NETCoreSdk.BundledVersions.props. Should we do that for this? How would we support it if we add support for new RuntimeIdentifiers for AoT, and the list of supported RIDs depends on the target framework?

Note that the list above (which is the supported list that flows from /installer), is not quite the same as the supported rids here, namely that osx-x64 is in the "to download" list but not in the "supported" list.

The only thing that matters for unsupported architectures is that there is some way for people who want to give it try. It does not need to be pretty. It is ok if it requires extra boilerplate like explicit reference to the osx package.

Does the new SkipAotSupportedCheck property enable that? If yes, I think it would be fine to get rid of this special-casing for available, but officially unsupported architectures.

dsplaisted · 2022-10-20T17:52:57Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets

                 ResourceName="CompressionInSingleFileRequiresSelfContained" />
+    <NETSdkError Condition="'$(PublishAot)' == 'true' And
+                            '$(_SkipAotSupportedRidCheck)' != 'true' And
+                            $(_AotSupportedRids.Contains(';$(NETCoreSdkRuntimeIdentifier);')) != 'true'"


Instead of checking whether a string contains the RuntimeIdentifier, shouldn't we be checking to see whether there is a valid HostILCompilerPack or TargetILCompilerPack instead for this error?

No, because osx-x64 has an ILCompilerPack, but isn't supported.

agocke · 2022-10-31T20:47:14Z

Started thinking about this and realized that having a single property for RIDs doesn't make sense as it needs to be per-TFM. I'll try to rewrite this. The implicit RID changes are also interesting --we might want to backport that.

This was referenced Oct 12, 2022

Tracking Nuget 429s dotnet/arcade#10885

Closed

Nuget restore error caused by 503s dotnet/arcade#11239

Closed

agocke changed the base branch from main to release/7.0.1xx October 13, 2022 07:10

agocke added 4 commits October 13, 2022 00:10

Add unit test

461c08e

Only set AotEnabled for ProcessFrameworkReferences when publishing

c01ef82

Update existing test to run on all platforms

35555ae

Add error for unsupported RIDs

451707b

agocke force-pushed the build-aot-app branch from e2e2afc to 451707b Compare October 13, 2022 07:10

agocke changed the title ~~Add unit test~~ Allow building with PublishAot set Oct 13, 2022

agocke marked this pull request as ready for review October 13, 2022 07:14

agocke requested review from a team and AntonLapounov as code owners October 13, 2022 07:14

agocke requested review from LakshanF and sbomer October 13, 2022 07:14

LakshanF approved these changes Oct 13, 2022

View reviewed changes

sbomer reviewed Oct 13, 2022

View reviewed changes

AntonLapounov reviewed Oct 13, 2022

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets Outdated Show resolved Hide resolved

Respond to PR comments and stop failing runtimepack restore

2476859

sbomer reviewed Oct 13, 2022

View reviewed changes

agocke added 6 commits October 13, 2022 16:23

Merge remote-tracking branch 'upstream/release/7.0.1xx' into build-ao…

269081c

…t-app

Add test that checks if the bypass workds

08530c2

Also upgrades global.json as the build pulls the BundledVersion file from the toolset SDK.

Fixup KnownILCompilerPack RIDs

b93dd52

Revert global.json change as we're just going to overwrite the RID li…

6bcbb96

…st anyway

Fix comment

a3c3fad

Remove duplicate test

a3675e9

Windows RID typo

ca8f5bf

jkotas reviewed Oct 20, 2022

View reviewed changes

jkotas mentioned this pull request Oct 20, 2022

Improve experience when using PublishAot with unsupported RID dotnet/runtime#73678

Closed

dsplaisted reviewed Oct 20, 2022

View reviewed changes

Thefrank mentioned this pull request Oct 25, 2022

Support for FreeBSD dotnet/runtime#14537

Closed

agocke marked this pull request as draft October 31, 2022 20:45

SingleAccretion mentioned this pull request Mar 23, 2023

[NativeAOT LLVM] Using together with .NET 8 SDK dotnet/runtimelab#2235

Open

agocke closed this Jul 21, 2023

agocke deleted the build-aot-app branch July 21, 2023 05:35

agocke restored the build-aot-app branch July 21, 2023 05:35

Conversation

agocke commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 12, 2022

Uh oh!

LakshanF left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagilson Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

agocke commented Oct 13, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbomer commented Oct 13, 2022

Uh oh!

agocke commented Oct 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke commented Oct 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke commented Oct 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

agocke commented Oct 12, 2022 •

edited

Loading

nagilson Oct 13, 2022 •

edited

Loading