Stop harvesting S.IO.Pipes.AccessControl #52294

ViktorHofer · 2021-05-05T11:19:24Z

The removed configurations (netstandard1.3, netcoreapp2.1, net46) aren't
built anymore. Instead the already built matching binaries from the
latest available packages are redistributed when packaging these
libraries.

Dropping these assets as the minimum supported set of platforms are ones
that support netstandard2.0 and are still in-support. As an example,
also dropping the netcoreapp2.1 asset which isn't supported anymore and
upgrading it to netcoreapp3.1.. For .NET Framework, there's still a
net461 configuration present to avoid binding redirect issues.

cc @carlossanlop

Contributes to #47530

The removed configurations (netstandard1.3, netcoreapp2.1, net46) aren't built anymore. Instead the already built matching binaries from the latest available packages are redistributed when packaging these libraries. Dropping these assets as the minimum supported set of platforms are ones that support netstandard2.0 and are still in-support. As an example, also dropping the netcoreapp2.1 asset which isn't supported anymore and upgrading it to netcoreapp3.1.. For .NET Framework, there's still a net461 configuration present to avoid binding redirect issues. The ugly part of this change is that System.IO.Pipes.AccessControl depends on an internal API of System.IO.Pipes for all .NETCoreApp configurations. Because of that, for configurations that don't apply to the current band (netcoreapp3.1, net5.0), the runtime packs need to be downloaded so that runtime assembly can be referenced. Ideally we will come up with a better solution to not require runtime packs to be fetched as that adds 60MB of download size to the repo and breaks source build. Contributes to dotnet#47530

ghost · 2021-05-05T11:19:31Z

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

The removed configurations (netstandard1.3, netcoreapp2.1, net46) aren't
built anymore. Instead the already built matching binaries from the
latest available packages are redistributed when packaging these
libraries.

Dropping these assets as the minimum supported set of platforms are ones
that support netstandard2.0 and are still in-support. As an example,
also dropping the netcoreapp2.1 asset which isn't supported anymore and
upgrading it to netcoreapp3.1.. For .NET Framework, there's still a
net461 configuration present to avoid binding redirect issues.

The ugly part of this change is that System.IO.Pipes.AccessControl
depends on an internal API of System.IO.Pipes for all .NETCoreApp
configurations. Because of that, for configurations that don't apply
to the current band (netcoreapp3.1, net5.0), the runtime packs need
to be downloaded so that runtime assembly can be referenced. Ideally
we will come up with a better solution to not require runtime packs
to be fetched as that adds 60MB of download size to the repo and breaks
source build.

@ericstj I'm considering checking in the netcoreapp3.1 and net5.0 Pipes runtime assembly into the repository directly so that they can be referenced and the runtime packs don't need to be fetched. What are your thoughts on this? Also we need to come up with a better solution as otherwise this will plague us for the entire existence of this package.

cc @carlossanlop

Contributes to #47530

Author:	ViktorHofer
Assignees:	ViktorHofer
Labels:	`area-Infrastructure-libraries`
Milestone:	6.0.0

src/libraries/System.IO.Pipes.AccessControl/src/System.IO.Pipes.AccessControl.csproj

ericstj

I'm OK with this. It's a bit of a trick which I'd want the IO owners to sign off on. I'd want to make sure that whatever questions they have we document in comments so we give future devs a chance at understanding what's going on here.

ericstj · 2021-05-06T16:17:35Z

src/libraries/System.IO.Pipes/ref/System.IO.Pipes.csproj

  <ItemGroup>
    <Compile Include="System.IO.Pipes.cs" />
+    <Compile Include="System.IO.Pipes.Internal.cs" Condition="'$(TargetFramework)' != '$(NetCoreAppCurrent)'" />


I guess this name is OK. I couldn't think of anything better.

I scanned existing patterns of that and found couple of ".Internal" files so I renamed it from ".Internals" to ".Internal".

Second attempt (not sure why GitHub deleted my original message):

"*TypeForwards.cs"? I don't mind the currently selected name, since it seems to align with other similar patterns.

Even thought that file is the input to generate type forwards, it doesn't contain any. It only lists non exposed APIs which is why we chose the "Internal" wording.

carlossanlop

LGTM. I left a few questions but mainly for my own education, not blocking.

carlossanlop · 2021-05-10T20:40:03Z

src/libraries/System.IO.Pipes.AccessControl/src/Resources/Strings.resx

@@ -57,6 +57,15 @@
  <resheader name="writer">
    <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
  </resheader>
+  <data name="ArgumentOutOfRange_NeedValidPipeAccessRights" xml:space="preserve">


What was the reason for the addition of these resource strings? I don't see src changes in this PR, which is why I ask.

These strings are used by the added netstandard2.0 build.

carlossanlop · 2021-05-10T20:40:57Z

src/libraries/System.IO.Pipes.AccessControl/pkg/System.IO.Pipes.AccessControl.pkgproj

@@ -2,13 +2,9 @@
  <Import Project="$([MSBuild]::GetPathOfFileAbove(Directory.Build.props))" />
  <ItemGroup>
    <ProjectReference Include="..\ref\System.IO.Pipes.AccessControl.csproj">
