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

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

Open
ViktorHofer opened this issue Mar 5, 2021 · 29 comments
Open

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

ViktorHofer opened this issue Mar 5, 2021 · 29 comments

Comments

@ViktorHofer
Copy link
Member

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
Copy link

ghost commented Mar 5, 2021

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
Copy link
Member Author

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

@jkoritzinsky
Copy link
Member

The proposal sounds good to me.

@VSadov
Copy link
Member

VSadov commented Mar 10, 2021

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
Copy link
Member

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

@VSadov
Copy link
Member

VSadov commented Mar 10, 2021

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

@jkoritzinsky
Copy link
Member

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
Copy link
Member Author

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

Absolutely. That's tracked via #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 #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
Copy link
Member

jkotas commented Mar 10, 2021

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
Copy link
Member Author

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
Copy link
Member

jkotas commented Mar 10, 2021

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. #47639 (comment)

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
Copy link
Member

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:

  • corehost C code
  • Packaging logic for the host NuGet packages
  • Microsoft.NET.HostModel C# code
  • Tests

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
Copy link
Member

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

@vitek-karas
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

jkotas commented Mar 15, 2021

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.

@ViktorHofer ViktorHofer removed the untriaged New issue has not been triaged by the area owner label Mar 24, 2021
@ViktorHofer ViktorHofer added this to the 6.0.0 milestone Mar 24, 2021
@hoyosjs hoyosjs modified the milestones: 6.0.0, 7.0.0 Jul 30, 2021
@am11
Copy link
Member

am11 commented Aug 8, 2021

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: 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
Copy link
Member Author

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
Copy link
Member

am11 commented Aug 8, 2021

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
Copy link
Member Author

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 #49428.

@am11
Copy link
Member

am11 commented Aug 9, 2021

@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
Copy link
Member

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
Copy link
Member

am11 commented Aug 9, 2021

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
Copy link
Member

jkotas commented Aug 9, 2021

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
Copy link
Member Author

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
Copy link
Member

jkotas commented Aug 9, 2021

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
Copy link
Member

am11 commented Aug 9, 2021

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
Copy link
Member

@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.

@SamMonoRT SamMonoRT modified the milestones: 7.0.0, 8.0.0 Aug 10, 2022
@trylek
Copy link
Member

trylek commented Aug 10, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests