Clean up dead code in ILCompiler targets #85618
Conversation
sbomer
commented
May 1, 2023
sbomer
commented
- The logic for referencing ILCompiler as an MSBuild SDK should no longer be needed because we use PackageReference.
- IlcCalledViaPackage should always be true
ghost
assigned 
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue Details
|
| @@ -33,22 +31,22 @@ | |||
| <IlcDynamicBuildPropertyDependencies>SetupProperties</IlcDynamicBuildPropertyDependencies> | |||
| </PropertyGroup> | |||
|
|
|||
| <!-- If called via package instead of the SDK, update the runtime package version to match the build package --> | |||
| <ItemGroup Condition="'$(AotRuntimePackageLoadedViaSDK)' != 'true'"> | |||
| <!-- Update the KnownILCompilerPack version to match the PackageReference, in case there's an explicit reference to a specific version. --> | |||
There was a problem hiding this comment.
If we drop this, https://github.com/dotnet/runtime/blob/main/docs/workflow/building/coreclr/nativeaot.md#building-packages and https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/docs/compiling.md#using-daily-builds should be updated with alternative instructions.
There was a problem hiding this comment.
I don't think we should drop it (at least not yet) - aspnet is relying on this for testing. cc @eerhardt
There was a problem hiding this comment.
I think we could support this by using RuntimeFrameworkVersion or setting the framework pack version. Agreed that we should update the docs
There was a problem hiding this comment.
One thing I'm worried about with this configuration is that we're effectively splitting the framework with this config, namely the ref assemblies are from one version and the runtime assemblies are from another.
There was a problem hiding this comment.
Unfortunately that has a problem which @jkotas pointed out (#84372 (comment)) - it also sets the aspnet framework version. I'm not sure whether that's by design. I think the real fix for this would be to have a property like RuntimeFrameworkVersion that only applies to the runtime version.
There was a problem hiding this comment.
One thing I'm worried about with this configuration is that we're effectively splitting the framework with this config,
Yes, it has potential versioning issues. These versioning issues are rarely problem in practice. The only 100% reliable way to avoid these potential versioning issues is by waiting for fully coherent SDK build. that happens like once a month.
There was a problem hiding this comment.
only 100% reliable way to avoid these potential versioning issues is by waiting for fully coherent SDK build. that happens like once a month.
I thought this was all done by targeting packs. Can we not engineer the SDK to pull the appropriate targeting pack based on a single property value? That is, you specify one version and it pulls a ref pack (if not locally available) from NuGet with that version, and then an ILCompiler pack the same way?
I suppose the alternative is to rely more on the VMR to accelerate the production of coherent SDK builds, which honestly doesn't sound too bad either.
There was a problem hiding this comment.
Providing one property to consistently control the version of runtime bits (and any tool/targeting packs that should version with runtime) would make sense to me, though I don't know much about the targeting packs.
|
Failures are known issues. |
