-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Allow building with PublishAot set #28495
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! ITs good that users can bypass the rid check in the SDK if needed when ILC packages with new rids get added
@@ -82,6 +82,10 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
</PropertyGroup> | |||
</Target> | |||
|
|||
<PropertyGroup> | |||
<_AotSupportedRids>linux-x64;linux-arm64;linux-musl-x64;linux-musl-arm64;windows-x64;windows-arm64</_AotSupportedRids> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right - I meant that we should update that list instead of adding another one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
@@ -81,6 +81,7 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
|
|||
<PropertyGroup> | |||
<_NuGetRestoreSupported Condition="('$(Language)' == 'C++' and '$(_EnablePackageReferencesInVCProjects)' != 'true')">false</_NuGetRestoreSupported> | |||
<_AotEnabled Condition="('$(PublishAot)' == 'true') and ('$(_IsPublishing)' == 'true')">true</_AotEnabled> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation on what I should use instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
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
Outdated
Show resolved
Hide resolved
@@ -82,6 +82,10 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
</PropertyGroup> | |||
</Target> | |||
|
|||
<PropertyGroup> | |||
<_AotSupportedRids>linux-x64;linux-arm64;linux-musl-x64;linux-musl-arm64;windows-x64;windows-arm64</_AotSupportedRids> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
Also upgrades global.json as the build pulls the BundledVersion file from the toolset SDK.
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 |
@@ -111,10 +115,15 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
'$(EnableCompressionInSingleFile)' == 'true' And | |||
'$(SelfContained)' != 'true'" | |||
ResourceName="CompressionInSingleFileRequiresSelfContained" /> | |||
<NETSdkError Condition="'$(PublishAot)' == 'true' And | |||
'$(_SkipAotSupportedRidCheck)' != 'true' And |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
toSkipAotSupportedCheck
in dotnet/runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you agree that we should use the same property name in all places, the property should be renamed in this servicing fix.
@sbomer Anything else here? |
// 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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to have browser-wasm
here for NativeAOT-LLVM support.
@@ -82,6 +82,10 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
</PropertyGroup> | |||
</Target> | |||
|
|||
<PropertyGroup> | |||
<_AotSupportedRids>;linux-x64;linux-arm64;linux-musl-x64;linux-musl-arm64;win-x64;win-arm64;</_AotSupportedRids> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -111,10 +115,15 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
'$(EnableCompressionInSingleFile)' == 'true' And | |||
'$(SelfContained)' != 'true'" | |||
ResourceName="CompressionInSingleFileRequiresSelfContained" /> | |||
<NETSdkError Condition="'$(PublishAot)' == 'true' And | |||
'$(_SkipAotSupportedRidCheck)' != 'true' And | |||
$(_AotSupportedRids.Contains(';$(NETCoreSdkRuntimeIdentifier);')) != 'true'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because osx-x64
has an ILCompilerPack, but isn't supported.
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. |
Fixes #28475
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.