Skip to content
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

Remove System.IO.Pipelines from ASP.NET Core shared framework #24392

Closed
davidfowl opened this issue Jul 29, 2020 · 17 comments
Closed

Remove System.IO.Pipelines from ASP.NET Core shared framework #24392

davidfowl opened this issue Jul 29, 2020 · 17 comments
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework Done This issue has been fixed

Comments

@davidfowl
Copy link
Member

It's now in Microsoft.NETCore.App with this PR dotnet/runtime#39524

@davidfowl davidfowl added the feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform label Jul 29, 2020
@dougbu
Copy link
Member

dougbu commented Jul 29, 2020

I'm pretty sure removal will be automatic but we'll need to adjust our shared Fx and targeting pack tests in whatever dependency PR gets the updated runtime bits. Basically, this issue is something to keep in mind for our ops-on-call person (currently @wtgodbe).

@ericstj will System.IO.Pipelines.dll show up in Microsoft.NETCore.App.Ref as well as Microsoft.NETCore.App.Runtime.* packages❔

@JunTaoLuo
Copy link
Contributor

JunTaoLuo commented Jul 29, 2020

We might need to update some of files though. I think we would no longer need this: https://github.com/dotnet/aspnetcore/blob/master/eng/Dependencies.props#L68. This can be done by ops during rotation where we get the dependency update.

@dougbu
Copy link
Member

dougbu commented Jul 29, 2020

Agreed @JunTaoLuo though that line might not break the dependency PR's build -- as the list of expected content in our tests will.

We probably won't need https://github.com/dotnet/aspnetcore/blob/master/eng/SharedFramework.External.props#L43 either unless this turns out to be one of the oddities included in Microsoft.NETCore.App.Runtime.* but not Microsoft.NETCore.App.Ref.

@ericstj
Copy link
Member

ericstj commented Jul 29, 2020

@ericstj will System.IO.Pipelines.dll show up in Microsoft.NETCore.App.Ref as well as Microsoft.NETCore.App.Runtime.* packages

It should. I also noticed a packaging problem in that PR that needs to be corrected, so be on the lookout for that.

@davidfowl
Copy link
Member Author

Is this solved now?

@wtgodbe
Copy link
Member

wtgodbe commented Aug 6, 2020

@captainsafia did this get fixed during your ops duty? CC @dougbu

@dougbu
Copy link
Member

dougbu commented Aug 6, 2020

I don't think #24496 needed to change the shared Fx tests and that indicates we either don't have recent-enough runtime dependencies (surprisingly) or changes mentioned above are needed to remove duplicate copies of the assembly. So, the question is really whether we see System.IO.Pipelines.dll in both Microsoft.NETCore.App.Runtime and Microsoft.AspNetCore.App.Runtime packages. If yes, we're up-to-date and need to remove that duplication.

@davidfowl
Copy link
Member Author

Who is doing this work?

@davidfowl
Copy link
Member Author

Can somebody direct on how to do this?

@wtgodbe
Copy link
Member

wtgodbe commented Aug 15, 2020

I can take a look Monday

@dougbu
Copy link
Member

dougbu commented Aug 15, 2020

@pranavkm fixed this in 56be06b aka "Update test runner to 2.4.3 (#24685)". We just forgot to close the issue.

@dougbu dougbu closed this as completed Aug 15, 2020
@dougbu dougbu added the Done This issue has been fixed label Aug 15, 2020
@davidfowl
Copy link
Member Author

I don't see it in the latest builds. When I install RC1 I still see pipelines

@dougbu
Copy link
Member

dougbu commented Aug 15, 2020

@davidfowl that likely means our builds w/ 56be06b in them are not yet part of the installer you tried. But, our tests focus in the *.nupkg assets and don't double-check e.g. the Zip files.

@wtgodbe can you please download the shared Fx and targeting pack outputs from a recent official build of 'master' to confirm none contain System.IO.Pipelines❔ Reopen this issue if we missed something.

@davidfowl
Copy link
Member Author

The installer is what matters. It should be trivial to check if it's up to date with ASP.NET right?

@dougbu
Copy link
Member

dougbu commented Aug 15, 2020

I shouldn't have used the word "likely". I'd already checked the staleness and dotnet/installer is 10 days or 35 builds behind dotnet/aspnetcore. That's much older than @pranavkm's changes.

My question to @wtgodbe was about confirming things will work correctly when dotnet/installer catches up.

@davidfowl
Copy link
Member Author

Why is it so far behind...

@dougbu
Copy link
Member

dougbu commented Aug 15, 2020

I think the hold-up is mostly around ingesting dotnet/sdk into dotnet/installer. See discussions in dotnet/installer#8129

dotnet/sdk is at least caught up to our builds from 4 days ago.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 14, 2020
@JunTaoLuo JunTaoLuo added area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework and removed feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform labels Jan 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests

5 participants