Updating Microsoft.NETCore.Platforms/runtime.json with corert RIDs #14142
Conversation
|
Looks fine to me, but I do not know enough about the RID business to signoff on the change. |
|
Are there any concrete examples of what rid specific assets for corert packages? |
| @@ -446,6 +446,264 @@ | |||
| "alpine.3.4.3-x64": { | |||
| "#import": [ "alpine.3.4.3", "alpine.3-x64" ] | |||
| }, | |||
|
|
|||
| //************************************************ | |||
There was a problem hiding this comment.
I'm not sure what if anything will break but in general comments aren't support in json files so you should remove this.
There was a problem hiding this comment.
GitHub certainly doesn't like it 😄
| //******************CORERT************************ | ||
| //************************************************ | ||
|
|
||
| "corert": { |
There was a problem hiding this comment.
Is it safe to assume you copied the aot graph?
There was a problem hiding this comment.
win*-corert looks like aot. There's but one difference. There is no win-corert, it starts with win7-corert. other than that it's a copy of the aot graph.
There was a problem hiding this comment.
Looks reasonable to me after you remove the comment section. Also I would like @ericstj to take a look before merging.
@davidfowl, anything that's being built against System.Private.Corelib is a candidate. Does that answer your question? |
|
@SedarG no, I meant concrete, as in, do we have a specific example today that uses these RIDs. System.Runtime.* and friends I assume will have these right? |
|
@davidfowl, runtime.win-corert.netcoreapp1.2.Microsoft.TargetingPack.Private.CoreRT has win-CoreRT assets. And I'm working on building the CoreFX libraries against this targeting pack, which is why I needed this change in. |
|
LGTM modulo prior /****/ comment feedback |
- Previously I had excluded some RIDs from the corert graph because they weren't 'supported' platforms for corert. But I think runtime.json is not the right place to enforce what's supported and what's not. Completing the graph helps maintaining runtime.json because it now follows a 'pattern'. - I updated the ordering of the nodes on win*-corert and that's where it differs from win*-aot. Unlike AOT counterparts, I'm always prioritizing CoreRT RIDS over non-CoreRT RIDs. - Also realized that the architecture specific chains were broken on non-windows CoreRT RIDS. I fixed that in this change too.
There was a problem hiding this comment.
The new RIDs and the pattern look correct, but I think you should look into generating this thing so that its maintainable. Previously the file was more-or-less equivalent with a data model of the same. Now that we're applying a generic cross-product of "corert" to a set of RIDs it seems a lot more prone to error.
| @@ -446,6 +446,378 @@ | |||
| "alpine.3.4.3-x64": { | |||
| "#import": [ "alpine.3.4.3", "alpine.3-x64" ] | |||
| }, | |||
|
|
|||
There was a problem hiding this comment.
We should think about generating this file during our build rather than all of this duplication. This seems way too prone to type-o's/inconsistencies.
There was a problem hiding this comment.
I agree. I'll file an issue to track auto generating runtime.json.
…otnet/corefx#14142) * Updating Microsoft.NETCore.Platforms/runtime.json with CoreRT RIDs * Further changes to runtime.json: - Previously I had excluded some RIDs from the corert graph because they weren't 'supported' platforms for corert. But I think runtime.json is not the right place to enforce what's supported and what's not. Completing the graph helps maintaining runtime.json because it now follows a 'pattern'. - I updated the ordering of the nodes on win*-corert and that's where it differs from win*-aot. Unlike AOT counterparts, I'm always prioritizing CoreRT RIDS over non-CoreRT RIDs. - Also realized that the architecture specific chains were broken on non-windows CoreRT RIDS. I fixed that in this change too. Commit migrated from dotnet/corefx@761fcf7
Adding CoreRT RIDs
@jkotas , @joshfree @weshaggard @ericstj , PTAL.
/cc @dotnet/corert-contrib