-      <SupportedFramework>$(NetCoreAppCurrent)</SupportedFramework>
+      <SupportedFramework>net461;netcoreapp2.0;uap10.0.16299;$(AllXamarinFrameworks)</SupportedFramework>


For my own education, why was uap added again? I remember it was removed some time ago.

uap10.0.16299 supports netstandard2.0. As we are producing such an asset, we mark uap10.0.16299 as a supported framework.

carlossanlop · 2021-05-10T20:56:44Z

src/libraries/System.IO.Pipes.AccessControl/ref/System.IO.Pipes.AccessControl.cs

@@ -65,14 +65,4 @@ public partial class PipeSecurity : System.Security.AccessControl.NativeObjectSe
        public void SetAccessRule(System.IO.Pipes.PipeAccessRule rule) { }
        public void SetAuditRule(System.IO.Pipes.PipeAuditRule rule) { }
    }
-
-    public static class AnonymousPipeServerStreamAcl


I now remember these APIs were added to .NET Core with the intention of bringing ACL support to pipes, which already existed in .NET Framework.

There were other ACL cases in which I worked were the new extensions class needed to be consumable both from .NET Core and .NET Framework. It seems it's not the case with these two classes, so this change makes sense (within my limited understanding of the infra changes).

carlossanlop · 2021-05-10T20:58:23Z

src/libraries/System.IO.Pipes.AccessControl/ref/System.IO.Pipes.AccessControl.csproj

    <Nullable>enable</Nullable>
  </PropertyGroup>
+  <!-- DesignTimeBuild requires all the TargetFramework Derived Properties to not be present in the first property group. -->
+  <PropertyGroup>
+    <IsPartialFacadeAssembly Condition="'$(TargetFramework)' == 'net461'">true</IsPartialFacadeAssembly>


Why was it necessary to add this net461 check (along with others)? It was my understanding that the *PipeServerStreamAcl APIs were only available in .NET Core.

We are generating a partial facade for System.IO.Pipes.AccessControl so that the package can be used on .NET Framework as well so that customers don't need to condition their PackageReferences for different TFMs. I can share more details (perhaps offline) in case you are interested.

ViktorHofer added the area-Infrastructure-libraries label May 5, 2021

ViktorHofer added this to the 6.0.0 milestone May 5, 2021

ViktorHofer requested a review from ericstj May 5, 2021 11:19

ViktorHofer self-assigned this May 5, 2021

ghost added the new-api-needs-documentation label May 5, 2021

ViktorHofer mentioned this pull request May 5, 2021

Replace harvested package assets with live built configurations #47530

Closed

25 tasks

ViktorHofer added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. and removed new-api-needs-documentation labels May 5, 2021

ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label May 5, 2021

dotnet deleted a comment May 5, 2021

ericstj reviewed May 5, 2021

View reviewed changes

src/libraries/System.IO.Pipes.AccessControl/src/System.IO.Pipes.AccessControl.csproj Outdated Show resolved Hide resolved

Use non shipping refs for Pipes typeforwards

7c22dc7

ericstj requested a review from carlossanlop May 6, 2021 16:12

ericstj approved these changes May 6, 2021

View reviewed changes

ericstj requested review from jozkee and adamsitnik May 6, 2021 16:20

carlossanlop approved these changes May 10, 2021

View reviewed changes

ViktorHofer merged commit 089717b into dotnet:main May 11, 2021

ViktorHofer deleted the StopHarvestPipesAccessControl branch May 11, 2021 15:40

ghost locked as resolved and limited conversation to collaborators Jun 10, 2021

ViktorHofer removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Sep 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Stop harvesting S.IO.Pipes.AccessControl #52294

Stop harvesting S.IO.Pipes.AccessControl #52294

Uh oh!

ViktorHofer commented May 5, 2021 •

edited

Loading

Uh oh!

ghost commented May 5, 2021

Uh oh!

Uh oh!

ericstj left a comment

Uh oh!

ericstj May 6, 2021

Uh oh!

ViktorHofer May 6, 2021

Uh oh!

carlossanlop May 10, 2021

Uh oh!

ViktorHofer May 11, 2021

Uh oh!

carlossanlop left a comment

Uh oh!

carlossanlop May 10, 2021

Uh oh!

ViktorHofer May 11, 2021

Uh oh!

carlossanlop May 10, 2021

Uh oh!

ViktorHofer May 11, 2021

Uh oh!

carlossanlop May 10, 2021

Uh oh!

carlossanlop May 10, 2021

Uh oh!

ViktorHofer May 11, 2021

Uh oh!

Uh oh!

Stop harvesting S.IO.Pipes.AccessControl #52294

Stop harvesting S.IO.Pipes.AccessControl #52294

Uh oh!

Conversation

ViktorHofer commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 5, 2021

Uh oh!

Uh oh!

ericstj left a comment

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!

carlossanlop left a comment

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!

Uh oh!

ViktorHofer commented May 5, 2021 •

edited

Loading