-
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
Changes from all commits
461c08e
c01ef82
35555ae
451707b
2476859
269081c
08530c2
b93dd52
6bcbb96
a3c3fad
a3675e9
ca8f5bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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 commentThe 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. |
||
</PropertyGroup> | ||
|
||
<!-- | ||
============================================================ | ||
PrepareForPublish | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. We have similar property in Would it make sense to use one property to suppress all these supported checks?
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
$(_AotSupportedRids.Contains(';$(NETCoreSdkRuntimeIdentifier);')) != 'true'" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, because |
||
ResourceName="PublishAotRidNotSupported" | ||
FormatArguments="$(NETCoreSdkRuntimeIdentifier)" /> | ||
|
||
<!-- Enable warning for trying to use PublishRelease or PackRelease with a solution if env-var is not set.--> | ||
<NETSdkWarning Condition="'$(PublishRelease)' != '' and '$(SolutionExt)' == '.sln' and '$(DOTNET_CLI_ENABLE_PUBLISH_RELEASE_FOR_SOLUTIONS)' == ''" | ||
ResourceName="PReleaseRequiresEnvVarOnSln" | ||
ResourceName="PReleaseRequiresEnvVarOnSln" | ||
FormatArguments="PublishRelease;DOTNET_CLI_ENABLE_PUBLISH_RELEASE_FOR_SOLUTIONS" | ||
/> | ||
|
||
|
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.