Multiple copies of xunit.abstractions.dll breaks the build up-to-date check

[sharwell]

When a project references both xunit (2.3.1) and xunit.runner.visualstudio (2.3.1), there are two places where xunit.abstractions.dll is copied to the output directory. These two files do not have the same last write date, so if the older one gets copied to the output directory the fast up-to-date check in Visual Studio will determine that the project is never up-to-date.

For now, we're using an approach like the following in our projects:

<ItemGroup>
  <None Remove="$(NuGetPackageRoot)xunit.runner.visualstudio\$(XUnitRunnerVisualStudioVersion)\build\net20\..\_common\xunit.abstractions.dll" />
</ItemGroup>

A better solution would be to condition the use of CopyToOutputDirectory in xunit.runner.visualstudio to only cases where xunit.abstractions.dll is not referenced by the project itself.

[clairernovotny]

A better solution would be to condition the use of CopyToOutputDirectory in xunit.runner.visualstudio to only cases where xunit.abstractions.dll is not referenced by the project itself.

Would you be able to submit a PR with that fix?

[sharwell]

@onovotny I don't think I'm going to be able to get to it

[ViktorHofer]

I would like us to fix this as it still affects every test project that uses xunit and xunit.runner.visualstudio on .NET Framework.

Here's a repro:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net481</TargetFramework>
    <IsPackable>false</IsPackable>
    <IsTestProject>true</IsTestProject>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.8.0" />
    <PackageReference Include="xunit" Version="2.6.6" />
    <PackageReference Include="xunit.runner.visualstudio" Version="2.5.6" />
  </ItemGroup>

</Project>

dotnet build /bl -> open the binary log and observe the following:

@bradwilson @clairernovotny do you remember the reason why xunit.runner.visualstudio redistributes xunit.abstractions.dll for .NET Framework? xunit already brings xunit.abstractions in transitively.

[bradwilson]

It's linked against this assembly, so it must be present.

[ViktorHofer]

Is xunit.runner.visualstudio ever used without xunit? Is that even possible? I don't see it being copied on .NETCoreApp.

[bradwilson]

You can use it to run v1 tests (and someday, v3 tests), neither of which would have copies of xunit.abstractions.

I believe the reason it does not get copied in the case of .NET Core is because we aren't concerned with supporting v1 tests there (since v1 significantly pre-dates .NET Core). I assume once v3 support is added, the copy would become a necessary dependency step, since v3 tests wouldn't bring a copy of xunit.abstractions (but it would still be binary dependency that is required to support running v2 tests, as xunit.v3.runner.utility is still linked against xunit.abstractions for v2 back-compat).

[bradwilson]

Also, it would be an interesting question to see what would happen if someone added a reference to xunit.runner.visualstudio to a non-xUnit.net project, what would happen when the system tried to run discovery without a binary dependency like xunit.abstractions being present. I assume you'd get the equivalent of a Fusion binding failure, which would probably be surfaced to the user as an error somehow.

[bradwilson]

Would it be possible to replace the ItemGroup item for xunit.abstractions.dll with a copy step that only copied the assembly if it isn't already present? How would that affect the up-to-date checks?

[bradwilson]

I see this in the Copy MSBuild task documentation:

I don't think the warning is applicable here (since we know the two copies are pretty much always going to be the same bits), and dependency analysis for the test adapter itself is really all that matters, which we would leave as an ItemGroup item.

[bradwilson]

I'll test it out and see what happens. Worst case I revert the experiment. 😄

[bradwilson]

Damn it GitHub, be stupider. 😂

[bradwilson]

