dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.22k stars 1.35k forks source link

Packing iOS binding projects with NativeReference fails #4584

Closed mattleibow closed 2 years ago

mattleibow commented 5 years ago

Steps to reproduce

I have created a repository with this issue: https://github.com/mattleibow/XamarinNativeReferencesBug

I have opened an issue on the MSBuild.Sdk.Extras repository as they can add a workaround there as well: https://github.com/onovotny/MSBuildSdkExtras/issues/176

Project file

<Project>
<Project Sdk="MSBuild.Sdk.Extras/2.0.31">
  <PropertyGroup>
    <TargetFrameworks>xamarinios1.0</TargetFrameworks>
    <IsBindingProject>true</IsBindingProject>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
  </PropertyGroup>
  <ItemGroup>
    <Compile Remove="ApiDefinition.cs" />
    <Compile Remove="StructsAndEnums.cs" />
    <ObjcBindingApiDefinition Include="ApiDefinition.cs" />
    <ObjcBindingCoreSource Include="StructsAndEnums.cs" />
    <NativeReference Include="..\native\Aardvark.framework" Kind="Framework" ForceLoad="true" SmarkLink="true" Frameworks="MessageUI" LinkerFlags="-ObjC" />
  </ItemGroup>
</Project>

Command line

msbuild /restore /t:pack /bl

Expected behavior

The project builds with two packages as output.

Actual behavior

The project fails with an error:

error : The file '/Users/matthew/Projects/Testing/XamarinNativeReferences/source/Square.Aardvark/bin/Debug/xamarinios1.0/Native.Square.Aardvark.manifest' to be packed was not found on disk.

Environment data

msbuild /version output:

Microsoft (R) Build Engine version 16.2.0-ci for Mono
Copyright (C) Microsoft Corporation. All rights reserved.

16.200.19.36501

OS info: macOS Mojave 10.14.5 (18F132)

Redth commented 5 years ago

@chamons your team should probably be aware of / involved in this discussion as well :)

livarcocc commented 5 years ago

@mattleibow why do you believe this is an issue with MSBuild?

mattleibow commented 5 years ago

Because MSBuild (Microsoft.Common.CurrentVersion.targets) assumes that all instances of NativeReference is a sort of thing used to generate a manifest or something:

https://github.com/microsoft/msbuild/blob/v16.2.32702/src/Tasks/Microsoft.Common.CurrentVersion.targets#L5548-L5554

    <ItemGroup>
      <_BuiltProjectOutputGroupOutputIntermediate Include="$(OutDir)$(_DeploymentTargetApplicationManifestFileName)" Condition="'@(NativeReference)'!='' or '@(_IsolatedComReference)'!=''">
        <TargetPath>$(_DeploymentTargetApplicationManifestFileName)</TargetPath>
        <!-- For compatibility with 2.0 -->
        <OriginalItemSpec>$(OutDir)$(_DeploymentTargetApplicationManifestFileName)</OriginalItemSpec>
      </_BuiltProjectOutputGroupOutputIntermediate>
    </ItemGroup>

The assumption of the existence of a NativeReference instance does not mean that we need to have a file named Native.$(AssemblyName).manifest added to the output.

Maybe it should, then in that case, it appears that MSBuild does not output that file for iOS projects. If that file is created, then it causes issues with other parts of the build.

That could be a Xamarin thing - not generating the file - so we could move it to the https://github.com/xamarin/xamarin-macios repo.

rolfbjarne commented 3 years ago

That could be a Xamarin thing - not generating the file - so we could move it to the https://github.com/xamarin/xamarin-macios repo.

No, we (Xamarin) is not generating that file because it serves us no purpose.

The assumption of the existence of a NativeReference instance does not mean that we need to have a file named Native.$(AssemblyName).manifest added to the output.

This is the problem: the existence of a NativeReference item doesn't mean that such a manifest file will be created.

rolfbjarne commented 2 years ago

So we found a workaround for this:

https://github.com/xamarin/xamarin-macios/blob/df395f2cff9b121fb3b7f1dc3a37f2948dbebc04/msbuild/Xamarin.Shared/Xamarin.Shared.targets#L208-L225

unfortunately the workaround does not work when a project uses TargetFrameworks (plural) instead of TargetFramework (singular), because in that case none of our targets files are imported.

Example binlog: dotnet.binlog.zip

For comparison here's a buildlog using TargetFramework instead: dotnet-targetframework.binlog.zip

This basically means that we (Xamarin) can't work around it in this scenario, because none of our code is involved in the build (pack) process.

rainersigwald commented 2 years ago

unfortunately the workaround does not work when a project uses TargetFrameworks (plural) instead of TargetFramework (singular), because in that case none of our targets files are imported.

You should be able to hook into the buildCrossTargeting imports to get your code into that scenario, but #7564 looks fine to me.

https://github.com/dotnet/msbuild/blob/12bf0a8ae0c4362b0b3fdcfb3eba75132db9bc69/src/Tasks/Microsoft.Common.CrossTargeting.targets#L20-L23

rolfbjarne commented 2 years ago

You should be able to hook into the buildCrossTargeting imports to get your code into that scenario

You mean we could add a targets file to the CustomBeforeMicrosoftCommonCrossTargetingTargets property? Not sure where we could do that, because nothing from our iOS workload is loaded.

rainersigwald commented 2 years ago

That's what I expected but I might be missing something in the workload implementation.

Do you have a timeframe for when you need the fix?

rolfbjarne commented 2 years ago

Do you have a timeframe for when you need the fix?

The sooner the better of course, but for now it's only been reported internally, so I'm unsure how many customers would be affected. Unless somebody else runs into it, I wouldn't backport this anywhere, and just let it flow to a stable release. I'm guessing that would be with .NET 7 in the fall?

Also we have a workaround available, so nobody should be blocked by it.

rainersigwald commented 2 years ago

We can easily get it into 6.0.400, but it would be very difficult to get it into 6.0.300 at this point.

rolfbjarne commented 2 years ago

6.0.400 should work just fine!

mattjohnsonpint commented 2 years ago

A workaround if you don't want to use 6.0.400 while it's in preview:

<Target Name="RemoveInvalidManifest" AfterTargets="BuiltProjectOutputGroup">
  <ItemGroup Condition="!Exists('$(OutDir)$(_DeploymentTargetApplicationManifestFileName)')">
    <BuiltProjectOutputGroupOutput Remove="$([System.IO.Path]::GetFullPath('$(OutDir)$(_DeploymentTargetApplicationManifestFileName)'))" />
  </ItemGroup>
</Target>

This applies the same effect as was done in #7564, and works for me.