dotnet / arcade

Tools that provide common build infrastructure for multiple .NET Foundation projects.
MIT License
660 stars 335 forks source link

Add target to the Arcade SDK that substitutes template replacement markers to avoid hardcoded TFM values #14138

Open ViktorHofer opened 9 months ago

ViktorHofer commented 9 months ago

With using floating TFM properties (i.e. NetCurrent), hardcoded TFMs in props/targets files will cause build failures whenever these TFM properties get updated. This would show up during consumption or during the repository's testing. Example of such a hardcoded TFM value: https://github.com/dotnet/xliff-tasks/blob/d66a09c8b11e1235273c8e6cd946e5c8e247f189/src/XliffTasks/build/Microsoft.DotNet.XliffTasks.targets#L5-L6

I imagine that we could solve this by adding a substitution marker into the .targets file which then gets replaced with the actual value at build time. i.e.

Microsoft.DotNet.XliffTasks.targets:

    <XliffTasksDirectory Condition="'$(MSBuildRuntimeType)' == 'Core'">$(MSBuildThisFileDirectory)..\tools\$$NetCurrent$$\</XliffTasksDirectory>
    <XliffTasksDirectory Condition="'$(MSBuildRuntimeType)' != 'Core'">$(MSBuildThisFileDirectory)..\tools\$$NetFrameworkMinimum$$\</XliffTasksDirectory>

XliffTasks.csproj:

  <ItemGroup>
    <None Include="build\Microsoft.DotNet.XliffTasks.props" Pack="true" PackagePath="build" />
    <None Include="build\Microsoft.DotNet.XliffTasks.targets" Pack="true" PackagePath="build" UpdateFileWithTemplateSubstitution="true" />
    <None Include="buildCrossTargeting\*" Pack="true" PackagePath="buildCrossTargeting" />
  </ItemGroup>

We should use the GenerateFileFromTempalte msbuild task that's already part of Arcade.

The target could pass in defaults, i.e. the default TFM properties and values that are defined by the Arcade SDK with the option of adding custom values.

cc @mmitche @mthalman @MichaelSimons @ericstj

ericstj commented 9 months ago

Seems like a reasonable feature - maybe we have some items that are honored by default by the GenerateFileFromTemplate task and we also have some properties with values are specified as well.

One thing I could imagine is that we have something like:

Item Name: TemplateFile

Metadata: FinalItemName -> The name of the item to add to after applying template substitution. EG: Content TemplateProperties -> A semi-colon separated list of name value pairs to set for template properties. EG: MyTemplateProp1=value1;MyTemplateProp2=value2

All other metadata -> copied to FinalItemName

By default we could have some properties specified and available for use in all templates: ProjectTargetFramework.

One thing I could see being a little challenging is inner vs outer build for execution of template substitution. IIRC NuGet does Content inclusion from the outer build - so we'd need to run this in the outer build. I can still see some scenarios where folks might want to do an inner build template substitution - so maybe there is a different item name for that?

tmat commented 8 months ago

Alternatively, we can avoid using specific TFMs for the directory names. We can use e.g. Full and Core (the values of MSBuildRuntimeType variable).

missymessa commented 7 months ago

@ViktorHofer do you need anything from dnceng for this?

ViktorHofer commented 7 months ago

Alternatively, we can avoid using specific TFMs for the directory names. We can use e.g. Full and Core (the values of MSBuildRuntimeType variable).

That works when we only have one .NETCoreApp and .NETFramework version. We have cases where our tools include multiple versions (i.e. for source build compatibility or to load the right set of msbuild assemblies). Slngen is one example:

We also have cases where we need to hardcode the TFM values for assets under the lib/ folder and in those cases we usually target multiple .NETCoreApp versions.

So I still see the value in having this. This will allow our tools to target the minimum supported versions and the current version (for max perf, source build compatibility and flexibility).

@ViktorHofer do you need anything from dnceng for this?

No, this just tracks a feature that will be added to the Arcade.Sdk, hence this repository.