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

Removing the use of RefPath and binplacing for old frameworks #31844

Closed
5 tasks done
Anipik opened this issue Feb 6, 2020 · 10 comments · Fixed by #35606
Closed
5 tasks done

Removing the use of RefPath and binplacing for old frameworks #31844

Anipik opened this issue Feb 6, 2020 · 10 comments · Fixed by #35606

Comments

@Anipik
Copy link
Contributor

Anipik commented Feb 6, 2020

//edited by @ViktorHofer, copied from #31844 (comment)

Make our non-NetCoreAppCurrent projects behave more like normal SDK projects with respect to references.
Eliminate the use of artifacts\bin\ref as the reference path for all frameworks exception NetCoreAppCurrent. Eliminate the need of depprojs.

Next steps:

  • Enable restore for ref and src projects: Enable restore for ref and src projects in libraries #33242
  • Allow non-NetCoreAppCurrent projects to use implicit shared framework (which requires restore).
  • Add PackageReferences to any project that relies on a package that was previously restored by depprojs. EG: System.Memory for NETStandard2.0. Make sure to use version.props to represent the dependency version: Replace binplace.depproj by inlining PackageReferences #33422
  • Handle references to other live-built libraries. EG: System.Collections.Immutable depends on System.Reflection.Metadata. This could either be done with a projectreference to the reference assembly, or a projectreference to the src assembly.
  • Once done for all projects, delete depprojs that were responsible for building out ref packs. Ideally we can delete all depprojs and just meld whatever remaining restores were happening into toolset restore.

cc @ericstj @ViktorHofer

@ViktorHofer
Copy link
Member

It has been some time since I last used implicit framework references. Do I remember correctly that when enabling the build would use the default references that come with the framework and by either referencing via ProjectReferences or named references conflict resolution would overrule the default references with the live built references? Does all that apply to .NET Framework as well? cc @ericstj

@ericstj
Copy link
Member

ericstj commented Feb 6, 2020

This is more/less true. We may need to fix / modify conflict resolution to work with ProjectReferences: dotnet/sdk#2674. .NETFramework has a behavior defined by SDK + Arcade, we'd use that.

We better not need conflict resolution with .NETFramework, that would imply we're OOBing .NETFramework assemblies... which isn't very popular... Even so, yes it works for .NETFramework.

I think in most of our cases we wouldn't need ProjectReferences because we'd instead want to compile against the minimum thing provided by the framework and then our packaging infra would deal with getting the right version for PackageReferences. Testing infra (today) would still run on latest. We'd only need the P2P's when we wanted to use new surface area.

@ViktorHofer
Copy link
Member

We better not need conflict resolution with .NETFramework, that would imply we're OOBing .NETFramework assemblies... which isn't very popular... Even so, yes it works for .NETFramework.

Funny thing, we already do but not because of any reason, just for consistency with netcoreapp.

I more or less understand what you are proposing here but I thing we should write this down in more detail.

@ViktorHofer
Copy link
Member

@ericstj just discussed with @Anipik that will tackle this. Can you please help us identify goals and required steps here? Thanks

@ericstj
Copy link
Member

ericstj commented Feb 19, 2020

Goal:
Make our non-NetCoreAppCurrent projects behave more like normal SDK projects with respect to references.
Eliminate the use of artifacts\bin\ref as the reference path for all frameworks exception NetCoreAppCurrent. Eliminate the need of depprojs.

Next steps:

  • Enable restore for ref and src projects.
  • Allow non-NetCoreAppCurrent projects to use implicit shared framework (which requires restore).
  • Add PackageReferences to any project that relies on a package that was previously restored by depprojs. EG: System.Memory for NETStandard2.0. Make sure to use version.props to represent the dependency version.
  • Handle references to other live-built libraries. EG: System.Collections.Immutable depends on System.Reflection.Metadata. This could either be done with a projectreference to the reference assembly, or a projectreference to the src assembly.
  • Once done for all projects, delete depprojs that were responsible for building out ref packs. Ideally we can delete all depprojs and just meld whatever remaining restores were happening into toolset restore.

@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Feb 24, 2020
@ViktorHofer ViktorHofer self-assigned this Mar 8, 2020
@ViktorHofer
Copy link
Member

Assigning myself as well as the project restore work is already ongoing. @Anipik my work also covers some of the other todos so I recommend to not start with this work yet until I can clearly draw a line.

@ViktorHofer
Copy link
Member

ViktorHofer commented Mar 9, 2020

@ericstj I'm copying your instructions to the top post.

@Anipik
Copy link
Contributor Author

Anipik commented Mar 9, 2020

Thanks @ViktorHofer for letting me know. I will wait for ur work to go in and then we can evaluate about which tasks are still left.

@Anipik
Copy link
Contributor Author

Anipik commented Apr 6, 2020

@ViktorHofer can i start working on this now ?

@ViktorHofer
Copy link
Member

Thanks Anirudh but I already started with the next steps in a private branch.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants