Change TFM to netcoreapp2.0 #6234

pranavkm · 2017-05-04T16:22:41Z

No description provided.

dougbu

Need to recheck our assumptions on what's available in .NET Core. 2.0 is way more inclusive.

dougbu · 2017-05-04T19:33:52Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/AmbiguousActionException.cs


 namespace Microsoft.AspNetCore.Mvc.Internal
 {
    /// <summary>
    /// An exception which indicates multiple matches in action selection.
    /// </summary>
-#if NET46
-    [Serializable]


If [Serializable] is available in .NET Core, use it.

dougbu · 2017-05-04T19:35:06Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/NonDisposableStream.cs

@@ -108,49 +108,6 @@ public override Task<int> ReadAsync(byte[] buffer, int offset, int count, Cancel
            return _innerStream.ReadAsync(buffer, offset, count, cancellationToken);
        }

-#if NET46
-        /// <inheritdoc />
-        public override IAsyncResult BeginRead(


Aren't these methods part of the .NET Core surface? Need to be overridden if they're now included.

dougbu · 2017-05-04T19:36:54Z

src/Microsoft.AspNetCore.Mvc.Formatters.Xml/Internal/FormattingUtilities.cs

@@ -15,13 +12,6 @@ public static class FormattingUtilities
    {
        public static readonly int DefaultMaxDepth = 32;

-#if NET46
-        public static readonly XsdDataContractExporter XsdDataContractExporter = new XsdDataContractExporter();


Bit surprised XsdDataContractExporter isn't included in .NET Core App. Thought the XML assemblies were the subject of a long .NET Standard discussion and that the decision was to included almost everything.

dougbu · 2017-05-04T19:40:02Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/Internal/CryptographyAlgorithms.cs

-            catch (System.Reflection.TargetInvocationException)
-            {
-                // Fallback to a FIPS compliant SHA256 algorithm.
-                sha256 = new SHA256CryptoServiceProvider();


If SHA256CryptoServiceProvider is available in .NET Core App, may need to handle this fallback on FIPS machines.

I thought it worked correctly in netcoreapp. @blowdart ?

I believe it does.

My intent was twofold: Do we need this fallback? And, if it's required, does SHA256CryptoServiceProvider exist? So, we've resolved only the second part.

If SHA256.Create() does not work correctly on FIPS machines, we need this fallback.

SHA256.Create() in netcoreapp works correctly on FIPS machines.

@blowdart is my statement correct? We don't need to worry about the desktop specific code here because calling SHA256.Create() works correctly in netcoreapp?

Yes it's correct. Core is FIPS compliant by default, there's no choice.

dougbu · 2017-05-04T19:44:57Z

test/Microsoft.AspNetCore.Mvc.Razor.Test/Compilation/ViewsFeatureProviderTest.cs

@@ -150,30 +128,5 @@ public ViewInfoContainer2()
            {
            }
        }
-
-#if NET46
-        private class AssemblyWithEmptyLocation : Assembly


Is this still impossible in .NET Core?

dougbu · 2017-05-04T19:45:39Z

test/Microsoft.AspNetCore.Mvc.Core.Test/NonDisposableStreamTest.cs

-            var nonDisposableStream = new NonDisposableStream(innerStream);
-
-            // Act
-            nonDisposableStream.Close();


Still no Stream.Close() in .NET Core App?

dougbu · 2017-05-04T19:47:59Z

test/Microsoft.AspNetCore.Mvc.WebApiCompatShimTest/DefaultContentNegotiatorTest.cs

-
-                    { new List<MediaTypeFormatterMatch>() { matchMapping05, matchAccept10 }, matchAccept10 },
-                    { new List<MediaTypeFormatterMatch>() { matchMapping05, matchAcceptSubTypeRange10 }, matchAcceptSubTypeRange10 },
-                    { new List<MediaTypeFormatterMatch>() { matchMapping05, matchAcceptAllRange10 }, matchAcceptAllRange10 },


Why as this part of the data set specific to .NET Framework?

Uses APIs that aren't available in the portable System.Net.Http.Formatting.

dougbu · 2017-05-04T19:48:35Z

...t.AspNetCore.Mvc.WebApiCompatShimTest/HttpRequestMessage/HttpRequestMessageExtensionsTest.cs

@@ -278,32 +278,6 @@ public void CreateResponse_AcceptingFormatter_WithOverridenMediaTypeHeader_Creat
            Assert.Equal("bin/baz", response.Content.Headers.ContentType.MediaType);
        }

-#if NET46
-        // API doesn't exist in CoreCLR.


What API and is this still true?

InvalidByteRangeException

And that's not in .NET Core App 2.0?

It's from System.Net.Http.Formatting.

Got it, thx.

pranavkm · 2017-05-04T21:23:08Z

🆙 📅

dougbu

Very close.

dougbu · 2017-05-04T22:03:15Z

src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlDataContractSerializerOutputFormatter.cs

@@ -132,13 +132,7 @@ protected virtual DataContractSerializer CreateSerializer(Type type)

            try
            {
-#if NET46
-                // Verify that type is a valid data contract by forcing the serializer to try to create a data contract
-                FormattingUtilities.XsdDataContractExporter.GetRootElementName(type);


You've restored FormattingUtilities.XsdDataContractExporter. Should restore this too.

dougbu · 2017-05-04T22:06:45Z

...t.AspNetCore.Mvc.WebApiCompatShimTest/HttpRequestMessage/HttpRequestMessageExtensionsTest.cs

@@ -278,32 +278,6 @@ public void CreateResponse_AcceptingFormatter_WithOverridenMediaTypeHeader_Creat
            Assert.Equal("bin/baz", response.Content.Headers.ContentType.MediaType);
        }

-#if NET46
-        // API doesn't exist in CoreCLR.


And that's not in .NET Core App 2.0?

dougbu · 2017-05-04T22:11:14Z

test/Microsoft.AspNetCore.Mvc.WebApiCompatShimTest/DefaultContentNegotiatorTest.cs

-
-                    { new List<MediaTypeFormatterMatch>() { matchMapping05, matchAccept10 }, matchAccept10 },
-                    { new List<MediaTypeFormatterMatch>() { matchMapping05, matchAcceptSubTypeRange10 }, matchAcceptSubTypeRange10 },
-                    { new List<MediaTypeFormatterMatch>() { matchMapping05, matchAcceptAllRange10 }, matchAcceptAllRange10 },


dougbu · 2017-05-04T22:13:39Z

Separately, XmlSerializerInputFormatter isn't conditionally-compiled. Why are you deleting XmlSerializerInputFormatterTest?

pranavkm · 2017-05-04T23:01:20Z

🆙 📅

dougbu · 2017-05-04T23:07:14Z

src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlDataContractSerializerOutputFormatter.cs

@@ -132,13 +132,7 @@ protected virtual DataContractSerializer CreateSerializer(Type type)

            try
            {
-#if NET46
-                // Verify that type is a valid data contract by forcing the serializer to try to create a data contract
-                FormattingUtilities.XsdDataContractExporter.GetRootElementName(type);


ctolkien · 2017-05-05T00:47:18Z

A lot of people are really confused (and freaking out, myself included) as to what is going on here. It's no longer targeting NetStandard?! We can't use this against Full Framework?

Change TFM to netcoreapp2.0 (#6234)

Eilon · 2017-05-05T18:26:34Z

@ctolkien - Please refer to dotnet/aspnetcore#2022 (comment) for some more details on this.

pranavkm requested review from Eilon and dougbu May 4, 2017 16:22

dnfclas added the cla-already-signed label May 4, 2017

pranavkm force-pushed the prkrishn/netcoreapp branch 2 times, most recently from 3a67a96 to 1cf31a8 Compare May 4, 2017 18:53

dougbu suggested changes May 4, 2017

View reviewed changes

Change TFM to netcoreapp2.0

48c9b18

pranavkm force-pushed the prkrishn/netcoreapp branch from 1cf31a8 to 48c9b18 Compare May 4, 2017 20:32

pranavkm changed the base branch from dev to rel/2.0.0-preview1 May 4, 2017 20:33

Changes per PR comments

7a7c0ca

dougbu suggested changes May 4, 2017

View reviewed changes

Revive XmlSerializerInputFormatterTest

bb358d7

dougbu approved these changes May 4, 2017

View reviewed changes

pranavkm merged commit 1c5e417 into rel/2.0.0-preview1 May 5, 2017

kevinchalet mentioned this pull request May 5, 2017

The new ASP.NET Core 2.0 packages can no longer be used on .NET Desktop dotnet/aspnetcore#2022

Closed

pranavkm added a commit that referenced this pull request May 5, 2017

Merge pull request #6236 from aspnet/rel/2.0.0-preview1

64ffcfa

Change TFM to netcoreapp2.0 (#6234)

smitpatel deleted the prkrishn/netcoreapp branch June 28, 2017 21:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change TFM to netcoreapp2.0 #6234

Change TFM to netcoreapp2.0 #6234

pranavkm commented May 4, 2017

dougbu left a comment

dougbu May 4, 2017

dougbu May 4, 2017

dougbu May 4, 2017

dougbu May 4, 2017

pranavkm May 4, 2017

blowdart May 4, 2017

dougbu May 4, 2017 •

edited

Loading

pranavkm May 4, 2017

pranavkm May 4, 2017

blowdart May 4, 2017

dougbu May 4, 2017

dougbu May 4, 2017

dougbu May 4, 2017

pranavkm May 4, 2017

dougbu May 4, 2017

dougbu May 4, 2017

pranavkm May 4, 2017

dougbu May 4, 2017

pranavkm May 4, 2017

dougbu May 4, 2017

pranavkm commented May 4, 2017

dougbu left a comment

dougbu May 4, 2017

pranavkm May 4, 2017

dougbu May 4, 2017

dougbu May 4, 2017

dougbu May 4, 2017

dougbu commented May 4, 2017

pranavkm commented May 4, 2017

dougbu May 4, 2017

ctolkien commented May 5, 2017

Eilon commented May 5, 2017

Change TFM to netcoreapp2.0 #6234

Change TFM to netcoreapp2.0 #6234

Conversation

pranavkm commented May 4, 2017

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu May 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented May 4, 2017

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented May 4, 2017

pranavkm commented May 4, 2017

Choose a reason for hiding this comment

ctolkien commented May 5, 2017

Eilon commented May 5, 2017

dougbu May 4, 2017 •

edited

Loading