-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Convert C# Reference Assembly Generation to Use GenAPI #1057
Conversation
Disabling use of ProduceReferenceAssembly across the codebase.
…nces. Otherwise we can potentially import at incorrect times, overriding various values from the WpfArcadeSdk.
… the runtime assembly's project as a template. Based on PBT's GenerateTemporaryTargetAssembly.
Removing EmbeddedResources from reference assemblies.
…eference sources.
File dumping the initial generated code and projects.
Changes to ensure that reference assembly projects are properly bin placed into the ref pack.
Adding WindowsBase reference assembly.
…ally fixed output for WindowsBase reference assembly.
…ageso that it doesn't get added when the package isn't defined (building in the open).
.NET Framework reference assemblies contain resources (BAML/strings resx, etc). Should we also include these in .NET Core? |
Fixing Intellisense XML placement.
Re-baselining.
Removing ManualFixups.txt as they may cause confusion with needed XAML types.
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.
In your description you mention a "ManualFixups.txt" file in the PR, but i don't see that anywhere. was that removed or changed to a different file name?
src/Microsoft.DotNet.Wpf/ApiCompat/Baselines/PresentationFramework-ref-Net48.baseline.txt
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/ApiCompat/Baselines/PresentationCore-ref-Net48.baseline.txt
Show resolved
Hide resolved
When a build is run with that property enabled, GenAPI will read the runtime assembly and generate a new `{AssemblyName}.cs` file under the ref directory in the assembly's source tree. | ||
|
||
This new file will contain the newly created surface area and will need to be checked in along with the runtime assembly change. The next build without `GenerateReferenceAssemblySource` enabled will no longer display an ApiCompat error as the surface area will now match the baseline. | ||
### Issues with GenAPI |
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.
is all of this extra work needed because we use the InternalsVisibleToAttribute
?
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.
It's a combination of a few things, InternalsVisibleTo
certainly complicates things between assemblies, but also XAML compilation will reflect and try to find internal API in order to take certain shortcuts and optimizations. (Look at IAddChildInternal for example).
There are also issues where we have public API that has internal abstract members, which confuses GenAPI. GenAPI itself just sometimes adds strange things, like private dummy int fields and properties. No idea why, there are just bugs. I intend to make small reproduction applications when I have time and file bugs against GenAPI, but that's future stuff.
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.
Are those bugs reported back to the Roslyn team so they can be fixed?
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.
GenAPI doesn't use Roslyn it uses Microsoft.CCI). Originally, we were using Roslyn to generate our reference assemblies, but the use of InternalsVisibleTo was creating dangling references. We filed dotnet/roslyn#36409 to address these issues. In the meantime, this was the fix.
I haven't created issues against GenAPI yet, I haven't had the time to create minimal reproductions of the various bugs.
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.
If i'm adding a new function to a class, what's the difference between adding the API to the reference assembly manually and using GenAPI? I might not fully understand the problem we are trying to solve here, but it seems like it would be much simpler to just manually modify the reference assembly. Is there some part of our workflow that makes things more complicated?
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.
I missed this. The C# reference assemblies didn't exist in a manual fashion before this (back in the old internal repo maybe), so we could not just add things. So GenAPI was used as an initial seed for the most part. For some changes, it's probably easy enough just to pop the change into the reference assembly. Other changes it might be more difficult and require basically mirrored code changes. Also if you make a small change sans GenAPI then someone makes a very large change and uses it, it'll likely generate a noisier diff since your code location might not be the same.
It's probably the simplest and most thorough to GenAPI and use that output to manually add to the reference assembly. It's pretty easy (mostly) to diff the generated output against the current surface and just pick and choose your new stuff.
Long term we'd rather just have Roslyn allow us to say, "All publics, but also these specific internals" in a nice configurable fashion and call it a day. But we need something in the short term.
src/Microsoft.DotNet.Wpf/src/Shared/ref/AttributesForReferenceAssemblies.cs
Show resolved
Hide resolved
I removed it from the docs and the repo. Basically, it would confuse more than help in light of some internals needing to be kept for XAML compilation. I decided it's just best to tell developers to selectively add your change (as generated by GenAPI) and ignore the other fluff. We can work on improving the output from GenAPI in terms of the cleanup, but it's not there just yet. |
No, let's not include these. |
* Extract just the new surface area (and related code) from the generated code | ||
* Revert the generated file | ||
* Add back the new surface area to the reference assembly code | ||
* Ensure that nothing in the new surface area is private or internal unless requried by XAML compilation or other reference assemblies |
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.
It doesn't sound like a good idea to have internal/private APIs required by the XAML compiler. I understand that IAddChildInternal exists today, but is this something we want to put in as being an ok thing to do in the future?
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 is more if you make a change to an existing class that uses something like that, the entire class will get re-generated, sans the IAddChildInternal. So you need to ensure that you're not messing with that piece.
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.
We can have a discussion about fixing the docs up later, at this point I don't want to spin another build just due to doc updates. Helix queues are killing me today.
Fixes #932
What work is currently in this PR:
GitHub
dotnet-wpf-int
Testing Work:
What you should center your review around
These are what eventually feed into the reference assemblies and dictate the public surface area.
Links
dotnet-wpf-int PR: https://dev.azure.com/dnceng/internal/_git/dotnet-wpf-int/pullrequest/1772?_a=overview
Upstream notification issue: https://github.com/dotnet/core-setup/issues/6901