-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only include System.Threading.Channels ref for needed targets #81
Merged
Merged
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't need to be 6.0 for these TFMs, 8.0 supports them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andnet6.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 ofSystem.*
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bartelink
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 :-)