The Copy task still double writes, I think because the file dates are set by the NuGet packaging process when packed (i.e., they're not preserved). Maybe a Condition instead of SkipUnchangedFiles will do the trick...

[bradwilson]

That does seem to do the trick (no more double writes logged in the binlog):

You should be able to verify with 2.5.7-pre.4 from feedz.io https://xunit.net/docs/using-ci-builds

[ViktorHofer]

The problem that I see with that target is that it doesn't verify if that xunit.abstractions.dll in the output path is the one needed (i.e. the version referenced by xunit.runner.visualstudio).

[bradwilson]

And here's where I landed if you want to double check to make sure I'm solving the problem correctly w/rt up-to-date checks:

https://github.com/xunit/visualstudio.xunit/compare/f970798..3cb1ad2

[bradwilson]

The problem that I see with that target is that it doesn't verify if that xunit.abstractions.dll in the output path is the one needed

There has only ever been one actual binary version. The purpose of that DLL was to stick a flag in the sand when 2.0 released and never change it, so that our runners could be both backward and forward compatible within the same major version (older 2.x runners can run newer 2.x tests in addition to newer 2.x runners running older 2.x tests).

There have been four NuGet packages with the exact same binary in it (built against PCL259), only changing the nuspec file as necessary (usually for new platforms).

[bradwilson]

Updated version in 2.5.7-pre.8 now has the original file dates preserved per the above referenced issue.

Note that you'll still have double-writes for any version that relies on xunit.abstractions other than 2.0.3, but those would all be relatively old releases since 2.0.3 was released in 2018 and used by 2.4.1 and later of xunit and friends.

And as mentioned above, the binaries are otherwise identical aside from code signing. 2.0.0 and 2.0.1 weren't signed while 2.0.2 and 2.0.3 were, so there are three identical versions that are actually physically different.

[ViktorHofer]

This is great. I love that we found the actual fix for this and were able to remove the Copy target workaround.

[bradwilson]

Thanks to your persistence. 😄

[bradwilson]

(I'm scouring the other projects to see if I can find any more places I can fix)

[bradwilson]

The whole thing makes me want to re-evaluate signing, and sign binaries before packing, then pass an empty file list when asking to sign the package so it'll just sign the package.

An example: xunit.runner.utility.*.dll from xunit.runner.utility will have a different signature from xunit.runner.utility.*.dll placed into any of the runner packages (like xunit.runner.console), and if for some reason (ahem 😂) someone copies the runner into their output folder (like we're forced to do with VSTest), they may have the same double write problem, since the copy in the runner nupkg will be signed at a different time than one in xunit.runner.utility.

From a correctness perspective this seems like the right thing to do, but it's definitely very much an edge case, in particular because the number of people writing runners is very low. And it wouldn't break the code, just constantly fail that out-of-date problem, assuming people were using MSBuild to do the copies for the runner.

[ViktorHofer]

From a correctness perspective this seems like the right thing to do, but it's definitely very much an edge case

Agreed that this would be the right thing to do. In general, when redistributing a binary via different means (explicit nuget package and inside another nuget package), it's important to not mutate the binary in a way that makes it different to its sibling.

That said, I agree that this is an edge case which currently probably only affects a handful of people. But I'm not saying you shouldn't do it ;) We would probably benefit from that.

[bradwilson]

Now is the time when I really dislike the fact that signing can only occur on Windows. I don't want to build on Windows, so now the split becomes much worse.

Currently, we build packages on Linux and push them in Actions artifacts. Then we download them on Windows, sign them, upload them to feedz.io, then push the signed versions over top of the unsigned versions in Actions artifacts.

The alternative is a lot more cumbersome to preserve the Linux builds. Unfortunately the Windows Actions runners are just godawful slow, presumably because you're both "paying" for the overhead of running Windows instead of Linux, and most definitely because NTFS is about half the performance of ext4.

[bradwilson]

sigh

[bradwilson]

The move to Windows for CI adds 33% to the build time, despite the fact that we previously had to clone and restore the project twice previously (once in Linux and once in Windows).

Now to see if the resulting packages actually work. 😁

[bradwilson]

I declare some kind of victory (for v3). Reluctant, maybe. 😭😂

Time to do v2, and then update the VSTest adapter, so that everything becomes consistent. I think I'll stop posting (aka whining) here now. 😆