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

Permissive handling for runtimeconfig.json #92037

Merged
merged 12 commits into from
Oct 8, 2023

Conversation

szilvaa-adsk
Copy link
Contributor

@szilvaa-adsk szilvaa-adsk commented Sep 14, 2023

This is a follow up to #91238. With these changes users can omit .runtimeconfig.json for C++/CLI dlls (i.e. dlls on Windows that link to ijwhost.dll) that are loaded after the runtime is already initialized. Note that missing .runtimeconfig.json will continue to cause a failure in all other situations.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 14, 2023
@ghost
Copy link

ghost commented Sep 14, 2023

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

Issue Details

This is a follow up to #91238. With these changes users can omit .runtimeconfig.json for dlls that are loaded after the runtime is already initialized. Note that missing .runtimeconfig.json will continue to cause a failure in all other situations.

Author: szilvaa-adsk
Assignees: -
Labels:

area-Host, community-contribution

Milestone: -

@vitek-karas
Copy link
Member

Sorry for the possible misunderstanding. I perceived the discussion in #91238 as concluding that we would only change ijwhost. The fxr_resolver.cpp is shared among all the hosts, so your change modifies the behavior for all of them. So I think the change is good but I think it should add a new parameter to load_fxr_and_get_delegate whether the "non-existent runtime config" is allowed or not. And ijwhost would pass in true, everybody else would pass in false.

@szilvaa-adsk
Copy link
Contributor Author

Ah! I will update the PR accordingly.

Opt ComHost out and IjwHost in.
@szilvaa-adsk szilvaa-adsk marked this pull request as ready for review September 20, 2023 21:23
@szilvaa-adsk
Copy link
Contributor Author

@dotnet-policy-service agree company="Autodesk"

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.

The product changes look good.

We need to add a test for the new behavior as well - and ideally also test that comhost correctly fails without a runtime config (not sure if we have such a test).

The ijwhost tests are here: https://github.com/dotnet/runtime/blob/main/src/installer/tests/HostActivation.Tests/NativeHosting/Ijwhost.cs

What I would do is modify this

else if (pal::strcmp(command, _X("ijwhost")) == 0)
to accept additional parameter, which tells it if it should load a runtime - something similar to what other tests do (load from runtimeconfig and get some runtime delegate), and THEN trigger the ijwhost part of the test.

And then just add a new test into the C# part which moves/renames the runtimeconfig (make sure to make a copy of the test asset so that it doesn't corrupt test state for the other tests).

src/native/corehost/comhost/comhost.cpp Show resolved Hide resolved
@elinor-fung
Copy link
Member

comhost correctly fails without a runtime config (not sure if we have such a test)

Comhost.ActivateClass_ValidateIErrorInfoResult indirectly does this. It uses the missing runtime config scenario as the failure that should be set in error info.

@szilvaa-adsk
Copy link
Contributor Author

@vitek-karas PR has been updated as you requested.

@szilvaa-adsk
Copy link
Contributor Author

@vitek-karas @elinor-fung Is there anything else you need to approve this?

@elinor-fung
Copy link
Member

One last suggestion for tests. Everything else looks good to me.

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

Thank you!

@szilvaa-adsk
Copy link
Contributor Author

@elinor-fung I can't merge this. Can you please merge?

@elinor-fung elinor-fung merged commit fa54ce8 into dotnet:main Oct 8, 2023
176 checks passed
@vitek-karas
Copy link
Member

@szilvaa-adsk : Thank you very much for the change!

@szilvaa-adsk szilvaa-adsk deleted the runtimeconfig branch October 9, 2023 15:43
@ghost ghost locked as resolved and limited conversation to collaborators Nov 8, 2023
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.

3 participants