search

dotnet / standard

This repo is building the .NET Standard
3.06k stars 427 forks source link

FileStream dependencies #52

Closed ramarag closed 8 years ago

ramarag commented 8 years ago

Remove ctor(System.IO.FileStream) from "System.Reflection.StrongNameKeyPair see https://github.com/dotnet/coreclr/pull/7530#issuecomment-252404800

weshaggard commented 8 years ago

@jakotas what about the other FileStream dependencies from Reflection? Like https://github.com/dotnet/standard/blob/master/netstandard/ref/mscorlib.cs#L7581 Assembly.GetFiles?

danmoseley commented 8 years ago

You mean @jkotas

jkotas commented 8 years ago

I forgot about those. Well, we can either move FileStream to System.Runtime/System.Private.CoreLib; or drop these from netstandard20. Either one is fine with me.

Pretty much any core runtime implementation I can think of needs to access files. I do not think we are getting a lot of positive value by keeping it up stack (except for the WinRT cruft - but that can be dealt with in isolation) - if the FileStream is upstack, the core runtime needs to have its own LowLevelFileStream sort of type anyway.

Given this, I would lean towards moving FileStream to System.Runtime/System.Private.CoreLib.

jkotas commented 8 years ago

Related issues:

FileStream refactoring https://github.com/dotnet/corefx/issues/11707 Assembly.GetFile/Assembly.GetFiles https://github.com/dotnet/corefx/issues/11655 Dependencies on FileStream: https://github.com/dotnet/corefx/issues/11151

weshaggard commented 8 years ago

I do think we should consider moving FileStream into System.Runtime/S.P.CoreLib. However I worry a little about the closure both API and Implementation.

I'd like to get some other opinions on this before deciding to do it. @ericstj @stephentoub @JeremyKuhne @ianhays any thoughts on potential issues before we attempt to do this?

stephentoub commented 8 years ago

There are a bunch of places we've had to workaround the lack of FileStream low enough in the stack, e.g. using reflection from StreamWriter.ctor(string), having to P/Invoke directly to libc from System.Console to read terminfo files, etc. There are lots of good reasons to move it lower in the stack.

weshaggard commented 8 years ago

I had a closer look at the API surface and from an API point of view moving down FileStream into System.Runtime isn't bad(see https://github.com/weshaggard/corefx/commit/e77e2b39371e5e3f202e4f8222bf3f73c9a70455) the only questions I have is about the implementation dependencies and how much trouble they will give in corelib, especially WinRT stuff.

weshaggard commented 8 years ago

With that I've opened https://github.com/dotnet/corefx/issues/12633 to do that instead of removing these for netstandard.

@ramarag that means we will need to still expose these FileStream dependent APIs once FileStream is moved down.