dotnet / Nerdbank.Streams

Specialized .NET Streams and pipes for full duplex in-proc communication, web sockets, and multiplexing
MIT License
623 stars 59 forks source link

Package Dependencies #145

Closed mapfel closed 4 years ago

mapfel commented 4 years ago

The NuGet package Nerdbank.Streams doesn't have a (direct) dependency to System.Memory.

see Screenshot: NETStandard 2.0 Dependencies of Nerdbank.Streams

Interestingly the assembly has one.

see Screenshot: Dependency to System.Memory

I'm surprised to see this. Also, your codebase doesn't contain a reference to the System.Memory assembly (at a fist glance). Any ideas how that happened? Compiler magic by inlining of parts from System.Memory??

Because there is no direct dependency our package management has some issues to resolve the right version. It might help in these circumstances, if the System.Memory package reference is listed inside the nuspec file.

AArnott commented 4 years ago

Yes, System.Memory is brought in transitively. As such, the compiler is fed a reference to it and can certainly produce an assembly that references it. Because this is depended on transitively through nuget dependencies as well (via System.IO.Pipelines) we needn't express it directly in our csproj file nor in the nuspec file. NuGet brings it all down and adds references to this to consuming projects automatically.

Because there is no direct dependency our package management has some issues to resolve the right version.

What package management system are you using? I've only ever intended to support NuGet.

mapfel commented 4 years ago

So far, so clear. But none of your source code parts is using System.Memory (as fare I could see). So the question is, why the compiled assembly contains a reference? I have never observed such an effect before. What magic is it here?

From the higher perspective of dependency management, we have a reference to System.Memory in the assemblies metadata and therefore at this level the dependency is not transitive anymore, it is then a direct dependency to another assembly.

I treat the NuGet package as a wrapper around assemblies which allows better management regarding dependencies, versioning, ... So this wrapper must follow the same dependency rules as its content.

We use Paket for the management. And during the dependency graph calculation (paket install), Paket runs into a version mismatch issue because it cannot see the reference inside the assembly metadata and brings then the wrong version into the workspace which blocks the compiler later.

Currently, we work-around that with binding redirects. But that is an ugly duct-tape.

AArnott commented 4 years ago

But none of your source code parts is using System.Memory

I'm not sure how you came to this determination. We use it all over the place. For example System.Memory defines the SequencePosition type which we use in many places in the source code of Nerdbank.Streams. Were you relying on searching for using System.Memory; in our code? System.Memory.dll doesn't define that namespace, so you wouldn't see it anywhere.

So the question is, why the compiled assembly contains a reference? I have never observed such an effect before. What magic is it here?

The compiler doesn't create an assembly reference where there's no need for it.

We use Paket for the management.

I've heard of it. But I've never used it. And hearing of the troubles it's causing you, I now have one more reason to avoid it.

it cannot see the reference inside the assembly metadata and brings then the wrong version into the workspace which blocks the compiler later.

Paket is looking at assembly metadata? That sounds more invasive than just package management. I don't know why it would even need that information as assembly versions have nothing to do with ascertaining what package versions you'd need to download. And further I'm confused by you saying "it cannot see the reference". But you and I both see the reference in the assembly metadata. Your whole question centers around how and why that reference got there.

Currently, we work-around that with binding redirects. But that is an ugly duct-tape.

Binding redirects may often be necessary, independent of any package manager you may be using. The version of Nerdbank.Streams that I'm looking at references System.Memory 4.0.1.0. But there are other versions out there, and if the transitive closure of all your dependencies includes any other version than 4.0.1.0 of System.Memory, you'll have to use binding redirects in your .NET Framework application in order for assembly loads to work. That's just a .NET Framework runtime requirement and no failing of your package manager. But what you may not be aware of is that MSBuild has a couple properties you can set to automatically generate these binding redirects during the build when MSBuild can see the transitive closure of all assembly references. I use the automatically generated binding redirects almost everywhere and they work great.

mapfel commented 4 years ago

Were you relying on searching for using System.Memory; in our code? System.Memory.dll doesn't define that namespace, so you wouldn't see it anywhere.

Yes, right: I searched that way. And now I see SequencePosition is indeed from System.Memory and in namespace System "only".

For example System.Memory defines the SequencePosition type which we use in many places in the source code of Nerdbank.Streams.

mmh, now with that knowledge IMHO the missing reference is a failure. Hence the source code references SequencePosition it needs a reference to System.Memory because we have that reference just simply. That the reference is brought in transitive doesn't mean, that we can skip it.

And hearing of the troubles it's causing you, I now have one more reason to avoid it.

I have created the wrong impression and conclusion yet. Paket is very useful and brings issues on the desk which are otherwise hidden - like here: a dependency which was forgotten to define.

Paket is looking at assembly metadata?

No no .. that was not meant. Because it cannot do that, it calculates a dep-graph, which later will result in other issues (not expecting that version 4.0.1.0 is necessary, it will point to another version)

Regarding the binding redirects: I'm aware of the topic but not absolutely deep inside. But as long as I can avoid adjustments there, I'm happy. It is the last option and often enough it was an indicator, that something other was not optimal arranged.

mapfel commented 4 years ago

To underline why I think the missing reference is a flaw: If the dependency, which brings in System.Memory, will be removed, the build will fail and needs defining this dependency explicitely. So we have the fact, that today it works by "some coincidence".

AArnott commented 4 years ago

I'm not quite clear whether you feel the missing reference is on your side or Nerdbank.Streams.

If on Nerdbank.Streams' side, I disagree. NuGet brings in compile time references transitively by design. It is not supposed to be required that every transitive package reference that you want to compile against has to be explicitly specified. That would put an extra burden on all package authors and cause version mismatch issues in some cases. While it's true that if my direct dependencies somehow found a way to remove their dependency on System.Memory that I would lose mine when I upgraded, that will never happen as these are corefx packages and their public API is always backward compatible so types referenced will not disappear, and thus a package reference could never be removed.

Any system that requires that every single assembly reference be represented by a direct nuget package reference as well is adding requirements beyond what nuget requires, and I'm not interested in the extra work to support these other systems. If Paket doesn't support this feature of NuGet, I don't consider that a bonus that "finds issues" because the only reason this is an issue for you is because of Paket's limitation in the first place.

mapfel commented 4 years ago

NuGet brings in compile time references transitively by design.

Sure. But this is not a transitive dependency, this is a direct dependency because you use SequencePosition from System.Memory assembly.

I'm absolute with you, when A references B and B references C and A doesn't reference C we have a transitive dependency from A to C where it is not appropriate to have a dependency to C.
But here we have the case that A references C via B AND also C directly. So it is not a transitive reference anymore.

AArnott commented 4 years ago

What your arguing is that NuGet's default behavior of IncludeAssets="compile" is not by design. That's nonsense. Package A can choose to hide the 'compile' asset from its dependency on package B so that when a project depends on A, it will not also have an opportunity to reference B.dll (thus preventing it from producing a "direct" dependency on the assembly). As a package author, you can therefore choose whether your dependencies should be allowed to implicitly become "direct" dependencies of your own consumer.

Have you ever heard of a meta-package? It's an empty package with only dependencies expressed. Large SDKs often use meta packages in order to allow a project to consume the entire SDK with just one package reference. Obviously this project expects and wants to reference the assemblies in all the transitive dependencies brought in from that meta-package. That's the whole point of the meta package. But your argument is that anyone who consumes it that way is flawed.

It is not flawed. It is NuGet design that a package author can choose to expose their own dependencies for direct consumption by their own consumer. And I utilize that design just like many other packages.