dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.16k stars 4.71k forks source link

[NativeAOT] Clean up AOT Publishing #72415

Closed LakshanF closed 2 years ago

LakshanF commented 2 years ago

Given that building a native AOT application is supported only via the SDK (with the option to get the latest ILCompiler* packs via an explicit reference), the publish path with properties, items, tasks, and targets etc., should be cleaned up. The history of this code has been with specific goals that have changed over time,

It should be easy to follow the publishing logic which currently is a little convoluted.

Specifically,

Regarding removing IlcCalledViaPackage, see this comment and https://github.com/dotnet/corert/pull/5123

MichalStrehovsky commented 2 years ago

@LakshanF could you milestone the issue and remove untriaged? It shows up in queries.

LakshanF commented 2 years ago

Incorporating @sbomer suggestions below

The native AOT application publishing experience is outlined below for supported scenarios for PublishAot property and when an explicit package reference to Micsoroft.dotnet.ilcompiler is present

Publishing a native AOT application requires 2 external packages, Micsoroft.dotnet.ilcompiler (hereafter referred as build package) and platform dependent one, runtime.<RID>.microsoft.dotnet.ilcompiler (hereafter referred as runtime package).

The options below take different paths to acquire the packages.

These scenarios impose the following requirements

These requirements are currently implemented in the following way

Runtime repo impact The above plan requires taking into account that the package targets will be imported in dotnet build. Specifically, no publish related AOT targets will be invoked during the build process. In addition, Building and running native AOT tests in the runtime repo will continue to work as before. The following repo assets are impacted,

SDK repo impact

sbomer commented 2 years ago

I think we should simplify the third scenario to behave either identically to the second (as if PublishAot were set to true), or to not do an AOT publish at all. Since the OOB package scenario is already in use, I would probably make it behave like PublishAot is true.

Setting PublishAot property to false is not supported

I think that setting PublishAot to false should prevent AOT publish, turn off the AOT analyzer, etc, even if there is a package reference.

MichalStrehovsky commented 2 years ago

The PR is getting reverted.

MichalStrehovsky commented 2 years ago

@LakshanF you still had some concerns around this in the meeting. Should we keep open or it's good to close?

LakshanF commented 2 years ago

I would like to keep this open for a little longer. I need to clean up the tests in SDK and that requires the runtime changes to flow there. Will do that soon.

LakshanF commented 2 years ago

Validated the test in the rc1 branch, including cross-target scenario without the explicit packages in the project. However, the test will only be updated on the main branch and not on the RC1 branch due to the high bar for changes in the RC1