dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.21k stars 4.72k forks source link

Move host components out of installer into a common location #49233

Open ViktorHofer opened 3 years ago

ViktorHofer commented 3 years ago

To my knowledge the host components consist of the native source files, nuget packages, the managed HostModel library and tests. Most of these assets are located under src/installer as that's where they lived before repository consolidation (core-setup repo). I propose moving these components together to common place out of src/installer:

src/host
  - Microsoft.NET.HostModel
  - native (+ msbuild entrypoint: corehost.proj)
  - packages
  - tests
  - Solution File (for opening inside VS)

As the HostModel library isn't publicly shipping but maintains the contract with the host, it doesn't belong into libraries as that's where we ship public APIs from. Aside from that, the host tests require the HostModel library and the HostModel tests aren't unit tests but end-to-end tests which we currently don't have under src/libraries.

I don't propose to change any of the subset names or the host dependencies (i.e. shared framework packs for testing).

cc @agocke @vitek-karas @VSadov @jkotas

ghost commented 3 years ago

Tagging subscribers to this area: @dotnet/runtime-infrastructure See info in area-owners.md if you want to be subscribed.

Issue Details
To my knowledge the host components consist of the native source files, nuget packages, the managed HostModel library and tests. Most of these assets are located under src/installer as that's where they lived before repository consolidation (core-setup repo). I propose moving these components together to common place out of src/installer: ``` src/host - Microsoft.NET.HostModel - native (+ msbuild entrypoint: corehost.proj) - packages - tests - Solution File (for opening inside VS) ``` As the HostModel library isn't publicly shipping but maintains the contract with the host, it doesn't belong into libraries as that's where we ship public APIs from. Aside from that, the host tests require the HostModel library and the HostModel tests aren't unit tests but end-to-end tests which we currently don't have under src/libraries. I don't propose to change any of the subset names or the host dependencies (i.e. shared framework packs for testing). cc @agocke @vitek-karas @VSadov @jkotas
Author: ViktorHofer
Assignees: -
Labels: `Discussion`, `area-Infrastructure`
Milestone: -
ViktorHofer commented 3 years ago

Any concerns with the proposal? If not I would start decoupling the components from the src/installer infrastructure and move bits by bits.

jkoritzinsky commented 3 years ago

The proposal sounds good to me.

VSadov commented 3 years ago

The native part has been moved recently to src\native\corehost, since it is a shared component. It is used by Mono too and the singlefile part is used by clr partition build. I think that is where it should be.

The rest looks reasonable, but I wonder how much will stay under src\installer

jkoritzinsky commented 3 years ago

The eventual goal is to remove src/installer and have the subfolders under src better match the subset category names (host, packs).

VSadov commented 3 years ago

Yes, that is what I thought. Once host and packs are out, I do not think there is something that belongs to "installer".

jkoritzinsky commented 3 years ago

Technically, we could keep the dotnet-host.proj, dotnet-hostfxr.proj , the dotnet-runtime-prereqs folder and the bundle project in src/installer since they only build installers. Depends on what we want to do.

ViktorHofer commented 3 years ago

Once host and packs are out, I do not think there is something that belongs to "installer".

Absolutely. That's tracked via https://github.com/dotnet/runtime/issues/49428.

Technically, we could keep the dotnet-host.proj, dotnet-hostfxr.proj , the dotnet-runtime-prereqs folder and the bundle project in src/installer since they only build installers. Depends on what we want to do.

That discussion probably belongs into https://github.com/dotnet/runtime/issues/49426. I would be fine to keep it as-is (installers as part of packs) for simplicity.

The native part has been moved recently to src\native\corehost, since it is a shared component. It is used by Mono too and the singlefile part is used by clr partition build. I think that is where it should be.

