Start using System.Text.Encoding.Web from CLR

[muratg]

No description provided.

[Eilon]

This is now available on the internal share that we use, so this is unblocked.

[muratg]

@Tratcher

[Tratcher]

@Eilon @muratg 4.0.0-beta-23326 is not compatible with .NET 4.5.1, it requires 4.6. It depends on System.Runtime 4.0.20, where we need 4.0.10 for 4.5.1.

[Tratcher]

@ericstj ?

[ericstj]

It's compiling against quite a few contracts that are only supported in .NET 4.6.

I gave a try at moving it to contract versions that would support .NET 4.5.1 and it is not possible because the implementation depends on Buffer.MemoryCopy which was added in .NET 4.6. This appears to be the only dependency that is preventing portability to even to .NET 4.5. @KrzysztofCwalina

[muratg]

@ericstj thanks, @KrzysztofCwalina Should we file a bug on your side to track this? Otherwise we'll need to maintain our implementation at least for Desktop CLR.

[KrzysztofCwalina]

I will fix this tomorrow. No need to file a new bug.

[muratg]

@KrzysztofCwalina Sounds good, thanks!

[davidfowl]

Ifdefs will set you free

[KrzysztofCwalina]

@ericstj, most of our projects in corefx depend on System.Runtime 4.0.20.

[ericstj]

That's because most of these don't intend to run downlevel. There are a few that have downlevel scenarios (like Immutable & Reflection.Metadata) and intentionally restrict their dependencies to versions that will work on those downlevel platforms.

[KrzysztofCwalina]

@ericstj, almost all (if not all) components in corefx depend on 4.0.20. And it seems like ASP.NET needs to and is successfully using them on 4.5.1 (i.e. downlevel)

[Eilon]

Right, all of DNX, ASP.NET, and EF (with maybe some very small exceptions, where it's usually runtime light-up) need to run on .NET 4.5.1.

[KrzysztofCwalina]

@muratg, where you able to successfully pick up the updated package?

[muratg]

@KrzysztofCwalina Not yet. @Tratcher will be picking it up in the dev branch for RC1 and close this bug.

[Tratcher]

@KrzysztofCwalina Which build should it be in? 4.0.0-beta-23405 still depends on 4.0.20.

[ericstj]

This version has the updates: https://www.myget.org/F/dotnet-core/api/v2/package/System.Text.Encodings.Web/4.0.0-beta-23406

[Tratcher]

Hey @ericstj, I've got a PR for the swap, but we've found that the new types are much harder to mock out in tests because Encode isn't virtual. Here's an attempt at mocking (90c13ad#diff-387f3dc26a94ebcb7c37dd754bab84c5), but it only gets one character at a time so the best mock encoding I can manage is encoding "value" to "_v_a_l_u_e". We used to return "HtmlEncode[[value]]" (90c13ad#diff-5093aee64e24c262ac1165902a6ceef9L89).

This may require us to keep using our interfaces and wrapper classes and only delegate the implementation.
/cc: @muratg @Eilon @davidfowl

[KrzysztofCwalina]

Seems like that would be throwing baby with the bathwater. Can you use the mocks that add the underscore for now and I will make the Encode method virtual (next week)?

[Tratcher]

Not really, it breaks too many tests to update in the short term. I could make my first check in still using the interfaces and postpone removing them until next week. @muratg? There are plenty of changes that we'll need to make upstream regardless (method names, parameter order, etc.).

[KrzysztofCwalina]

That sounds good, I think, i.e. the tests would need to keep the old interfaces, not the library, correct?

[Tratcher]

The libraries will still need to use the interfaces or else the tests won't be able to mock out the encoders used in the libraries.

[KrzysztofCwalina]

I see. I just submitted a fix to corefx. If everything goes well, you should have a new drop tomorrow.