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-LLVM: Reinstate SingleEntry.targets #2248

Merged

Conversation

yowl
Copy link
Contributor

@yowl yowl commented Apr 7, 2023

This PR, adds back SingleEntry.targets to src\coreclr\nativeaot\BuildIntegration\Microsoft.DotNet.ILCompiler.LLVM.targets and basically copies part of what was NeedNativePublishSupportForSDK6 to NeedNativePublishSupportForLLVM. It also adds a call to ResolvePackageDependencies to get PackageDefinitions set.

Minor version update to the docs.

As I've replaced NeedNativePublishSupportForSDK6 I'm basically saying we are not supporting .net SDK 6.

Hopefully this will fix dotnet publish --self-contained -r browser-wasm, it does so locally:

E:\tmp\consolelocal>dotnet publish --self-contained -r browser-wasm /p:MSBuildEnableWorkloadResolver=false /bl
MSBuild version 17.5.0+6f08c67f3 for .NET
  Determining projects to restore...
  Restored E:\tmp\consolelocal\console.csproj (in 2.87 sec).
  console -> E:\tmp\consolelocal\bin\Debug\net7.0\browser-wasm\console.dll
  Generating native code
  LLVM bitcode generation finished in 69.78 seconds
  console -> E:\tmp\consolelocal\bin\Debug\net7.0\browser-wasm\publish\

E:\tmp\consolelocal>e:\github\emsdk\node\14.18.2_64bit\bin\node --stack-trace-limit=100 bin\Debug\net7.0\browser-wasm\native\console.js
Hello, World!
True

Call ResolvePackageDependencies to get PackageDefinitions
@yowl yowl force-pushed the reinstate-package-resolve branch from f12b8bb to 58e71f5 Compare April 7, 2023 21:54
@yowl yowl marked this pull request as ready for review April 7, 2023 23:26
@yowl
Copy link
Contributor Author

yowl commented Apr 7, 2023

Cc @dotnet/nativeaot-llvm

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should fix #2235. cc @kant2002

Looks good to me. @SingleAccretion Any additional feedback?

Copy link

@SingleAccretion SingleAccretion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a note somewhere in the user-facing documentation that PublishAot=true is not supported.

Comment on lines 56 to 58
<PropertyGroup Condition="'$(TargetArchitecture)' == 'wasm'">
<NeedNativePublishSupportForLLVM>true</NeedNativePublishSupportForLLVM>
</PropertyGroup>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NeedNativePublishSupportForSDK6 will be deleted in dotnet/runtime#82568 at which point this will conflict a lot...

I think we should just unconditionally override ImportRuntimeIlcPackageTarget:

  <!-- NativeAOT-LLVM: override ImportRuntimeIlcPackageTarget to not depend on the SDK-provided ResolvedILCompilerPack item -->
  <PropertyGroup>
    <!-- EmitLegacyAssetsFileItems tells the SDK to generate PackageDefinitions -->
    <EmitLegacyAssetsFileItems>true</EmitLegacyAssetsFileItems>
  </PropertyGroup>

  <Target Name="ImportRuntimeIlcPackageTarget" Condition="'$(BuildingFrameworkLibrary)' != 'true'" DependsOnTargets="$(ImportRuntimeIlcPackageTargetDependsOn)" BeforeTargets="Publish">
    <Error Condition="'@(PackageDefinitions)' == ''" Text="The PackageDefinitions ItemGroup is required for target ImportRuntimeIlcPackageTarget" />

    <PropertyGroup>
      <RuntimePackagePath Condition="'%(PackageDefinitions.Name)' == '$(RuntimeIlcPackageName)'">%(PackageDefinitions.ResolvedPath)</RuntimePackagePath>
      <IlcHostPackagePath Condition="'%(PackageDefinitions.Name)' == '$(IlcHostPackageName)'">%(PackageDefinitions.ResolvedPath)</IlcHostPackagePath>
    </PropertyGroup>

  </Target>

Before importing Microsoft.NETCore.Native.targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, have tried that and looks simpler.

@jkotas jkotas merged commit f9740bf into dotnet:feature/NativeAOT-LLVM Apr 9, 2023
@yowl yowl deleted the reinstate-package-resolve branch April 9, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants