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

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

Open
omajid opened this issue May 7, 2024 · 5 comments
Labels
area-sbrp Source build reference packages

Comments

@omajid
Copy link
Member

omajid commented May 7, 2024

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?

Copy link

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.

@omajid omajid changed the title ILAsmToolPath looks like it's supposed to be overrideable but overriding it breaks the build SBRP: ILAsmToolPath looks like it's supposed to be overrideable but overriding it breaks the build May 7, 2024
@MichaelSimons
Copy link
Member

Do you have a binlog that you can share?

@MichaelSimons
Copy link
Member

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
Copy link
Member Author

omajid commented May 7, 2024

@MichaelSimons
Copy link
Member

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

@MichaelSimons MichaelSimons added area-sbrp Source build reference packages and removed untriaged labels May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-sbrp Source build reference packages
Projects
Status: 9.0
Development

No branches or pull requests

2 participants