Make PublishAot an optional feature because Mono does not support it. #28674

tmds · 2022-10-20T07:54:23Z

Customer impact

The sdk assumes native aot is always available. This is not the case when .NET 7 is source-built with mono runtime for architectures like s390x and ppc64le.

Testing

The builds for RHEL and Fedora already include this patch.

Risk

Low, as it is already used.

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

agocke

LGTM

AntonLapounov · 2022-10-21T20:01:12Z

src/Tasks/Common/Resources/Strings.resx

@@ -875,4 +875,8 @@ You may need to build the project on another operating system or architecture, o
    <value>NETSDK1192: Targeting .NET 7.0 or higher in Visual Studio 2022 17.3 is not supported.</value>
    <comment>{StrBegin="NETSDK1192: "}</comment>
  </data>
+  <data name="AotNotSupported" xml:space="preserve">
+    <value>NETSDK1193: The SDK does not support Ahead of time compilation. Set the PublishAot property to false.</value>


Nit: Why is the initial A in Ahead capitalized? It is usually spelled "ahead-of-time compilation", (e.g., in Wikipedia).

I took the spelling from

sdk/src/Tasks/Common/Resources/Strings.resx

Line 839 in 1624c6a

<value>NETSDK1183: Unable to optimize assemblies for Ahead of time compilation: a valid runtime package was not found. Either set the PublishAot property to false, or use a supported runtime identifier when publishing and make sure to restore packages with the PublishAot property set to true.</value>

@AntonLapounov should I update both values?

Sorry, missed the question. I would update both values.

I've replaced it everywhere where the language is English. I left the other ones untouched.

Strings.es.xlf: <target state="translated">NETSDK1183: No se pueden optimizar los ensamblados para la compilación Ahead of time: no se ha encontrado un paquete en tiempo de ejecución válido. Establezca la propiedad PublishAot en false o use un identificador de tiempo de ejecución compatible al publicar. Cuando el destino sea .NET 7 o una versión posterior, asegúrese de restaurar los paquetes con la propiedad PublishAot establecida en true.</target> Strings.fr.xlf: <target state="translated">NETSDK1183: Impossible d'optimiser les assemblys pour la compilation Ahead of time : un package d'exécution valide n'a pas été trouvé. Définissez la propriété PublishAot sur false ou utilisez un identificateur d'exécution pris en charge lors de la publication. Lorsque vous ciblez .NET 7 ou supérieur, assurez-vous de restaurer les packages avec la propriété PublishAot définie sur true.</target> Strings.it.xlf: <target state="translated">NETSDK1183: non è possibile ottimizzare gli assembly per la compilazione Ahead Of Time perché non è stato trovato alcun pacchetto di runtime valido. Impostare la proprietà PublishAot su false oppure usare un identificatore di runtime supportato durante la pubblicazione. Quando si usa .NET 7 o versioni successive, assicurarsi di ripristinare i pacchetti con la proprietà PublishAot impostata su true.</target> Strings.ja.xlf: <target state="translated">NETSDK1183: Ahead Of Time コンパイル用にアセンブリを最適化できません: 有効なランタイムパッケージが見つかりませんでした。PublishAot プロパティを false に設定するか、公開時に、サポートされているランタイム識別子を使用してください。.NET 7 以降を対象とする場合は、必ず PublishAot プロパティを true に設定してパッケージを復元してください。</target> Strings.ko.xlf: <target state="translated">NETSDK1183: Ahead of Time 컴파일을 위해 어셈블리를 최적화할 수 없습니다. 유효한 런타임 패키지를 찾을 수 없습니다. PublishAot 속성을 false로 설정하거나 게시할 때 지원되는 런타임 식별자를 사용하세요. .NET 7 이상을 대상으로 하는 경우 PublishAot 속성이 true로 설정된 패키지를 복원해야 합니다.</target> Strings.ru.xlf: <target state="translated">NETSDK1183: не удалось оптимизировать сборки для компиляции Ahead Of Time: не найден допустимый пакет среды выполнения. Задайте для свойства PublishAot значение false либо используйте поддерживаемый идентификатор среды выполнения при публикации. При выборе .NET 7 или более поздней версии в качестве цели восстановите пакеты со свойством PublishAot со значением true.</target>

