Only include System.Threading.Channels ref for needed targets #81
Conversation
| @@ -22,9 +22,12 @@ | |||
| <DefineConstants>$(DefineConstants);FEATURE_ASYNCDISPOSABLE</DefineConstants> | |||
| </PropertyGroup> | |||
|
|
|||
| <ItemGroup Condition=" '$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'net462'"> | |||
| <PackageReference Include="System.Threading.Channels" Version="6.0.0" /> | |||
There was a problem hiding this comment.
Doesn't need to be 6.0 for these TFMs, 8.0 supports them
There was a problem hiding this comment.
My thought on having it as 6 was it's one of the lowest versions that still "in support" for .NET. So that way further chained dependencies wouldn't get essentially locked to using .NET 8. I'm not sure if there's a better way to do that or not, I'm not super familiar with how all the dependencies shake out, I just know when it's broken ;)
I can change it to 8 if you prefer though, I don't think it will impact the issue I'm running into at least.
There was a problem hiding this comment.
Re "locked to using .NET 8.", Nick is saying that this is not implied
However in general I do agree personally with keeping the lower bound as low as possible for inter-lib dependencies
So I can't think of any reason to up it to 8.0 either; Nick?
There was a problem hiding this comment.
👍
Just the usual problems with shipping older code; because these are implementation packages on those platforms, the consumer will actually get (potentially) older, slower, buggier channel implementations unless they add a direct reference to the newer package versions themselves.
6.0 is also out of support in November, and after that I wouldn't be surprised to start seeing "hey, my dependency scanner says Serilog.Xyz is insecure because of..." types of issues as v6 packages fall out of the patch cycle, whether anyone actually runs the v6 versions or not.
We'd also be setting ourselves up for a larger future leap forward if we decide we want functionality that's only in v8, 9, or 10.
Finally, I think it'd also be a regression, since the released version has a v8 dependency.
I definitely could be wrong on any of these points though, just putting forward my thinking when I targeted v8 here originally.
There was a problem hiding this comment.
On reflection, the fact the original dep was 8.0 is plenty reason not to lower it, and you're not wrong on the scanner point unfortunately at the present time.
Of course, channels is a very specific case, and it's not like Serilog brings a raft of dependencies; so I agree with 8.0 here and your general stance; just thinking aloud really...
I'd however still be be reluctant about setting an expectation that Serilog (or any other lib) owns the busywork of making sure that all its dependencies must be in date at all times. That's really not much better than some tiny package that would be fine with netstandard2.0 and net6.0 TFM-specific builds until the end of time having requests and/or PRs for net19.0 TFM specific builds a week after it's released, and/or a phalanx of PRs across all the projects constantly updating the min depended-on versions of System.* backports as they age out.
So I guess a high level one liner policy (with a rationale) that crisply defines the way in which a version gets selected would be ideal. But then you get into stuff like making sure it covers how that is then applied:
- for an initial release
- for minor/patch releases (should they up a system deps that do not have a vulnerability)
- when moving to a new major versions (should the policy be to up the min dep to align with the current LTS or something?)
As a default, I personally for a small lib would thus tend to start with a minimal dep and only the dependency at the point where there's a concrete benefit for the implementation's clarity, or the overall performance.
Over time, I'd be hoping that scanners and/or .NET can have a better answer for making sure apps get the latest versions of all libs on a reasonable schedule, with no confusing messaging suggesting that the lib NEEDs to be updated when that's not actually the case.
In other words, having Serilog's policies appear to cover your transitive dependency versioning is not ever going to solve overall problems for apps (the app owner still ultimately needs to take ownership of transitive deps for plenty libraries anyway), and is a distraction for Serilog.
There was a problem hiding this comment.
I'll swap it over to 8, but I would add that this is one of the weird Micorosoft-isms of package versions being tied directly to .NET versions, so realistically, package version 8 is for .NET 8 and package version 6 is for .NET 6 and not necessarily that package version 8 has more or different features (though it probably does because, Microsoft).
It may be worth looking into how Microsoft expects you to reference these packages for .netstandard builds etc, because I honestly don't know and I couldn't find anything in my rather cursory search. I wish it was more clear what Microsoft expected in cases like this so things don't break randomly.
There was a problem hiding this comment.
Over time, I'd be hoping that scanners and/or .NET can have a better answer for making sure apps get the latest versions of all libs on a reasonable schedule, with no confusing messaging suggesting that the lib NEEDs to be updated when that's not actually the case.
This seems like the right outcome to me, too. I'd bet it's on someone's radar 🤔 ... a nontrivial problem to solve.
@mprice-gcmlp I think in general, if the package supports the TFM it'll run on it. Things can get a bit hazy around the edges (I've seen this fall apart, too), but having these packages able to run on older .NET Framework versions means a lot of the work to adapt to older API sets is being done and tested in there. Agreed it's a confusing situation :-)
Fixes #80