Pipelines.Sockets.Unofficial icon indicating copy to clipboard operation
Pipelines.Sockets.Unofficial copied to clipboard

Upgrade SIOP dependency

Open jasper-d opened this issue 2 years ago • 8 comments

  • Add new target .net6.0 and upgrade SIOP to 6.0.3 for this new target only
    • Add overrides for CanGetUnflushedBytes and UnflushedBytes to WrappedWriter for .net6.0/SIOP 6 only

jasper-d avatar Jul 21 '22 19:07 jasper-d

@jasper-d What is the intent overall here? I'm asking because this breaks a great deal of projects, both those depending on earlier TFMs and especially anyone dealing with binding redirects. Most libraries can rely on the minimum required version for the APIs they need, thereby offering the most flexibility and surface area for consumers. If we upgrade dependencies to require later versions, that causes a lot of real practical headaches.

Fixes are good, but limiting where this can be used and making upgrades painful is less so...and this is used by some rather large downstream package volume, so it'll cause quite a few of those headaches.

NickCraver avatar Jul 21 '22 20:07 NickCraver

@NickCraver Primarily I would like to use UnflushedBytes and ReadAtLeastAsync, otherwise I would need to do the accounting myself. I believe upgrading SIOP alone shouldn't break anyone, given that even 6.0.3 still supports .net461?! But maybe that's just me who lives in a happy world were only .NET 6+ is a thing.

Updating TFMs was meant as housekeeping, but shouldn't be required. I can revert those if you wish. Same for the auxiliary nuget packages.

jasper-d avatar Jul 21 '22 22:07 jasper-d

@jasper-d Follow-up then: where do you want to use UnflushedBytes? I don't see it used in the library here, only in tests, which means the library dependency need not upgrade. You can always use a newer version in your downstream project as long as there are no breaking changes in that newer version, the library doesn't need to require everyone to upgrade :)

NickCraver avatar Jul 21 '22 22:07 NickCraver

You can always use a newer version in your downstream project as long as there are no breaking changes in that newer version, the library doesn't need to require everyone to upgrade :)

I tried that, but when referencing this library and SIOP 6.0.3 explicitly from a .NET 6 application I still get a NotSupportedException when calling UnflushedBytes. I think WrappedWriter would need to delegate that call to _writer to make it work, because that's the actual DefaultPipeWriter which supports it. The abstract PipeWriter WrappedWriter derives from doesn't.

But delegating won't work (or at least I wouldn't know how), because the members simply don't exist when targeting SIOP < 6.

EDIT:

where do you want to use UnflushedBytes

From my client code that uses Pipelines.Sockets.Unofficial.

jasper-d avatar Jul 21 '22 22:07 jasper-d

@jasper-d Gotcha - so that sounds like an argument for adding (and only adding) a net6.0 build which has a later dependency and lights up those new members, that's the approach I'd take here. Hopefully Marc has time to chime in there later, just saw this and was curious :)

NickCraver avatar Jul 21 '22 22:07 NickCraver

The addition of a net6 build and some new overrides for new features: no problem at all. Some of the other changes look suspect and would need further reading. My advice would be: limit the PR to exactly what you need for this feature - leave out the TFM cull etc.

mgravell avatar Jul 22 '22 06:07 mgravell

Two things caught my eye in particular (in a cursory read):

  1. It looks like a #if NET461 became a #if NET462 in code that already had a net462 target. This appears to have the effect of changing how the net462 code operates, rather than simply removing the net461 specialization
  2. That csproj change with the very unusual package ref condition: I'm going to need a comment on what that condition is trying to achieve, and why it isn't the vanilla package ref

mgravell avatar Jul 22 '22 06:07 mgravell

I've reverted all non-necessary changes, added .net6 as a new target. and upgraded SIOP only for .net6 as suggested. I'll file a separate PR with test fixes.

jasper-d avatar Jul 22 '22 15:07 jasper-d