Skip to content

Conversation

@am11
Copy link
Member

@am11 am11 commented Dec 8, 2020

Commits:

  • Move corehost under src/native
  • Move content out of cli/ directory
  • Update configurations post-move
  • Update docs

@ghost
Copy link

ghost commented Dec 8, 2020

Tagging subscribers to this area: @vitek-karas, @agocke
See info in area-owners.md if you want to be subscribed.

Issue Details

Commits:

  • Move corehost under src/native
  • Move content out of cli/ directory
  • Update configurations post-move
  • Cleanup trailing whitespaces from changeset
    • Leaving out RapidJson dir
  • Update docs
Author: am11
Assignees: -
Labels:

area-Host

Milestone: -

Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just nits - I would split the PR into at least two - the move itself and a followup "cleanup".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @ViktorHofer
This is the only corehost related file I had to keep under src/installer directory, as it is sharing some targets (related to Version/GitSHA1 etc.) with rest of the installer infra. Looks like something that can be shared via src/tasks/installer in the future?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, src/tasks/* only defines tasks. The repo root's Directory.Build.targets would be used to share targets.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have eng/versioning.targets for very similar stuff. (and yup, targets can be converted to tasks. 😉)

Copy link
Member

@ViktorHofer ViktorHofer Dec 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but as a principle we should only create tasks when pure msbuild isn't sufficient.

@am11 am11 marked this pull request as ready for review December 8, 2020 18:50
@jkotas
Copy link
Member

jkotas commented Dec 8, 2020

https://github.com/dotnet/runtime/pull/45765/files#diff-7e828f345ddcecadc219a5b27948d0f94763928a9ae33de9a6a6b60f3aca685aL120

It is not quite right that this is linking coreclr libs. It would be nice to keep the dependencies linear (e.g. src/native->src/coreclr).

@VSadov
Copy link
Member

VSadov commented Dec 8, 2020

As I understand this change only moves the files. It does not change how things are built, or tested.

I am not sure we want to do file move as a separate step. We evidently can and it looks like a step to the right direction.
However I wonder if the change achieves enough on its own.

@VSadov
Copy link
Member

VSadov commented Dec 8, 2020

Changing static host to build in clr partition may be a bit more involved.
Right now the native libs that did not move to the new plan (only Globalization and Compression did) still rely on DLL entry points being re-exported from the final executable.

Because of that the static host picks the corresponding lib files from the libs artefacts.
We do build all necessary libs in clr partition now, but those are expected to be used through "overrider" mechanism and do not have Dll entry points exposed.
It all can be worked around, but it will be easier to wait until all native libs work the same. Then we can start building statichost in clr partition and get rid of coreclr_static.lib
It would be "easier" move then. Not simple :-) In my experience such changes often bring up unexpected troubles.

There is also an issue of testing. If we build static host in clr partition, we probably should test it there as well. This was not yet discussed. Maybe it is a simple matter. I am not sure.

@am11
Copy link
Member Author

am11 commented Dec 9, 2020

Rebased, conflicts resolved. Installer legs on all supported platforms are green.

renames are tricky to review, so I would prefer to keep the PR as simple as possible.

@vitek-karas, made it as simple as possible. Touching only 36 lines fixing paths.
I mainly chose today as other similar "just move" PRs are merged today. Agreed that further changes would be in-place ,and can be done separately from this, much disruptive, move.

@ViktorHofer
Copy link
Member

@am11 would you mind once again resolving the conflicts? Thanks a lot for your work.

@vitek-karas @VSadov after the conflicts are resolved could you please give this another look? Thanks

It is not quite right that this is linking coreclr libs. It would be nice to keep the dependencies linear (e.g. src/native->src/coreclr).

@jkotas is this something that should be addressed pre-merge? Maybe I'm misunderstanding but it sounds to me like the cross dependency was already there pre-move.

@jkotas
Copy link
Member

jkotas commented Feb 9, 2021

@jkotas is this something that should be addressed pre-merge? Maybe I'm misunderstanding but it sounds to me like the cross dependency was already there pre-move.

@jkotas is this something that should be addressed pre-merge?

I am ok with addressing it separately.

@VSadov
Copy link
Member

VSadov commented Feb 9, 2021

I am working on building host (or at least singlefilehost) in clr partition. It should deal with the dependency that @jkotas mentioned and make coreclr_static.lib unnecessary.

I think moving the files and changing the build are fairly independent. I would be ok with merging this PR.

@am11 am11 closed this Feb 9, 2021
@am11
Copy link
Member Author

am11 commented Feb 9, 2021

Ops, I pushed after resetting to master and before cherry-picking. 🤦
Can't reopen this PR now., changes are in #48071.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants