Move S.IO.FileAccess to corelib. by alexperovich · Pull Request #12206 · dotnet/corefx

alexperovich · 2016-09-29T19:59:33Z

This enum is used by UnmanagedMemoryStream which is required for ResourceManager.GetStream()

Fixes #12169

stephentoub · 2016-09-29T20:04:53Z

+    <TargetingPackReference Include="mscorlib" />
+  </ItemGroup>
  <ItemGroup Condition="'$(TargetGroup)' != 'net46'">
-    <Compile Include="System\IO\FileAccess.cs" />


If nothing else still uses the file, it should be deleted, too.

alexperovich · 2016-09-29T21:58:56Z

@weshaggard

weshaggard · 2016-09-30T06:01:07Z

  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'net46_Debug|AnyCPU'" />
  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'net46_Release|AnyCPU'" />
+  <ItemGroup>
+    <TargetingPackReference Include="mscorlib" />


This change is going to clash with #12187, where @jkotas is moving everything to build against System.Private.CoreLib.

weshaggard · 2016-09-30T06:03:31Z

    <AssemblyName>System.IO.FileSystem.Primitives</AssemblyName>
-    <IsPartialFacadeAssembly Condition="'$(TargetGroup)' == 'net46'">true</IsPartialFacadeAssembly>
+    <IsPartialFacadeAssembly>true</IsPartialFacadeAssembly>
    <NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETStandard,Version=v1.3</NuGetTargetMoniker>


You need to bump the assembly version and the target framework. for this library. See my docs which are still in PR #12036, and please add feedback to that PR if you have further questions.

I have bumped the versions

weshaggard

Need to version.

This enum is used by UnmanagedMemoryStream which is required for ResourceManager.GetStream() Fixes #12169

alexperovich · 2016-09-30T20:05:39Z

@weshaggard PTAL

weshaggard · 2016-10-01T06:05:58Z

  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
  <ItemGroup>
    <Project Include="System.IO.FileSystem.Primitives.Tests.csproj"/>
+    <Project Include="System.IO.FileSystem.Primitives.Tests.csproj">


@joperezr this will conflict with your PR.

weshaggard · 2016-10-01T22:15:01Z

  <ItemGroup>
    <Project Include="System.IO.FileSystem.Primitives.Tests.csproj"/>
    <Project Include="System.IO.FileSystem.Primitives.Tests.csproj">
-      <TestTFMs>netcore50;net46</TestTFMs>


why did you remove the netcore50 test configuration? I would expect we should still make sure we test that configuration.

I'll put it back.

Moreover, why'd you change this at all?

I didn't catch this when I changed this back. Will fix.

We should still keep netcore50 as a TestTFM.

Which would mean you still need a netstandard1.3 build configuration for this project.

weshaggard · 2016-10-01T22:15:57Z

  <Import Project="..\dir.props" />
  <PropertyGroup>
-    <AssemblyVersion>4.0.2.0</AssemblyVersion>
+    <AssemblyVersion>4.0.3.0</AssemblyVersion>


We generally bump minor versions but this is just moving one type down. @ericstj what is your thoughts on version bumping in this case?

If all that was changed was impl then we don't need to change version at all (I typically do that at the start of a release).

alexperovich · 2016-10-02T10:45:49Z

@dotnet-bot test Innerloop Ubuntu14.04 Release Build and Test please (Build Timeout)

weshaggard · 2016-10-03T16:07:16Z

    <Project Include="System.IO.FileSystem.Primitives.Tests.csproj">
-      <TestTFMs>netcore50;net46</TestTFMs>
+      <TargetGroup>netstandard1.7</TargetGroup>
+      <TestTFMs>netcoreapp1.1</TestTFMs>


Looks like @joperezr PR got in before yours. Can you please adjust this .builds file and csproj such that the netstandard1.7 is the default configuration. See #12241 as an example

weshaggard · 2016-10-03T16:07:52Z

