React to runtime/594 Encoder.Convert change

[Tratcher]

dotnet/runtime#594 is reverting a 3.1 fix to encoders for compat reasons. That might have side-effects for our use of encoders so we're pre-emptively updating AspNetCore as well.

This change works because we know WriteMultiSegmentEncoded is always called with a complete string, there's no tearing, so we can always flush the encoder.

[Pilchie]

👀

[Pilchie]

@aspnet/build we've done away with patchconfig stuff, right?

[Pilchie]

@GrabYourPitchforks I thought you had detailed two affected locations?

[Tratcher]

@GrabYourPitchforks I thought you had detailed two affected locations?

@Pilchie we reviewed the other two uses of this API and determined that they were not affected by the change.

[GrabYourPitchforks]

@Pilchie I believe this is the only location that would be affected by the 3.1.1 change. I had called out another location that has a buggy call site, but that second call site is buggy regardless of which runtime it's running against. The 3.1.1 change won't affect it.

[wtgodbe]

@aspnet/build we've done away with patchconfig stuff, right?

We've done so in 3.0, but haven't merged that forward to 3.1 yet: https://github.com/aspnet/AspNetCore/blob/9a4ab80f7e068e30da5ea3c819b546b511b64302/Directory.Build.targets#L61-L64

We intend to merge it forward with the rest of #17311 (CC @dougbu @JunTaoLuo)

[Pilchie]

@mmitche Any reason not to merge this? @GrabYourPitchforks are the other changes ready/merged?

[mmitche]

@mmitche Any reason not to merge this? @GrabYourPitchforks are the other changes ready/merged?

They aren't ready yet. I think we should hold those changes for 3.1.2.

[analogrelay]

@Pilchie is this approved? It still has servicing-consider, so I've put it in 3.1.x since we generally don't put things in a specific patch milestone until they're approved.

[mmitche]

Not approved for 3.1.1

[Pilchie]

It's been conceptually approved at Tactics, but that was before the PR was actually created. However, as @mmitche says, we'll hold it from 3.1.1 at this point as the base changes aren't going to make 3.1.1.

[analogrelay]

It's been conceptually approved at Tactics, but that was before the PR was actually created. However, as @mmitche says, we'll hold it from 3.1.1 at this point as the base changes aren't going to make 3.1.1.

I guess my question is this: Does tactics need to further review this change? If no, we can mark servicing-approved and move to 3.1.2 to resolve the confusion (it's clear that it needs no further approval from Tactics, but does need to wait until the branch is open for 3.1.2-approved changes). If yes, we leave it as-is (servicing-consider and 3.1.x) :).

[Pilchie]

I'm going to bring up the slip to 3.1.2 in Tactics tomorrow to make sure everyone is aware. Once that happens, we'll move to servicing-approved and 3.1.2.

[analogrelay]

Putting * NO MERGE* (not that it does much) just to make clear that the branch is not open for 3.1.2 changes yet.

[jamshedd]

Approved for 3.1.2

[vivmishra]

Moved to Mar as per Tactics. Will need to be explicitly approved for Feb, if required.

[jkotalik]

@anurse I believe this got bumped back to feb now. Please confirm when you have the chance 😄

[analogrelay]

Correct, but @Tratcher I believe this is reacting to a change that hasn't actually happened in runtime's 3.1.x branch. Can you check on that?

[Tratcher]

@GrabYourPitchforks?

[analogrelay]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).