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

[fxr] Use std::find_if when searching for root framework. #72080

Closed
wants to merge 2 commits into from

Conversation

AraHaan
Copy link
Member

@AraHaan AraHaan commented Jul 13, 2022

Fixes #71027.

.NET 6:

Consider:

Tests:

Test was updated to ensure that the modified code works.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 13, 2022
@ghost
Copy link

ghost commented Jul 13, 2022

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

Issue Details

Fixes #71027.

.NET 6:

Consider:

Author: AraHaan
Assignees: -
Labels:

area-Host

Milestone: -

fx_definitions.end(),
[&](const std::unique_ptr<fx_definition_t>& fxdef)
{
return fxdef->get_name() == L"Microsoft.NETCore.App";
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, when I first looked inside of pal.h I thought it unconditionally defined it's string_t as a typedef of wstring on all platforms. I was wrong 😄.

@vitek-karas
Copy link
Member

I would prefer we solved this by correctly sorting the frameworks during resolution. Currently the code doesn't really assume anything about the root framework - other than it having hostpolicy.dll in it. In particular its name is not hardcoded anywhere. I would prefer we keep that. Also the order of the frameworks is used for other things, for example we assume the first framework is the "app".

Also regarding .NET 6 backport - we can try it, but personally I would be against doing it. The risk is non-trivial (hostfxr is used in every single app and is used across all .NET versions). While I understand that this is important for your scenario, there is a workaround and it's the first time we've actually heard about anybody using additional frameworks like this.

I would much prefer we fix this fully (sorting) - if it makes it into .NET 7, great.

@AraHaan
Copy link
Member Author

AraHaan commented Jul 14, 2022

I guess here we could check that the framework here:

std::rotate(existing_framework, existing_framework + 1, fx_definitions.end());
is the absolute lowest level and if not rotate the one above it back (check it for it's own framework dependencies).

@vitek-karas
Copy link
Member

Something like that - currently we don't have information about the "level" (that's kind of the point of the sorting to create that info). I think what this should do is add a list of dependencies on each fx_definition_t - technically we need just the names, but we might as well store the fx_reference_t which was read from the runtimeconfig (might come handy for diagnostic purposes?).
With that we then would have to implement recursive sort - but that should be enough info to do it right.

@AraHaan
Copy link
Member Author

AraHaan commented Jul 14, 2022

I was thinking of another way as well, have hostpolicy and fxr become static libraries (like hostcommon) that way the loading code is only needed to load coreclr then that way it would not have to look in the framework directories to load up anything much else and would make it so much easier to maintain the host. Then that way all the code would have to do is check "is coreclr in the current directory? Yes, then self-contained." otherwise then all it would have to do is check if frameworks are installed and if not then it will fail with the usual "not installed" like it currently does.

Although there might be a reason for hostpolicy existing in the shared framework (I do not see a functional reason for it to be there unless the libraries in it try to p/invoke from it which I do not think it does). With all the complexity I think it would be an option as well.

@vitek-karas
Copy link
Member

hostpolicy is technically not necessary (it could be merged with coreclr.dll - or even better reimplement in C# into CoreLib). But hostfxr is necessary - it's the one thing which knows how to resolve framework dependencies. Obviously if you have a self-contained app it doesn't very little (and in single-file self-contained case the functionality it greatly simplified), but for framework dependent apps it must be there. The apphost is meant to be super simple, so all it knows is how to find hostfxr (no version resolution, it always uses the latest version on the machine) - the relatively complex thing of parsing the json files an resolving framework versions is all in hostfxr.

@AraHaan
Copy link
Member Author

AraHaan commented Jul 17, 2022

I think reimplementing it into corelib or to coreclr would be the better idea.

@vitek-karas
Copy link
Member

Closing this for now as the changes are not ready yet. @AraHaan if you're still interested in fixing this, please open a new PR with changed as discussed above.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Host community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AppHost] Don’t assume that last framework in runtimeconfig list contains hostpolicy.dll
2 participants