CsWinRT icon indicating copy to clipboard operation
CsWinRT copied to clipboard

`CsWinRTAotExportsEnabled` incorrectly assumes `PublishAot == true` implies a publish is actually occurring

Open Jeremy-Price opened this issue 1 year ago • 5 comments

Microsoft.Windows.CsWinRT.targets defines a property CsWinRTAotExportsEnabled:

   <!--
      If the AOT optimizer is enabled, and we're publishing with NativeAOT, automatically set CsWinRTAotExportsEnabled as well.
      Only do this if the property is not already set by the user, so we respect any existing preference.
    -->
    <CsWinRTAotExportsEnabled Condition="'$(CsWinRTAotExportsEnabled)' == '' and '$(CsWinRTAotOptimizerEnabled)' == 'true' and '$(PublishAot)' == 'true'">true</CsWinRTAotExportsEnabled>
    <CsWinRTAotExportsEnabled Condition="'$(CsWinRTAotExportsEnabled)' != 'true'">false</CsWinRTAotExportsEnabled>

However, PublishAot being true does not indicate that a build was actually an NAOT build, instead it indicates that a publish build will produce NAOT’d assemblies. This results in build failures when expected artefacts don’t exist. For example, WinRT.Host.dll is outputted conditional on CsWinRTAotExportsEnabled not being true.

I'm not sure how often this idiom is used across the targets files, so I can't recommend a specific fix, but it's probably something like setting CsWinRTAotExportsEnabled to true in a target that runs only before the publish target and ensuring that the only place PublishAot is checked is when that's set.

Jeremy-Price avatar Mar 20 '24 21:03 Jeremy-Price

@manodasanW @Sergio0694

Jeremy-Price avatar Mar 20 '24 21:03 Jeremy-Price

@manodasanW @Sergio0694

I propose the result of 'Build' is always both JIT and NAOT compatible, no matter if PublishAot is true. In other words, the result of Build, including what code CsWinRT generates, must not depend on PublishAot, _IsPublishing, etc. Any NAOT specific tweaks, such as omitting WinRT.Host.dll, should be achieved by modifying the behavior of the Publish target instead, to omit that DLL from the publish output folder only.

Rationale is that in the .NET build/publish model, Publish is a step in addition to Build, producing outputs in a separate location, and does not affect the output of Build.

nagya avatar Jun 28 '24 18:06 nagya

We should probably update that condition to also be conditional on _IsPublishing as well, yeah. @MichalStrehovsky remind me, is that property something we can take a dependency on? The fact it has the "_" prefix makes me a bit uncomfortable 😅

Sergio0694 avatar Jun 28 '24 21:06 Sergio0694

The issue if I recall with that property is it relies on dotnet publish and doesn't work with msbuild publish.

manodasanW avatar Jun 28 '24 23:06 manodasanW

We should probably update that condition to also be conditional on _IsPublishing as well, yeah. @MichalStrehovsky remind me, is that property something we can take a dependency on? The fact it has the "_" prefix makes me a bit uncomfortable 😅

There's a whole lot of discussion about this at https://github.com/dotnet/sdk/issues/26324.

The problem is that if you just force running the Publish target, you're not actually getting publish behaviors. The _IsPublishing property needs to be set as well. I was trying to get us to a spot where there is at least a public property one can set but it's really difficult to get any sort of traction on this and I don't own the SDK or the SDK behaviors. (This affects all publishes, not just PublishAot.) We'll at least get VS to set this property when doing publish soon.

This leads to stuff like this https://github.com/MichalStrehovsky/rt-sz/blob/4c11e7fb4c40d294086ca541546278605eda01c7/src/winrt-component-full/winrt-component-full.csproj#L20-L21 (this is not a .NET 9 issue, the issue is that _IsPublishing is not set and Publish target is getting activated).

Here you have an SDK owner saying it's okay to rely on _IsPublishing: https://github.com/microsoft/CsWinRT/pull/1547#issuecomment-2083825595

MichalStrehovsky avatar Jul 01 '24 06:07 MichalStrehovsky