-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Support project references to WinRT Components #19218
Conversation
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. |
cc: @marcpopMSFT @dsplaisted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable.
Were you able to test this manually?
Ideally this would include automated tests, however I'm assuming that there's not a good way to do so since this also depends on changes that will come in the CSWinRT targets. I would encourage coming back once both sets of changes are in and adding tests here to make sure that we don't regress the scenario.
@dsplaisted Yes I did test manually. I agree it would be good to have a test for this, and you are right that it needs to wait for cswinrt to update. I tried for a bit to get something together for testing and it seems like the best option is to make a folder with a solution for the scenarios we are trying to support -- in src/Assets/TestAssets/TestProjects. I'll make an issue on us to get that added when all the bits are together. |
Is the test for this tracked by this PR? #19635 Are you planning on updating the description to match other PRs that are labeled "servicing-approved" (see the template we use for tactics approval bugs), adding the label "servicing consider", and then either sending mail to tactics or joining the meeting? |
@marcpopMSFT Yes that PR is tracking the test, as well as this issue I have made in the cswinrt repo. And yes I have updated the description, I don't think I have ability to add labels, I tried. |
@j0shuams For the description, I was specifically referring to the approval format we used in issues like these: https://github.com/dotnet/sdk/pulls?q=is%3Apr+is%3Aclosed+label%3AServicing-approved. Description, Customer impact, Risk, and Testing are the key sections. Easier to read and present that way. |
@marcpopMSFT Okay thanks I have updated it again |
@j0shuams did you save the description update? I don't see the tactics format. I've added the label so this will show up in tactics discussion on Tuesday (or you can send mail before then for earlier approval if you didn't want to wait). |
@marcpopMSFT I misunderstood your original comment; I thought you have been talking about the other (draft) PR. My mistake. |
Let's get this into 6.0 first so we can see some real customer feedback on the changes before porting to 5.0. |
@j0shuams @angelazhangmsft per offline conversation, we're going to take this for 6.0 first. If you want it in for rc1 (release/6.0.1xx-rc1), you have until around the 25th and would need tactics approval (Though there is some flexibility with that). If rc2 (release/6.0.1xx), no approval required. |
@j0shuams I believe the fix for referencing multiple components was in CsWinRT right? Maybe this PR should be labeled "Project reference support for WinRT components" instead? |
RC1 version of this was approved. The guidance is to hold off on 5.0.4xx for now until there is more customer feedback/justification in a later servicing release. |
Feel free to close this (and reopen later). |
Description
Authoring C#/WinRT components has been working towards supporting native and managed consumers. This PR provides the needed changes to support consumers using project references. To do so, we must have some custom logic for Windows Runtime components.
Components typically output a .dll, but C#/WinRT components output a .winmd, which has a general implementation .dll "WinRT.Host.dll". This has complicated getting user scenarios working, as multiple assemblies need to be distributed for "one" assembly.
I am proposing we add a property in an existing target, which was made for WinRT support recently. As well as add a new target that uses the property set in the existing target to add a reference. We need this to support C# project reference consumers. Because C# consumers only get a .winmd via GetTargetPath, they do not have the assembly containing the managed implementation, so we annotate the .winmd to have the path to the implementation, and we add it as a reference here.
(FWIW: C++ consumers use the WinRT.Host.dll as the implementation assembly for the component.)
Customer Impact
Native app (C++) developers should be able to reference WinRT components made with C#/WinRT simply by adding a project reference. This PR removes the need for a project reference and a reference to the winmd made by the project -- the latter of which required an extra build step before the reference could be added.
Regression?
Risk
Verification