We usually group sources in dotnet/runtime by component and less by language (i.e. src/libraries/*, coreclr, mono, tasks). The existing src\native\corehost folder would be renamed to src\host\native which wouldn't prevent it from being a shared component, nor indicate that these files aren't shared components. Thoughts?

jkotas commented 3 years ago

We usually group sources in dotnet/runtime by component and less by language

We do not have one unifying scheme how to group sources together. It is not unusual to see sources grouped by language - src/libraries/native is example of such grouping. Or to group shared or common sources used from more than one place - src/libraries/Common, src/coreclr/tools/Common or src/native are examples of such groupings.

The existing src\native\corehost folder would be renamed to src\host\native which wouldn't prevent it from being a shared component

I agree that it would not hurt, but I do not see it helping much either.

ViktorHofer commented 3 years ago

We do not have one unifying scheme how to group sources together. It is not unusual to see sources grouped by language - src/libraries/native is example of such grouping. Or to group shared or common sources used from more than one place - src/libraries/Common, src/coreclr/tools/Common or src/native are examples of such groupings.

Putting the first time contributor hat on I would say it's highly confusing that a "src/native" folder exists. It suggests that it contains the native source files of the repository even though it only contains a very limited set of it. Even though we might not be consistent today doesn't mean we shouldn't strive towards a consistent scheme.

src/libraries/native

The native folder under libraries actually made perfect sense as it was the home of all native code in CoreFx before repositories were consolidated. IMO it doesn't make much sense anymore in it's current form.

Or to group shared or common sources used from more than one place - src/libraries/Common, src/coreclr/tools/Common or src/native are examples of such groupings.

Absolutely agree on the necessity of shared source folders. The first two examples make perfect sense as their paths clearly indicate which component their shared sources belong.

jkotas commented 3 years ago

Even though we might not be consistent today doesn't mean we shouldn't strive towards a consistent scheme.

Agree. I think we may want to work towards gradually moving all native code from the whole repo under src/native. Questions like this come up in number of discussion, e.g. https://github.com/dotnet/runtime/pull/47639#discussion_r582500870

The native folder under libraries actually made perfect sense as it was the home of all native code in CoreFx before repositories were consolidated. IMO it doesn't make much sense anymore in it's current form.

What would make more sense in the current repo in your opinion?

eerhardt commented 3 years ago

The existing src\native\corehost folder would be renamed to src\host\native which wouldn't prevent it from being a shared component

I agree that it would not hurt, but I do not see it helping much either.

The reason I would think src/host makes sense is because I would expect all the assets related to the "host" would be in one spot:

Splitting these assets across top-level folders would be confusing IMO.

If you wanted to have both src/host (for most of the host assets) and src/native/corehost (for the corehost C code), I guess that could work. Is the thinking that the corehost C code would be shared with something else? If yes, what? My thinking is that coreclr and mono are self-contained. And host would "live on top" of them, just like libraries does today.

jkoritzinsky commented 3 years ago

With the single-file host, the separation between the various groups of native code has kind of blurred.

vitek-karas commented 3 years ago

We could in theory look at single-file host as just a packaging thing. It in itself actually introduces only a small amount of special code. For the most part it's just a different way how to package our assets. It's not that different from runtime installer, or runtime pack.

I personally would also prefer organizing the code by the feature (host, runtime, libraries, ...) and not the language it's written in.

ViktorHofer commented 3 years ago

What would make more sense in the current repo in your opinion?

If we already have the src/native folder, I would just move everything which is shared into it. For anything else that is distinct (ie https://github.com/dotnet/runtime/tree/main/src/libraries/Native/Windows/System.IO.Compression.Native) would it make sense to move them next to managed component? If that isn't feasible (ie necessity of build wrapper scripts per component), should we just move everything into src/native?

We could in theory look at single-file host as just a packaging thing. It in itself actually introduces only a small amount of special code. For the most part it's just a different way how to package our assets. It's not that different from runtime installer, or runtime pack.

I'm not exactly what you mean by that. Could you please elaborate?

jkotas commented 3 years ago

necessity of build wrapper scripts per component

It is doable, but pain to maintain.

should we just move everything into src/native?

Right, that was my thinking.

am11 commented 3 years ago

Another library Microsoft.NETCore.Platforms from core-setup repo was moved under src/libraries. From MSBuild restore perspective, it has similar constraints as HostModel. If HostModel is moved under src/libraries, it wouldn't be too strange compared to other more stranger stuff we have in that directory. The only thing we need to care about is when do we run its tests. That can be remain tied to host.tests and packs.tests subsets.

I was able to do the mechanical work for it in commit 'Move HostModel under src/libraries' in: https://github.com/dotnet/runtime/compare/main...am11:feature/hostmodel-move, which is rebased on branch of PR #57036.

The benefit, IMO, is that this approach makes 'host packages' very independent and we can then rename src/installer to src/hostpacks or src/packs (something that resembles its subset name: packs). This folder will only left with bunch of proj/pkgproj files and give us a chance to rehash various properties/targets which is gluing together miscellaneous stuff.

ViktorHofer commented 3 years ago

I was able to do the mechanical work for it in commit 'Move HostModel under src/libraries' in: main...am11:feature/hostmodel-move, which is rebased on branch of PR #57036.

IMO it makes more sense to have everything host related under src/host.

am11 commented 3 years ago

everything host related under src/host.

by just renaming src/installer to src/host? (since besides hostmodel+tests and pkgs, there is nothing else left in that directory)

OK. I was thinking it a little bit differently; we already reference src/installer/pkg in src/libraries so in a sense they are not decoupled anyway. Which is why I thought a library project with src+tests is best suited under libraries rather than one off exception (that seldom gets maintenance updates).

ViktorHofer commented 3 years ago

by just renaming src/installer to src/host? (since besides hostmodel+tests and pkgs, there is nothing else left in that directory)

No, I still believe that the proposal in the top post makes sense. The remaining part of src/installer which is the shared framework and the publishing logic is tracked by https://github.com/dotnet/runtime/issues/49428.

am11 commented 3 years ago

@ViktorHofer, it makes sense to me as well. 🙂 Since corehost (native project) is now under src/native, what do you think about a slight modification to the proposed structure:

src/host
  - Microsoft.NET.HostModel
    - src/
    - tests/
  - packages
  - Solution File (for opening inside VS)

the prerequisites for Solution File to successfully build in VS / with MSBuild will continue to be coreclr and/or mono, libs and host.native subsets are built first. I think it is a similar prereq as solution files under src/libraries, which interop with src/libraries/Native and require libs.native subset to be built first.

vitek-karas commented 3 years ago

Would we also move host tests from src/installer/tests to src/host/tests then? (It seems it would make sense). I'm not sure I like the fact that "host" source code is NOT in src/host if we add it though.

am11 commented 3 years ago

Would we also move host tests from src/installer/tests to src/host/tests then? (It seems it would make sense).

Yup, it will be either src/host/{Microsoft.NET.HostModel,tests} or with the structure we use for other libraries:src/host/Microsoft.NET.HostModel/{src,tests}.

I'm not sure I like the fact that "host" source code is NOT in src/host if we add it though.

It consists of HostModel's src, tests, packages and corehost (native). The native bits were moved to neutral place src/native/corehost because it is shared with coreclr, mono and host. In terms of subset, corehost mainly builds as part of host.native.

jkotas commented 3 years ago

I would useful to write down the principle that we are going to use to decided whether a component or a feature area should get its own top-level directory.

The host does not look that special to me. I think it would be better to make its tests fit into the existing test structures (either src/tests or libraries/*/tests).

