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 OutputRID to RID graph during source build #50157

Closed
wants to merge 1 commit into from

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Mar 24, 2021

Also support the addition of RIDs through InferRuntimeIdentifiers.

Fixes #48507
cc @omajid

Needs dotnet/arcade#7141 (current staged in side-channel feed)

Also support the addition of RIDs through InferRuntimeIdentifiers.
@ericstj ericstj added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 24, 2021
@ericstj ericstj requested review from tmds and ViktorHofer March 24, 2021 05:35
@ericstj ericstj self-assigned this Mar 24, 2021
@dotnet-issue-labeler
Copy link

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.

@@ -5,14 +5,25 @@
<!-- We don't need to harvest the stable packages to build this -->
<HarvestStablePackage>false</HarvestStablePackage>
<PackageDescription>Provides runtime information required to resolve target framework, platform, and runtime specific implementations of .NETCore packages.</PackageDescription>
<InferRuntimeIdentifiers Condition="'$(DotNetBuildFromSource)' == 'true'">$(InferRuntimeIdentifiers);$(OutputRID)</InferRuntimeIdentifiers>
Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like we have both PackageRID and OutputRID since the runtime consolidation. I'm not sure why we have two: they both seem to try to be accomplishing the same thing. I use OutputRID here since that's what installer appears to use.

Copy link
Member

Choose a reason for hiding this comment

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

I believe @jkoritzinsky is who has more context about OutputRID vs PackageRID but it seems like installer uses PackageRID to restore tooling and stuff and OutputRID is the rid to produce the package for.

Copy link
Member Author

Choose a reason for hiding this comment

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

installer uses PackageRID to restore tooling and stuff

I don't think its tooling. I think installer uses this to for selecting packages from CoreCLR / Libraries (was more relevant when those were separate repos). I'm 90% sure that its wrong to have two different properties here.

Copy link
Member

Choose a reason for hiding this comment

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

Yep something like that was I couldn't remember exactly. I think we should just use PackageRID and remove OutputRID

@ghost
Copy link

ghost commented Mar 24, 2021

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

Also support the addition of RIDs through InferRuntimeIdentifiers.

Fixes #48507
cc @omajid

Needs dotnet/arcade#7141 (current staged in side-channel feed)

Author: ericstj
Assignees: ericstj
Labels:

* NO MERGE *, area-Infrastructure-libraries

Milestone: -

@omajid
Copy link
Member

omajid commented Mar 24, 2021

Would it make any sense to add (reuse?) a CI leg that sets up an environment with an unknown/fake RID and verify that the RID is present in the generated RID graph?

@ericstj
Copy link
Member Author

ericstj commented Mar 24, 2021

Would it make any sense to add (reuse?) a CI leg that sets up an environment with an unknown/fake RID and verify that the RID is present in the generated RID graph?

I cover the RID graph addition testing to produce the right runtime.json in the arcade side of this. https://github.com/dotnet/arcade/pull/7141/files#diff-dfd25c6d9740edaacaab6c8000e0bbc3d164ac6bc024c1d850c6e4d48509e583R44

We do have a source-build leg already. Perhaps we could add usage of InferRuntimeIdentifiers to that? 🤔

@ghost
Copy link

ghost commented Apr 24, 2021

Draft Pull Request was automatically closed for inactivity. It can be manually reopened in the next 30 days if the work resumes.

@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

source-build on unknown linux distros
4 participants