Skip to content
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

[NativeAOT] Clean up AOT Publishing #72415

Closed
LakshanF opened this issue Jul 18, 2022 · 7 comments · Fixed by #73416
Closed

[NativeAOT] Clean up AOT Publishing #72415

LakshanF opened this issue Jul 18, 2022 · 7 comments · Fixed by #73416
Assignees
Milestone

Comments

@LakshanF
Copy link
Member

LakshanF commented Jul 18, 2022

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,

  • The original code was added to only support explicit package references
  • SDK support was added for PublishAot with backward compatibility support (for a short period) for the explicit package reference without the SDK path
  • The current support allows SDK only as well as to add explicit package reference with SDK PublishAot. Explicit package reference takes precedence when one exists (for example, to pick up bug fixes in the package earlier)

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

Specifically,

Regarding removing IlcCalledViaPackage, see this comment and dotnet/corert#5123

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 18, 2022
@LakshanF LakshanF self-assigned this Jul 19, 2022
@MichalStrehovsky
Copy link
Member

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

@LakshanF LakshanF removed the untriaged New issue has not been triaged by the area owner label Aug 4, 2022
@LakshanF LakshanF added this to the 7.0.0 milestone Aug 4, 2022
@LakshanF
Copy link
Member Author

LakshanF commented Aug 4, 2022

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.

  • The PublishAot property is set to true in the project or elsewhere AND no explicit reference to the build package in the application.
    • The build package that was bundled with the SDK will be used
    • Target ProcessFrameworkReferences will use version information - $(MicrosoftNETCoreAppRuntimePackageVersion),
      set in GenerateBundledVersions.targets - to download the runtime package
    • AOT Source analyzer will be used during build
    • A telemetry signal will be triggered to indicate an AOT publish action has happened
  • The PublishAot property is set to true in the project or elsewhere AND an explicit reference to the build package is in the application.
    • This scenario will generate a warning but is supported to let developers get a specific version of the ILCompiler package. For example, to get a bug fix that is not yet present in the SDK ILCompiler package versions.
    • The build package that was restored by NuGet will be used
    • Target ProcessFrameworkReferences will use the same version as the build package version information to download the runtime package
    • AOT Source analyzer will be used during build
    • A telemetry signal will be triggered to indicate an AOT publish action has happened
  • PublishAot property is not set AND an explicit reference to the build package is in the application
    • This scenario is not supported to publish a native AOT application
    • No warnings will be generated and a 'normal' publish will happen
  • Cross target publishing support as described here
    • PublishAOT property setting as described above
    • Target ProcessFrameworkReferences will use the same version as the build package version information to also download a runtime package that matches the target
  • PublishAot property is set to false
    • No AOT related publishing will be done
  • Build process
    • Build should be successful, but no post IL processing will be done
    • AOT Source analyzer will be used during build process

These scenarios impose the following requirements

  • Distinguish build package loaded location. Specifically, the build package loaded location to be either via SDK or via NuGet (via an explicit reference) needs to be known.
  • SDK to know the version of the build package that is getting used. This information is needed so as to download the same version runtime packages.
  • Build and runtime packages to know the final root path information of the runtime package that is used. The root path is used to set the full path to the native AOT tools.

These requirements are currently implemented in the following way

  • Set a sentinel to the directory where the package is by the first load. Use that package location for loading all related package assets.
  • Build package assets to detect if the build package is from NuGet and if so, to pass the version information back to SDK prior to the task where SDK downloads runtime packages.
  • SDK will pass to the build package the root path of the runtime package that will be used

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

  • The above plan also needs to work with running existing AOT tests in the SDK repo. Specifically, the SDK needs to support downloading cross-target runtime packages and passing that information to runtime targets.
  • Currently, the PublishAOT package assets are not hardened to work if any non-NativeAOT assets access them (unlike the trimming assets).
  • Corresponding PublishAOT changes to match the runtime cleanup changes sdk#27159 tracks these changes.

@sbomer
Copy link
Member

sbomer commented Aug 4, 2022

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.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 4, 2022
@MichalStrehovsky MichalStrehovsky modified the milestones: 7.0.0, 8.0.0 Aug 12, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 12, 2022
@MichalStrehovsky MichalStrehovsky removed the untriaged New issue has not been triaged by the area owner label Aug 12, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 15, 2022
@MichalStrehovsky
Copy link
Member

The PR is getting reverted.

@MichalStrehovsky
Copy link
Member

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

@LakshanF
Copy link
Member Author

LakshanF commented Aug 18, 2022

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
Copy link
Member Author

LakshanF commented Aug 22, 2022

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

@ghost ghost locked as resolved and limited conversation to collaborators Sep 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants