Skip to content

Add design document for Mobile runtimeconfig.json host configuration #50744

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

Merged
merged 15 commits into from
Apr 8, 2021

Conversation

fanyang-mono
Copy link
Member

Contributes to #49237

@ghost
Copy link

ghost commented Apr 5, 2021

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

LGTM

@lambdageek
Copy link
Member

lambdageek commented Apr 6, 2021

The main feedback is that the design document should be more affirmative: "this is what the runtime does", not "this is what the runtime should do".

Also I think we don't need to talk about the implementation too much (ie, exactly when we parse the file, etc), but we should talk much more about the contract: the name of the task, the names of the parameters, etc.

It would be good to add an example of how to use the task. Something like this (but with correct details):

<!-- FIXME: I'm not sure what the task name is... use the correct one -->
<UsingTask TaskName="GenerateRuntimeConfigBlobTask"... /> <!-- here we should show how to find the task inside the runtime pack -->

<Target Name="GenerateRuntimeConfigBlob" AfterTargets="XYZ"> <!-- what's the right target to run after/depend on? -->
  <GenrateRuntimeConfigBlobTask InputFile="..." OutputFile="..." ReservedProperties="@(...)" />
</Target>

And we should describe the behavior of monovm_runtimeconfig_initialize and how embedders should interact with it. Maybe also with a concise example.

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

just a few nits

@fanyang-mono fanyang-mono merged commit 8b25350 into dotnet:main Apr 8, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 8, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@fanyang-mono fanyang-mono deleted the runtimeconfig_doc branch May 27, 2021 14:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants