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

Convert C# Reference Assembly Generation to Use GenAPI #932

Closed
3 of 6 tasks
rladuca opened this issue Jun 12, 2019 · 8 comments · Fixed by #1057
Closed
3 of 6 tasks

Convert C# Reference Assembly Generation to Use GenAPI #932

rladuca opened this issue Jun 12, 2019 · 8 comments · Fixed by #1057
Assignees
Labels
area-infrastructure Enhancement Requested Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@rladuca
Copy link
Member

rladuca commented Jun 12, 2019

Currently, C# assemblies are building reference assemblies via ProduceReferenceAssembly (corresponding to the CSC flag -refout).

This causes several issues:

  • Reference assemblies contain internal types due to WPF's use of InternalsVisibleTo in many of its assemblies.
  • Due to this, several reference assemblies may have dangling type references as we move toward the same reference assembly surface as .NET Framework. This was also the case in .NET Framework for some reference assemblies, but this wasn't as strictly enforced.

Currently, the known dangling references are in the PresentationCore and PresentationFramework reference assemblies, the later only when PresentationUI's reference assembly is removed.

To fix these, we want to switch to using the GenAPI tool so that we can tailor the content of the reference assemblies to only contain necessary API surface area.

Our builds directly use ProjectReference for needed assemblies that are in the same repository and will alter these for those that are not available in the same repository (via WpfProjectReference.targets). We likely will have to update this mechanic if internal building assemblies require internals from the reference assemblies.

  • Call GenAPI to generate PresentationCore's reference assembly
  • Trim output to only needed types
  • Do the same for the rest of the publicly building C# assemblies
  • Test package ingestion against dotnet-wpf-int to validate WpfProjectReference.targets
  • Fix issues as needed
  • Apply GenAPI changes to dotnet-wpf-int C# assemblies
@rladuca rladuca added this to the Preview milestone Jun 12, 2019
@rladuca rladuca self-assigned this Jun 12, 2019
@rladuca rladuca added area-infrastructure Enhancement Requested Product code improvement that does NOT require public API changes/additions labels Jun 12, 2019
@tannergooding
Copy link
Member

Reference assemblies contain internal types due to WPF's use of InternalsVisibleTo in many of its assemblies.

If this is the only thing blocking, would it not just be feasible to log an ask on Roslyn to allow this to be configured (that is, to not emit internals to the reference assembly if IVTs are enabled)?

As much as having those internals is non desirable, it is very easy (especially when using new language features) to deviate from what Roslyn considers to be part of the public surface area (and therefore to break downstream consumers who may be dependent on that).

CC. @jaredpar, @jcouv

@tannergooding
Copy link
Member

Also CC. @ericstj

@vatsan-madhavan
Copy link
Member

vatsan-madhavan commented Jun 12, 2019

I don't know if Roslyn can turn around a feature request for us in <2 weeks... we may want to use GenAPI for now and see if a future Roslyn feature can be adopted.

We need to use GenAPI anyway because Roslyn is not a magic-bullet for WPF (because of C++/CLI).

@jcouv
Copy link
Member

jcouv commented Jun 12, 2019

I don't know if Roslyn can turn around a feature request for us in <2 weeks...

Probably not at this moment.
@tannergooding, I tried to stop by your office, but missed you. Come find me when you have some time.

@rladuca
Copy link
Member Author

rladuca commented Jun 12, 2019

I don't know if Roslyn can turn around a feature request for us in <2 weeks... we may want to use GenAPI for now and see if a future Roslyn feature can be adopted.

We need to use GenAPI anyway because Roslyn is not a magic-bullet for WPF (because of C++/CLI).

I agree, we can pivot back to using Roslyn generated C# ref assemblies when the ability for configuration exists, but for now GenAPI seems like the fastest path toward Preview 7.

@tannergooding
Copy link
Member

don't know if Roslyn can turn around a feature request for us in <2 weeks... we may want to use GenAPI for now and see if a future Roslyn feature can be adopted.

Right, and I wasn't meaning to suggest that using GenAPI temporarily for C# code is a bad thing.

I just want to ensure that:

  • The Roslyn team is aware that the WPF team had an issue and you are needing to move off the RefOut feature
    • If the Roslyn team isn't made aware, there is little chance they can address the problem
  • It was raised that there may be some limitations or other pain points to utilizing GenAPI instead
    • As extern to Roslyn, it can naturally get out of sync and has to be revisited as impactful features are used in consuming libraries and it becomes aware that there is something that needs to be fixed

I think it would be generally beneficial to log an issue on https://github.com/dotnet/roslyn/issues tracking the issue here.

@rladuca
Copy link
Member Author

rladuca commented Jun 13, 2019

@tannergooding Thanks, I filed an issue for some way of excluding internal surface area from generated reference assemblies.

@rladuca
Copy link
Member Author

rladuca commented Jun 18, 2019

Currently building ref assemblies for all C# projects (that require it). Working branch is https://github.com/dotnet/wpf/tree/dev/roladuca/genapics

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-infrastructure Enhancement Requested Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants