SimonCropp / Polyfill

Source only package that exposes newer .net and C# features to older runtimes.
MIT License
277 stars 20 forks source link

PackageReference tests don't consider transitive dependencies #14

Closed mattjohnsonpint closed 1 year ago

mattjohnsonpint commented 1 year ago

The tests for various package references only consider directly referenced dependencies, not transitive ones.

For example, this checks if System.Threading.Tasks.Extensions is referenced:

https://github.com/SimonCropp/Polyfill/blob/8f862a423b2c65f8683f55f8f40bb7a5ffad6066/src/Polyfill/Polyfill.targets#L13-L14

If you've referenced it directly, then it works. But if you get it via dependency of System.Text.Json, then it is not detected.

mattjohnsonpint commented 1 year ago

Same is true for System.ValueTuple, System.Memory, etc.

mattjohnsonpint commented 1 year ago

I can workaround for now by adding the references conditionally (as per your readme). I can also use PrivateAssets="all" on those, so they don't become direct dependencies on our nuget package. (Users will get them transitively through System.Text.Json.)

baronfel commented 1 year ago

Try this to get the full list of direct and transitive PackageReferences used by the current project:

  <!-- Get the final resolved set of all packages -->
  <PropertyGroup>
    <!-- Need this to add your target to the overall build pipeline - 
        PrepareForBuild is a good general-purpose target for non-timing-critical tasks -->
    <PrepareForBuildDependsOn>$(PrepareForBuildDependsOn);PrintAllDeps</PrepareForBuildDependsOn>
  </PropertyGroup>
  <Target Name="PrintAllDeps" DependsOnTargets="ResolvePackageAssets">
    <!-- ResolvePackageAssets provides the resolved set of all packages in the `PackageDependencies` item list -->
    <Message Importance="High" Text="This project depends on the following exhaustive set of dependencies:" />
    <!-- This item is only the string name, no additional metadata, so for versions you might need to do more work -->
    <Message Importance="High" Text="@(PackageDependencies)" />
  </Target>

This prints the following for the .NET SDK Container builds unit tests:

09:33:06 ❯ dotnet build -bl
MSBuild version 17.5.0+6f08c67f3 for .NET
  Determining projects to restore...
  All projects are up-to-date for restore.
  This project depends on the following exhaustive set of dependencies:
  GitHubActionsTestLogger;Microsoft.CodeCoverage;Microsoft.NET.Test.Sdk;Microsoft.NETCore.Platforms;Microsoft.NETCore.Targets;Microsoft.TestPlatform.ObjectModel;Microsoft.TestPlatform.Te
  stHost;Microsoft.VisualStudioEng.MicroBuild.Core;Microsoft.Win32.Primitives;Nerdbank.GitVersioning;NETStandard.Library;Newtonsoft.Json;NuGet.Common;NuGet.Configuration;NuGet.Frameworks
  ;NuGet.Packaging;NuGet.Versioning;runtime.debian.8-x64.runtime.native.System.Security.Cryptography.OpenSsl;runtime.fedora.23-x64.runtime.native.System.Security.Cryptography.OpenSsl;run
  time.fedora.24-x64.runtime.native.System.Security.Cryptography.OpenSsl;runtime.native.System;runtime.native.System.IO.Compression;runtime.native.System.Net.Http;runtime.native.System.S
  ecurity.Cryptography.Apple;runtime.native.System.Security.Cryptography.OpenSsl;runtime.opensuse.13.2-x64.runtime.native.System.Security.Cryptography.OpenSsl;runtime.opensuse.42.1-x64.r
  untime.native.System.Security.Cryptography.OpenSsl;runtime.osx.10.10-x64.runtime.native.System.Security.Cryptography.Apple;runtime.osx.10.10-x64.runtime.native.System.Security.Cryptogr
  aphy.OpenSsl;runtime.rhel.7-x64.runtime.native.System.Security.Cryptography.OpenSsl;runtime.ubuntu.14.04-x64.runtime.native.System.Security.Cryptography.OpenSsl;runtime.ubuntu.16.04-x6
  4.runtime.native.System.Security.Cryptography.OpenSsl;runtime.ubuntu.16.10-x64.runtime.native.System.Security.Cryptography.OpenSsl;System.AppContext;System.Buffers;System.Collections;S
  ystem.Collections.Concurrent;System.Console;System.Diagnostics.Debug;System.Diagnostics.DiagnosticSource;System.Diagnostics.Tools;System.Diagnostics.Tracing;System.Formats.Asn1;System.
  Globalization;System.Globalization.Calendars;System.Globalization.Extensions;System.IO;System.IO.Compression;System.IO.Compression.ZipFile;System.IO.FileSystem;System.IO.FileSystem.Pri
  mitives;System.Linq;System.Linq.Expressions;System.Net.Http;System.Net.Primitives;System.Net.Sockets;System.ObjectModel;System.Reflection;System.Reflection.Emit;System.Reflection.Emit.
  ILGeneration;System.Reflection.Emit.Lightweight;System.Reflection.Extensions;System.Reflection.Metadata;System.Reflection.Primitives;System.Reflection.TypeExtensions;System.Resources.R
  esourceManager;System.Runtime;System.Runtime.CompilerServices.Unsafe;System.Runtime.Extensions;System.Runtime.Handles;System.Runtime.InteropServices;System.Runtime.InteropServices.Runt
  imeInformation;System.Runtime.Numerics;System.Security.Cryptography.Algorithms;System.Security.Cryptography.Cng;System.Security.Cryptography.Csp;System.Security.Cryptography.Encoding;S
  ystem.Security.Cryptography.OpenSsl;System.Security.Cryptography.Pkcs;System.Security.Cryptography.Primitives;System.Security.Cryptography.ProtectedData;System.Security.Cryptography.X5
  09Certificates;System.Text.Encoding;System.Text.Encoding.Extensions;System.Text.Encodings.Web;System.Text.Json;System.Text.RegularExpressions;System.Threading;System.Threading.Tasks;Sy
  stem.Threading.Tasks.Extensions;System.Threading.Timer;System.Xml.ReaderWriter;System.Xml.XDocument;Valleysoft.DockerCredsProvider;xunit;xunit.abstractions;xunit.analyzers;xunit.assert
  ;xunit.core;xunit.extensibility.core;xunit.extensibility.execution;xunit.runner.visualstudio
mattjohnsonpint commented 1 year ago

Also, my idea of adding them with PrivateAssets="all" doesn't work. That stops them flowing transitively at all, even from the System.Text.Json reference.

So having this would be super helpful. Thanks.

Tyrrrz commented 1 year ago

@baronfel is there a chance you could explain to an MSBuild idiot (i.e., myself), why is PrepareForBuildDependsOn required? Isn't DependsOnTargets="ResolvePackageAssets" already enough to add the task to the build pipeline?

baronfel commented 1 year ago

DependsOnTargets only comes into play when the target is actually going to be executed, so either you need to invoke the target directly (dotnet build /t:MYTARGET), or you need to cause your target to be a dependency of a target that is going to be run (for example Build, which is implicitly called by dotnet build and several other pathways).

I chose the latter path, and based on what I knew of the problem I figured that your timing just needed to be

Based on that, I went with the good old standby, the PrepareForBuild target. This target is often used as a grab-bag of 'I'm not super timing sensitive' tasks. It's also very handy because its DependsOnTargets uses a Property to allow for extensibility. Hence the two-part pattern of

There are some detailed docs about the build order in general here (including a specific ordering example), but we don't have a good set of examples for "you want to do action X, target Y is a good place for that".

Tyrrrz commented 1 year ago

DependsOnTargets only comes into play when the target is actually going to be executed, so either you need to invoke the target directly (dotnet build /t:MYTARGET), or you need to cause your target to be a dependency of a target that is going to be run (for example Build, which is implicitly called by dotnet build and several other pathways).

Ah, right. I confused it with AfterTargets and BeforeTargets.

Would setting BeforeTargets="PrepareForBuild" work here?

baronfel commented 1 year ago

Not on its own - even if you specify BeforeTargets="PrepareForBuild" that only specifies a 'max bound'. So you'd still need DependsOnTargets="ResolvePackageAssets" as well to establish the lower bound. That means you could drop the PrepareForBuildDependsOn at least though!

SimonCropp commented 1 year ago

i probs wont get to this till tomorrow. if anyone wants to attempt a PR before then, that would be appreciated

Tyrrrz commented 1 year ago

@baronfel I just tested a bit and it seems that I could also use @Reference, which appears to return all assemblies referenced by the project (including package dependencies, netfx GAC references, and project references). What's the difference between @Reference and @PackageDependencies?

Edit: actually, it seems that @Reference does not actually include package references after all, my bad.

SimonCropp commented 1 year ago

thanks for all the help peoples.

Can you try v1.9.0

baronfel commented 1 year ago

@Tyrrrz looks like you found it out :) I'd also say that Reference items are too general - they can come from too many different sources. I'd generally prefer using a less-powerful, more specific Item.

mattjohnsonpint commented 1 year ago

It worked! Thanks! I had an unrelated issue in updating to 1.9.0 though. See #19. Thanks!