Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Get Nancy compiling on dotnet54/dnxcore50 #2220

Closed
12 tasks done
Yantrio opened this issue Feb 3, 2016 · 10 comments
Closed
12 tasks done

Get Nancy compiling on dotnet54/dnxcore50 #2220

Yantrio opened this issue Feb 3, 2016 · 10 comments
Milestone

Comments

@Yantrio
Copy link
Contributor

Yantrio commented Feb 3, 2016

Proposal

As part of the latest microsoft movement, it would be nice to get nancy working on the new dotnet environment.
This was started with pull requests #2211 and #2212 to get things compiling using the project.json project system. Now we need to get some good work done to get this working on dotnet54.

Approach

So far, there has been analysis completed on the nancy package to see what is/isn't available in the latest runtimes, and there are some areas that need replacing.
Due to the amount of work that needs doing, It's been suggested by @thecodejunkie that we try to get the project compiling first, then approach tests later on once we are happy and settled with the replacement code.

Most areas of the code should remain the same for the full CLR, and ifdefs should be used to swap out logic with comparable parts in the new environment.

Why choose dotnet54 and not dnxcore50?

Dotnet54 is intended for use by microsoft by libraries and not the consuming applications, more information can be found here : aspnet/Announcements#98

Things that need tackling to get things compiling

Things to note:

@khellang has asked for a list of packages and why we use them so we can try and trim the list of dependencies down, do we need some place to put this? maybe a wiki page in the long run?

@thecodejunkie thecodejunkie added this to the coreclr milestone Feb 3, 2016
@thecodejunkie
Copy link
Member

@khellang has asked for a list of packages and why we use them so we can try and trim the list of dependencies down, do we need some place to put this? maybe a wiki page in the long run?

This is a good thing to bring up in the description of each of the pull-requests

@khellang
Copy link
Member

khellang commented Feb 3, 2016

RNGCryptoServiceProvider -> RandomNumberGenerator.Create()

I don't think there's a need to #ifdef anything here. The RandomNumberGenerator.Create() API is available on both net451 and dotnet5.4

RijndaelEncryptionProvider makes use of RijndaelManaged class, not available.

This can be replaced using the Aes class in the https://www.nuget.org/packages/System.Security.Cryptography.Algorithms package 👍

CultureInfo caching (ie BuiltInCultureConventions class) needs replacing

What does this mean? Are there any specific APIs that we use, that's not available?

Uri.IsHexDigit/Uri.FromHex() is not available, suitable replacement needed.

Let's just rip the original implementations 😈 They're trivial.

MemoryStream.GetBuffer is not available, suitable replacement needed

First, let's try to use TryGetBuffer. If that fails, and we can live with copying the data, let's just use ToArray here. Not sure where it's used.

Most reflection needs tweaking/replacing.

This is mostly just a matter of adding in a .GetTypeInfo() call everywhere 👯

Url class implements ICloneable, it's not available, is this an issue?

No, just 🔥 it. If it's heavily used, we can keep the method, but 🔥 the interface.

Removal of SerializableAttribute
BootstrapperException Ctor (SerializationInfo info, StreamingContext context)

These can easily be #ifdef'ed away, if we really need them, but in general, I think we should try to 🔥 them 😁

Just a couple of ¢ from me 😉

@grumpydev
Copy link
Member

Agree with most of what @khellang said, few notes:

As I said on the call, CultureInfo caching is the biggest concern I have - we put that in because the performance of doing it non-cached is stupid slow (like drop to 30 requests per second slow).

I cloneable I think we might use, but doing it without the interface is easy enough.

Bootstrapper exception - AFAIK this is the "best practice" way of making exception ctors, pretty sure we don't ever use that ctor so happy to nuke it :)

BinaryFormatter - can replace this with JSON once we have simple json in, although it's going to be a lot slower - surprised there isn't a replacement for this to be honest.

GetBuffer is (IIRC) a lot more efficient than ToArray so this is really going to nail perf when we have a stream switch I think.

The rest of the stream stuff is a reasonable chunk of work to switch over to using all the async methods, but it definitely something we need to do (along with update the response class)

@thecodejunkie
Copy link
Member

Everything on the checklist is done, are the additional items we should be adding to it or shall we close this issue?

@phillip-haydon
Copy link
Member

Woo that's awesome.

@jchannon
Copy link
Member

I guess add whatever the remaining errors are?

On 23 February 2016 at 07:13, Phillip Haydon [email protected]
wrote:

Woo that's awesome.


Reply to this email directly or view it on GitHub
#2220 (comment).

@jchannon
Copy link
Member

pinging @Yantrio

@Yantrio
Copy link
Contributor Author

Yantrio commented Feb 24, 2016

I think if @thecodejunkie is happy that we are compiling we can close this and open more issues if we need to further down the line.

@thecodejunkie
Copy link
Member

👍

@Yantrio
Copy link
Contributor Author

Yantrio commented Feb 25, 2016

WE DID IT GUYS!

Closed

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

No branches or pull requests

6 participants