mgravell / Pipelines.Sockets.Unofficial

.NET managed sockets wrapper using the new "Pipelines" API
Other
400 stars 51 forks source link

Upgrade SIOP dependency #68

Open jasper-d opened 1 year ago

jasper-d commented 1 year ago
NickCraver commented 1 year ago

@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.

jasper-d commented 1 year ago

@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.

NickCraver commented 1 year ago

@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 :)

jasper-d commented 1 year ago

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.

NickCraver commented 1 year ago

@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 :)

mgravell commented 1 year ago

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 commented 1 year ago

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
jasper-d commented 1 year ago

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.