ViktorHofer commented 3 years ago

I would useful to write down the principle that we are going to use to decided whether a component or a feature area should get its own top-level directory.

I agree that we need some clarity on this. Examples are src/tests or src/tasks which IMO don't belong there. src/tests implies tests for the repo being there where-as only runtime tests can be found there. src/tasks was considered the home for repo tasks which are used during the build (and hence need to be restored and built before anything else) and which are by definition non-shipping. In reality many of these tasks are now shipping and because of the lack of build dependencies all tasks are now by default built upfront which is a waste of build time for i.e. non-mobile configurations.

The host does not look that special to me. I think it would be better to make its tests fit into the existing test structures (either src/tests or libraries/*/tests).

Wasn't the host since the incarnation of corefx, coreclr and core-setup special as it only lived in core-setup because of the lack of a better place? It sounds to me that it doesn't belong into libraries nor into the runtime folders. As we would like to remove the src/installer partition, host related sources need to move to a different location. As the host test end-to-end I don't think they belong either into the libraries tests or the runtime tests.

jkotas commented 3 years ago

src/tests implies tests for the repo being there where-as only runtime tests can be found there

I look at src/tests as the place for everything that is not a pure managed API, where the libraries test setup that is designed for testing pure managed APIs would not work well.

Wasn't the host since the incarnation of corefx, coreclr and core-setup special as it only lived in core-setup because of the lack of a better place?

Yes, the host was special back then. I do not think it is as special in the merged repo.

am11 commented 3 years ago

Related to top-level directories, I think src/workloads is also something that probably does not warrant a top-level directory for two project files. (could be under src/mono)

SamMonoRT commented 2 years ago

@trylek - does your test consolidation work have any effect on any of the above discussion? At this point moving to 8.0.0 to avoid any stability issues so late in the cycle.

trylek commented 2 years ago

@SamMonoRT - I think this discussion is mostly unrelated to the pending test consolidation which only deals with runtime (CoreCLR & Mono) tests, not hosting tests.