crummel

LGTM from the source-build perspective.

tmds · 2022-11-03T14:06:00Z

@agocke @crummel #28690 has invalidated my PR.
Should I target the 7.0 branch instead?

tmds · 2022-11-07T10:51:10Z

Should I target the 7.0 branch instead?

What branch should I target? 7.0.1xx? Will that get merged to 7.0.2xx?

For the main branch, because #28690 removes the BundledSdk, we should be able to compile for mono runtime with the branch as-is.
There won't be a nice error message when a user tries to publish aot, but we can look into that later.
Should I make a PR against main that adds NETSDK1193 so the identifier doesn't get used for anything else?

MichaelSimons · 2022-11-07T14:40:27Z

What branch should I target? 7.0.1xx? Will that get merged to 7.0.2xx?

I would say 7.0.1xx. Changes to 7.0.1xx will get auto merged (via PR) to the higher feature bands (e.g. 7.0.2xx)

tmds · 2022-11-09T12:42:29Z

I've changed this to target 7.0.1xx.

@agocke when CI is green, can you merge this?

For main we don't need anything atm, except perhaps 'reserving' the NETSDK1193 error. Should I make another PR for that?

agocke · 2022-11-10T18:24:52Z

@tmds This will need to go through servicing approval for 7.0. Have you already done that?

tmds · 2022-11-14T07:41:13Z

@tmds This will need to go through servicing approval for 7.0. Have you already done that?

@agocke usually maintainers take care of this. I've updated the top-level comment with some additional information.

tmds · 2022-11-17T08:26:22Z

Is this good to merge?

agocke · 2022-11-17T18:30:10Z

Unfortunately couldn't get to it in Tactics today. I'll try to do this over email.

ayakael · 2022-11-27T18:12:49Z

@tmds Have you encountered any errors building SDK with this patch. Our Alpine build on armv7 fails when building on itself:

  Determining projects to restore...
    /home/buildozer/aports/community/dotnet7-build/src/dotnet-v7.0.100-rtm.22521.12/src/sdk/artifacts/source-build/self/src/src/Layout/redist/targets/sdks/sdks.csproj : error NU1101: Unable to find package Microsoft.DotNet.ILCompiler. No packages exist with this id in source(s): prebuilt, previously-source-built, reference-packages, source-built
    ##vso[task.logissue type=error;sourcepath=/home/buildozer/aports/community/dotnet7-build/src/dotnet-v7.0.100-rtm.22521.12/src/sdk/artifacts/source-build/self/src/src/Layout/redist/targets/sdks/sdks.csproj;linenumber=0;columnnumber=0;code=NU1101;](NETCORE_ENGINEERING_TELEMETRY=Build) Unable to find package Microsoft.DotNet.ILCompiler. No packages exist with this id in source(s): prebuilt, previously-source-built, reference-packages, source-built
      Failed to restore /home/buildozer/aports/community/dotnet7-build/src/dotnet-v7.0.100-rtm.22521.12/src/sdk/artifacts/source-build/self/src/src/Layout/redist/targets/sdks/sdks.csproj (in 38 ms)

ILCompiler nupkg doesn't get added in the artifacts tarball for some reason.

tmds · 2022-11-27T18:29:08Z

@tmds Have you encountered any errors building SDK with this patch. Our Alpine build on armv7 fails when building on itself:

Is this using mono runtime?

I verified this patch works in builds with coreclr runtime and with mono runtime

With mono runtime, the ILCompiler package doesn't get built, and this patch makes the necessary changes for that.

ayakael · 2022-11-27T18:32:56Z

@tmds Have you encountered any errors building SDK with this patch. Our Alpine build on armv7 fails when building on itself:

Is this using mono runtime?

I verified this patch works in builds with coreclr runtime and with mono runtime

With mono runtime, the ILCompiler package doesn't get built, and this patch makes the necessary changes for that.

