-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix .NET 5 FileStream compat detection in the test utility project #57288
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
Fix .NET 5 FileStream compat detection in the test utility project #57288
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-io Issue Details
runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs Lines 7 to 9 in a489488
|
| } | ||
|
|
||
| private static readonly Lazy<bool> _net5CompatFileStream = new Lazy<bool>(() => GetStaticNonPublicBooleanPropertyValue("System.IO.FileStreamHelpers", "UseNet5CompatStrategy")); | ||
| private static readonly Lazy<bool> _net5CompatFileStream = new Lazy<bool>(() => GetStaticNonPublicBooleanPropertyValue("System.IO.Strategies.FileStreamHelpers", "UseNet5CompatStrategy")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make this throw if the type can't be found?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about it, but that would require special handling for when the PlatformDetection type is used to run Full .NET Framework tests. I decided to just add a test instead. Is that OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok.
But I don't fully understand... tests on .NET Framework access _net5CompatFileStream.Value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Adam meant (please correct me if I'm wrong) that the PlatformDectection class, which contains the _net5CompatFileStream field, is also consumed by some .NET Framework tests. I can see the file can be built for both net60 and net461:

What I do not know is what special handling would be required to make the exception work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've forgotten that it's lazy and only compiled for .NET 4.6.1 (not executed).
carlossanlop
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Pending answering the question posted by Stephen.
| } | ||
|
|
||
| private static readonly Lazy<bool> _net5CompatFileStream = new Lazy<bool>(() => GetStaticNonPublicBooleanPropertyValue("System.IO.FileStreamHelpers", "UseNet5CompatStrategy")); | ||
| private static readonly Lazy<bool> _net5CompatFileStream = new Lazy<bool>(() => GetStaticNonPublicBooleanPropertyValue("System.IO.Strategies.FileStreamHelpers", "UseNet5CompatStrategy")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Adam meant (please correct me if I'm wrong) that the PlatformDectection class, which contains the _net5CompatFileStream field, is also consumed by some .NET Framework tests. I can see the file can be built for both net60 and net461:

What I do not know is what special handling would be required to make the exception work.
FileStreamHelperswas moved fromSystem.IOtoSystem.IO.Strategiesnamespaceruntime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs
Lines 7 to 9 in a489488