+    "netstandard1.7": {}
  },
  "supports": {
-    "coreFx.Test.netcore50": {},


You will need to also add back this "coreFx.Test.netcore50" section otherwise it will not run for the netcore50 configuration.

I put it back

alexperovich · 2016-10-03T18:48:12Z

@dotnet-bot test Innerloop CentOS7.1 Debug Build and Test please (disk IO issue)

alexperovich · 2016-10-03T18:48:20Z

@dotnet-bot test Innerloop CentOS7.1 Release Build and Test please (disk IO issue)

joperezr · 2016-10-04T16:57:27Z

-      "imports": [
-        "dotnet5.4"
-      ]
+        "Microsoft.TargetingPack.Private.CoreCLR": "1.2.0-beta-24603-02"


"Microsoft.TargetingPack.Private.CoreCLR": "1.2.0-beta-24603-02" [](start = 8, length = 64)

Why did we make this change to bind to coreclr instead of going through System.Runtime like we did before?

This change is necessary because UnmanagedMemoryStream has a ctor that takes a FileAccess enum and ResourceManager depends on UnmanagedMemoryStream. All of these types need to be in corelib.

Does this not cause warnings with other duplicated types coming from System.Private.CoreLib?

It does not.

ericstj · 2016-10-04T19:30:15Z

  <ItemGroup>
    <Project Include="System.IO.FileSystem.Primitives.Tests.csproj"/>
    <Project Include="System.IO.FileSystem.Primitives.Tests.csproj">
-      <TestTFMs>netcore50;net46</TestTFMs>


Moreover, why'd you change this at all?

alexperovich · 2016-10-04T19:49:21Z

/cc @ianhays

ericstj · 2016-10-04T21:16:07Z

-    "coreFx.Test.net46": {},
-    "coreFx.Test.net461": {},
-    "coreFx.Test.net462": {},
+    "coreFx.Test.netcoreapp1.1": {},


Why was this required?

This is required because netcoreapp1.1 is the default TestTFM now and the project file doesn't change that.

But now if someone what's to run this on net46-net462 they can't?

This file is unchanged now

weshaggard · 2016-10-04T21:47:57Z

  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'net46_Release|AnyCPU'" />
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcore50aot_Debug|AnyCPU'" />
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcore50aot_Release|AnyCPU'" />
+  <ItemGroup Condition="'$(TargetGroup)' == ''">


nit: can you please include with the other reference below.

weshaggard · 2016-10-04T22:00:30Z

      <TargetGroup>net46</TargetGroup>
    </Project>
+    <Project Include="System.IO.FileSystem.Primitives.csproj">
+      <TargetGroup>netcore50aot</TargetGroup>


I think this needs to be netcore50

weshaggard · 2016-10-04T22:02:42Z

    <ProjectGuid>{6C05678E-394C-4CFF-B453-A18E28C8F2C3}</ProjectGuid>
    <AssemblyName>System.IO.FileSystem.Primitives</AssemblyName>
-    <IsPartialFacadeAssembly Condition="'$(TargetGroup)' == 'net46'">true</IsPartialFacadeAssembly>
+    <IsPartialFacadeAssembly Condition="'$(TargetGroup)' != 'netcore50aot'">true</IsPartialFacadeAssembly>


I do not think we should be bothering with keeping the netcore50 build live. Delete it instead - it is what we are doing everywhere else.

We cannot remove this netcore50 build. This package was portable and now is not portable to .net native.

I do not understand. If we remove it, the package will still contain the repackaged netcore50 binary. There are many cases like this already.

There isn't a netcore50 binary in the previous stable package

Then just bump the default build to netstandard1.7 like I am doing it here: #12187.

alexperovich · 2016-10-04T23:22:30Z

@dotnet-bot test this please

alexperovich · 2016-10-05T17:39:12Z

@dotnet-bot test Innerloop Ubuntu14.04 Debug Build and Test please (Build Timeout)

weshaggard · 2016-10-05T19:03:03Z

    <OutputType>Library</OutputType>
    <AssemblyName>System.IO.FileSystem.Primitives.Tests</AssemblyName>
-    <NugetTargetMoniker>.NETStandard,Version=v1.3</NugetTargetMoniker>
+    <NugetTargetMoniker>.NETStandard,Version=v1.7</NugetTargetMoniker>


Once you add the netstandard1.3 build configuration you will need to conditions this on an empty targetgroup.

@ericstj

* Move S.IO.FileAccess to corelib. This enum is used by UnmanagedMemoryStream which is required for ResourceManager.GetStream() Fixes dotnet/corefx#12169 * Fix Dependencies * Delete FileAccess.cs * Bump Versions * Fix test configurations * Update dependencies * Fix dependencies * Add back netcore50 test TFM * Update dependency versions * Make netcoreapp1.1 the default test tfm * Update dependencies * Update based on feedback from @ericstj * Remove unneeded changes in tests * Change test TFM to net46 * Reorder items and change netcore50aot to netcore50 * Remove a left behind aot * Remove netcoreapp1.1 from test project.json * Fix dependencies * Retarget to ns1.7 * Update package index * Test on netstandard1.3 Commit migrated from dotnet/corefx@8a0f80e

dnfclas added the cla-already-signed label Sep 29, 2016

stephentoub reviewed Sep 29, 2016

View reviewed changes

weshaggard reviewed Sep 30, 2016

View reviewed changes

weshaggard suggested changes Sep 30, 2016

View reviewed changes

alexperovich added 4 commits September 30, 2016 10:32

Move S.IO.FileAccess to corelib.

15e6f9f

This enum is used by UnmanagedMemoryStream which is required for ResourceManager.GetStream() Fixes #12169

Fix Dependencies

1c2b472

Delete FileAccess.cs

d4ff5d7

Bump Versions

47df02f

alexperovich force-pushed the resources branch from c454e59 to 47df02f Compare September 30, 2016 18:14

Fix test configurations

96dc8a2

alexperovich added 2 commits September 30, 2016 13:38

Update dependencies

a68f69f

Fix dependencies

7e66ce3

weshaggard reviewed Oct 1, 2016

View reviewed changes

alexperovich added 2 commits October 1, 2016 20:44

Add back netcore50 test TFM

9c1dc37

Update dependency versions

43c7cb3

weshaggard reviewed Oct 3, 2016

View reviewed changes

alexperovich added 3 commits October 3, 2016 10:21

Merge branch 'master' into resources

59985bf

Make netcoreapp1.1 the default test tfm

b09c282

Update dependencies

763a4e1

alexperovich added 2 commits October 3, 2016 13:56

Update based on feedback from @ericstj

c3c03c8

Remove unneeded changes in tests

5da9518

joperezr reviewed Oct 4, 2016

View reviewed changes

ericstj suggested changes Oct 4, 2016

View reviewed changes

Change test TFM to net46

c669820

ericstj reviewed Oct 4, 2016

View reviewed changes

weshaggard reviewed Oct 4, 2016

View reviewed changes

Reorder items and change netcore50aot to netcore50

c40c6a5

weshaggard reviewed Oct 4, 2016

View reviewed changes

Remove a left behind aot

5ffef47

Remove netcoreapp1.1 from test project.json

e39b590

alexperovich added 4 commits October 5, 2016 10:52

Fix dependencies

2648eaa

Retarget to ns1.7

7304cb4

Merge branch 'master' into resources

2c6e251

Update package index

70c3784

weshaggard reviewed Oct 5, 2016

View reviewed changes

Test on netstandard1.3

65474ac

ericstj approved these changes Oct 5, 2016

View reviewed changes

jkotas approved these changes Oct 5, 2016

View reviewed changes

alexperovich merged commit 8a0f80e into dotnet:master Oct 5, 2016

alexperovich deleted the resources branch October 5, 2016 20:33

karelz modified the milestone: 1.2.0 Dec 3, 2016

Conversation

alexperovich commented Sep 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexperovich commented Sep 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weshaggard left a comment

Choose a reason for hiding this comment

Uh oh!

alexperovich commented Sep 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexperovich commented Oct 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexperovich commented Oct 3, 2016

Uh oh!

alexperovich commented Oct 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexperovich commented Oct 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!