Debugging further has confirmed that this patch isn't the cause. Coreclr-flavored runtime build on armv7 never builds runtime.alpine.3.17-arm.Microsoft.DotNet.ILCompiler.7.0.0.nupkg thus the SDK never picks up Microsoft.DotNet.ILCompiler on the first bootstrap build.

ayakael · 2022-11-28T03:15:41Z

eng/SourceBuild.props

@@ -8,6 +8,7 @@
    <PropertyGroup>
      <InnerBuildArgs>$(InnerBuildArgs) /p:Projects="$(InnerSourceBuildRepoRoot)\source-build.slnf"</InnerBuildArgs>
      <InnerBuildArgs>$(InnerBuildArgs) /p:UseSharedCompilation=false</InnerBuildArgs>
+      <InnerBuildArgs Condition="'$(SourceBuildUseMonoRuntime)' == 'true'">$(InnerBuildArgs) /p:NativeAotSupported=false</InnerBuildArgs>


Some coreclr-supported platforms do not support NativeAot as well (arm). Does SDK have a platform variable? I couldn't find any thus I had to find another way to set NativeAotSupported when arch is arm. On Alpine, I removed this part of the modification, and moved the check to repos/sdk.proj as $(BuildArchitecture) does not reach eng/SourceBuild.props.

Of course, you can keep this check and source-build can set NativeAotSupported=false when $(BuildArchitecture) == arm.

Thus, in dotnet/installer#14792:

diff --git a/src/SourceBuild/tarball/content/repos/sdk.proj b/src/SourceBuild/tarball/content/repos/sdk.proj index c18e00dd61..6567b41983 100644 --- a/src/SourceBuild/tarball/content/repos/sdk.proj +++ b/src/SourceBuild/tarball/content/repos/sdk.proj @@ -6,6 +6,7 @@ <BuildCommandArgs>$(BuildCommandArgs) $(FlagParameterPrefix)nodereuse $(ArcadeFalseBoolBuildArg)</BuildCommandArgs> <BuildCommandArgs>$(BuildCommandArgs) /p:PackageProjectUrl=https://github.com/dotnet/sdk</BuildCommandArgs> <BuildCommandArgs>$(BuildCommandArgs) /p:PublishCompressedFilesPathPrefix=$(SourceBuiltToolsetDir)</BuildCommandArgs> + <BuildCommandArgs Condition="'$(SourceBuildUseMonoRuntime)' == 'true' OR '$(BuildArchitecture)' == 'arm'">$(BuildCommandArgs) /p:NativeAotSupported=false</BuildCommandArgs> <LogVerbosityOptOut>true</LogVerbosityOptOut> <BuildCommandArgs>$(BuildCommandArgs) -v $(LogVerbosity)</BuildCommandArgs>

Does SDK have a platform variable?

I don't think so. This approach looks fine to me.

marcpopMSFT · 2023-01-11T22:17:19Z

@agocke @crummel this was approved by tactics for 7.0.3 which is open now. Can this be merged and if so, it needs to be this week?

crummel · 2023-01-12T20:50:10Z

eng/SourceBuild.props

@@ -8,6 +8,7 @@
    <PropertyGroup>
      <InnerBuildArgs>$(InnerBuildArgs) /p:Projects="$(InnerSourceBuildRepoRoot)\source-build.slnf"</InnerBuildArgs>
      <InnerBuildArgs>$(InnerBuildArgs) /p:UseSharedCompilation=false</InnerBuildArgs>
+      <InnerBuildArgs Condition="'$(SourceBuildUseMonoRuntime)' == 'true'">$(InnerBuildArgs) /p:NativeAotSupported=false</InnerBuildArgs>


Does SDK have a platform variable?

I don't think so. This approach looks fine to me.

crummel · 2023-01-12T21:00:35Z

@agocke @crummel this was approved by tactics for 7.0.3 which is open now. Can this be merged and if so, it needs to be this week?

This LGTM to merge.

dotnet-issue-labeler bot added the Area-Infrastructure label Oct 20, 2022

tmds commented Oct 20, 2022

View reviewed changes

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

