dotnet / source-build

A repository to track efforts to produce a source tarball of the .NET Core SDK and all its components
MIT License
259 stars 126 forks source link

SBRP: `ILAsmToolPath` looks like it's supposed to be overrideable but overriding it breaks the build #4383

Open omajid opened 1 month ago

omajid commented 1 month ago

Describe the Bug

I am trying to override the ilasm and ildasm tools used by SBRP (while building as part of the VMR) to provide a custom implementation (for non-intel, non-arm platforms).

This section of msbuild looks like it supports providing an ILAsmToolPath property that can be used to override the ilasm tool used (it is used to set IlasmDir). However, the code also errors out if it is set.

https://github.com/dotnet/source-build-reference-packages/blob/a4c02499bef24d0e16255657ccdb160d26c82c32/src/targetPacks/Directory.Build.targets#L89-L110

      <IlasmDir Condition="'$(ILAsmToolPath)' != ''">$([MSBuild]::NormalizeDirectory($(ILAsmToolPath)))</IlasmDir>
      <IldasmDir Condition="'$(ILDAsmToolPath)' != ''">$([MSBuild]::NormalizeDirectory($(ILDAsmToolPath)))</IldasmDir>
    </PropertyGroup>

    <ItemGroup Condition="'$(ILAsmToolPath)' == ''">
      <_IlToolPackageReference Include="$(MicrosoftNetCoreIlasmPackageName)" Version="$(MicrosoftNETCoreILAsmVersion)" />
    </ItemGroup>
    <ItemGroup Condition="'$(ILDAsmToolPath)' == ''">
      <_IlToolPackageReference Include="$(MicrosoftNetCoreIldasmPackageName)" Version="$(MicrosoftNETCoreILDAsmVersion)" />
    </ItemGroup>
  </Target>

  <Target Name="ResolveIlToolPaths"
          DependsOnTargets="GetILToolPackageReferences">
    <Error Condition="'@(_IlToolPackageReference)' == ''" Text="Package items '_IlToolPackageReference' were not created" />

    <ItemGroup>
      <_IlToolPackageReference NativePath="$(NuGetPackageRoot)\%(Identity)\%(Version)\runtimes\$(MicrosoftNetCoreIlasmPackageRuntimeId)\native" />
      <_IlasmSourceFiles Include="%(_IlToolPackageReference.NativePath)\**\*" />
    </ItemGroup>

    <Error Condition="!Exists('%(_IlToolPackageReference.NativePath)')" Text="Package %(_IlToolPackageReference.Identity)\%(_IlToolPackageReference.Version) was not restored" />

    <PropertyGroup>
      <IlasmDir Condition="'$(IlasmDir)' == '' and '%(_IlToolPackageReference.Identity)' == '$(MicrosoftNetCoreIlasmPackageName)'">%(_IlToolPackageReference.NativePath)/</IlasmDir>
      <IldasmDir Condition="'$(IldasmDir)' == '' and '%(_IlToolPackageReference.Identity)' == '$(MicrosoftNetCoreIldasmPackageName)'">%(_IlToolPackageReference.NativePath)/</IldasmDir>
    </PropertyGroup>
  </Target>

When building, I get:

dotnet/src/source-build-reference-packages/src/targetPacks/Directory.Build.targets(103,5): error : Package items '_IlToolPackageReference' were not created [dotnet/src/source-build-reference-packages/src/targetPacks/ILsrc/microsoft.aspnetcore.app.ref/8.0.0/Microsoft.AspNetCore.App.Ref.8.0.0.csproj] [dotnet/.packages/ArcadeBootstrapPackage/microsoft.dotnet.arcade.sdk/9.0.0-beta.24162.2/tools/Build.proj]

Is it supposed to be user-overrideable or not?

dotnet-issue-labeler[bot] commented 1 month ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

MichaelSimons commented 1 month ago

Do you have a binlog that you can share?

MichaelSimons commented 1 month ago

It looks like the intent is to support this extension point, but the logic isn't right. It seems like there needs to be a ResolveIlToolPaths path for when the ToolPaths are specified.

omajid commented 1 month ago

source-inner-build.binlog.tar.gz

MichaelSimons commented 1 month ago

[Triage] The logic appears broken. Either a custom path should be supported or the support that exists should be removed.