Convert C# Reference Assembly Generation to Use GenAPI #1057
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| # GenApi Usage in WPF on .NET Core | ||
| In WPF on .NET Core, C# reference assemblies are created via the use of [GenAPI](https://github.com/dotnet/arcade/tree/master/src/Microsoft.DotNet.GenAPI) and a separate reference assembly project located in the `ref` directory under a particular assemblies source directory. | ||
|
|
||
| WPF assemblies make extensive use of the [InternalsVisibleToAttribute](https://docs.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.internalsvisibletoattribute?view=netcore-3.0) which precludes the use of [ProduceReferenceAssembly](https://docs.microsoft.com/en-us/visualstudio/msbuild/common-msbuild-project-properties?view=vs-2019) or [ProduceOnlyReferenceAssembly](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-options/refonly-compiler-option). This is because these compiler options will include internal types and members in the reference assembly. In WPF, this creates dangling references to assemblies that do not exist in the `WindowsDesktop` reference pack. | ||
|
|
||
| Using GenAPI allows us to strip out internals, removing the dangling references from our reference assemblies. | ||
|
|
||
| ## Using GenAPI in WPF | ||
| GenAPI is run only on-demand. In the event that a change to a runtime assembly creates new public surface area, a developer will see an [ApiCompat](api-compat.md) error between the reference assembly and the runtime assembly. In order to address this, the developer must run GenAPI to generate new reference assembly code. | ||
| ### Running GenAPI | ||
| GenAPI can be run by setting the following MSBuild property while building. | ||
| ``` | ||
| /p:GenerateReferenceAssemblySource=true | ||
| ``` | ||
| 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 | ||
| Often, GenAPI will generate code output that will contain code that is either private, internal, or creates build errors. For this reason a developer usually cannot just use the output of GenAPI directly. Instead, the developer should do the following: | ||
| * Build with GenAPI enabled | ||
| * Diff the output file against the previous version | ||
| * 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. 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. 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. 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. |
||
| * Rebuild without GenAPI enabled and verify there are no ApiCompat errors | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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,
InternalsVisibleTocertainly 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.