tmds requested review from AntonLapounov and a team as code owners October 21, 2022 08:24

tmds mentioned this pull request Oct 21, 2022

Enabled source-building with mono runtime on any architecture. dotnet/installer#14792

Merged

dsplaisted approved these changes Oct 21, 2022

View reviewed changes

agocke approved these changes Oct 21, 2022

View reviewed changes

AntonLapounov reviewed Oct 21, 2022

View reviewed changes

crummel approved these changes Oct 26, 2022

View reviewed changes

tmds changed the base branch from main to release/7.0.1xx November 9, 2022 12:37

tmds requested review from ericstj, a team, joperezr, captainsafia, mkArtakMSFT, TanayParikh and javiercn as code owners November 9, 2022 12:37

tmds added 2 commits November 9, 2022 13:39

Make PublishAot an optional feature because Mono does not support it.

b1d8509

Move error check to Publish.targets.

d4f48a3

tmds force-pushed the mono_no_aot branch from 1624c6a to d4f48a3 Compare November 9, 2022 12:39

ayakael mentioned this pull request Nov 12, 2022

.NET 7: Alpine backporting dotnet/source-build#3115

Closed

16 tasks

crummel added the Servicing-consider label Nov 15, 2022

Spell as 'ahead-of-time'

23b21de

vlada-shubina removed the request for review from a team November 21, 2022 09:48

ayakael reviewed Nov 28, 2022

View reviewed changes

ayakael mentioned this pull request Nov 28, 2022

[release/7.0.1xx] repos/sdk.proj: set NativeAotSupported=false on arm platform dotnet/installer#15039

Merged

rbhanda added Servicing-approved and removed Servicing-consider labels Nov 29, 2022

rbhanda added this to the 7.0.3 milestone Nov 29, 2022

crummel approved these changes Jan 12, 2023

View reviewed changes

crummel merged commit b8ec9f7 into dotnet:release/7.0.1xx Jan 13, 2023

tmds mentioned this pull request Jan 18, 2023

[automated] Merge branch 'release/7.0.2xx' => 'main' #29980

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make PublishAot an optional feature because Mono does not support it. #28674

Make PublishAot an optional feature because Mono does not support it. #28674

tmds commented Oct 20, 2022 •

edited

Loading

agocke left a comment

AntonLapounov Oct 21, 2022

tmds Oct 22, 2022

tmds Oct 25, 2022

AntonLapounov Nov 17, 2022

tmds Nov 18, 2022

crummel left a comment

tmds commented Nov 3, 2022

tmds commented Nov 7, 2022

MichaelSimons commented Nov 7, 2022

tmds commented Nov 9, 2022

agocke commented Nov 10, 2022

tmds commented Nov 14, 2022

tmds commented Nov 17, 2022

agocke commented Nov 17, 2022

ayakael commented Nov 27, 2022

tmds commented Nov 27, 2022

ayakael commented Nov 27, 2022 •

edited

Loading

ayakael Nov 28, 2022

crummel Jan 12, 2023

marcpopMSFT commented Jan 11, 2023

crummel Jan 12, 2023

crummel commented Jan 12, 2023

Make PublishAot an optional feature because Mono does not support it. #28674

Make PublishAot an optional feature because Mono does not support it. #28674

Conversation

tmds commented Oct 20, 2022 • edited Loading

Customer impact

Testing

Risk

agocke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crummel left a comment

Choose a reason for hiding this comment

tmds commented Nov 3, 2022

tmds commented Nov 7, 2022

MichaelSimons commented Nov 7, 2022

tmds commented Nov 9, 2022

agocke commented Nov 10, 2022

tmds commented Nov 14, 2022

tmds commented Nov 17, 2022

agocke commented Nov 17, 2022

ayakael commented Nov 27, 2022

tmds commented Nov 27, 2022

ayakael commented Nov 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcpopMSFT commented Jan 11, 2023

Choose a reason for hiding this comment

crummel commented Jan 12, 2023

tmds commented Oct 20, 2022 •

edited

Loading

ayakael commented Nov 27, 2022 •

edited

Loading