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

Add hostfxr_get_dotnet_environment_info API #48097

Merged
merged 12 commits into from
Feb 13, 2021
Merged

Conversation

mateoatr
Copy link
Contributor

This PR adds a new hostfxr API to retrieve the installed SDKs and frameworks, hostfxr_get_dotnet_environment_info. See #46128 for the design of the API.

@ghost
Copy link

ghost commented Feb 10, 2021

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

Issue Details

This PR adds a new hostfxr API to retrieve the installed SDKs and frameworks, hostfxr_get_dotnet_environment_info. See #46128 for the design of the API.

Author: mateoatr
Assignees: -
Labels:

area-Host

Milestone: -

Mateo Torres Ruiz added 3 commits February 10, 2021 16:51
Validate size of structs
Test that result_context isn't modified
@rseanhall
Copy link
Contributor

What's the current status of the mitigation logic discussed in #46128 and dotnet/sdk#15021? Is that supposed to be part of this API?

@mateoatr
Copy link
Contributor Author

What's the current status of the mitigation logic discussed in #46128 and dotnet/sdk#15021? Is that supposed to be part of this API?

This PR does not address the mitigation logic mentioned in the linked spec, it just adds an API that mimics the behavior of dotnet --info.

Update tests
@vitek-karas
Copy link
Member

I created #48180 to track that change (since it should apply to all of the enumeration APIs as well as actual framework resolution).

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.

Except for the param validation and adding one more test case, this looks good.

src/installer/corehost/cli/fxr/hostfxr.cpp Show resolved Hide resolved
Comment on lines 231 to 233
int env_sdk_info_size = (sdks.Count > 0) ? Marshal.SizeOf(sdks[0]) : -1;
if (env_sdk_info_size != -1 && (env_sdk_info_size != sdks[0].size))
throw new Exception($"Size field value of hostfxr_dotnet_environment_sdk_info struct is {sdks[0].size} but {env_sdk_info_size} was expected.");
Copy link
Member

Choose a reason for hiding this comment

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

Nit: why not validate this when we add the SDK/framework to their respective lists? Each has this field, so it would make sense to validate all of them.

Copy link
Member

@sfoslund sfoslund left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @mateoatr!

@mateoatr mateoatr merged commit 64c7780 into dotnet:master Feb 13, 2021
@mateoatr mateoatr deleted the hostfxrapi branch March 9, 2021 19:31
@ghost ghost locked as resolved and limited conversation to collaborators Apr 8, 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.

4 participants