Skip to content
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

Upgrade SIOP dependency #68

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

jasper-d
Copy link
Contributor

@jasper-d jasper-d commented Jul 21, 2022

  • 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

@NickCraver
Copy link
Collaborator

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

@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
Copy link
Contributor Author

jasper-d commented Jul 21, 2022

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
Copy link
Collaborator

@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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants