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

Clean up ref files to more closely match GenerateReferenceSource #26187

Closed
stephentoub opened this issue May 16, 2018 · 13 comments
Closed

Clean up ref files to more closely match GenerateReferenceSource #26187

stephentoub opened this issue May 16, 2018 · 13 comments
Labels
area-Infrastructure-libraries enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@stephentoub
Copy link
Member

Running /t:GenerateReferenceSource leads to a ton of differences between the generated files and what we currently have for the refs. Some of the differences are by design, e.g. ifdefs, but many are not:

  • Out-of-order members
  • Missing _dummy or _dummyPrimitive fields (which seems like it could be a bug)
  • Missing protected ctors on abstract classes
  • Whitespace differences, e.g. blank lines, spaces between generic type parameters, etc.
  • Missing namespace qualifiers
  • Numeric vs named Flags enum attribute arguments
  • etc.

It would be good to clean all of this up, in order to a) do an audit to make sure nothing important is wrong, and b) make it easier in the future to use GenerateReferenceSource for minor additions without then needing to deal with a ton of noise.

@AraHaan
Copy link
Member

AraHaan commented Oct 17, 2018

tbh if the netcore packager for netcoreapp#.# and netstandard#.# projects supported generating reference assemblies from the actual sources instead of making reference only projects to make them it could be a better yet approach to not only clean up some of the source tree, but also be consistent with what netframework also supports. I think that would be a better yet solution for things like this.

So like:

- **/ref/* (anything in a ref folder)
+ (have the projects set to produce a reference only assembly based on the code in it).

That way the ref folders in the projects can be deleted.

Or better yet, do not strip the documentations on the actual assemblies of those target platforms (in the case of the xml documentations not being included in the nupkg's).

@ViktorHofer
Copy link
Member

From the issue description it doesn't seems like we need infrastructure work, moving back to area-Meta.

@joperezr
Copy link
Member

@stephentoub Is this required for 3.0?

@stephentoub
Copy link
Member Author

Is this required for 3.0?

Nope.

@tannergooding
Copy link
Member

Here is the baseline patch that I generated for dotnet/corefx#36966: Baseline.patch.txt

I think we would want to review these at the very least, as they can be impactful to some scenarios (like interop or unsafe code):

  • Missing _dummy or _dummyPrimitive fields (which seems like it could be a bug)
  • Missing protected ctors on abstract classes

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@ericstj ericstj added area-Infrastructure-libraries and removed area-Meta untriaged New issue has not been triaged by the area owner labels Jun 26, 2020
@ghost
Copy link

ghost commented Jun 26, 2020

Tagging subscribers to this area: @safern, @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@ericstj
Copy link
Member

ericstj commented Jun 26, 2020

@Anipik /@safern didn't we do something like this already? I thought we got the diff pretty clean after supporting follow-type-forwards.

@Anipik
Copy link
Contributor

Anipik commented Jun 26, 2020

I thought we got the diff pretty clean after supporting follow-type-forwards.

We fixed the scenarios(there was a couple of them) where it was not working, but we never did a whole sweep up.

@safern
Copy link
Member

safern commented Jun 26, 2020

I think we should do another pass. I remember for stuff that was manual added into the ref src files we added a feature to specify a file with ignored APIs so that should help us get it better for complicated assemblies like System.Runtime.

@AraHaan
Copy link
Member

AraHaan commented Jun 26, 2020

Sorry if this is offtopic but how did you guys somehow pack the ref projects with the outputs from the runtime ones in the same packages?

@ericstj
Copy link
Member

ericstj commented Jun 26, 2020

We use a custom packaging system that generates then packs a nuspec.

You can still do it with CSProj. See https://docs.microsoft.com/en-us/nuget/reference/msbuild-targets#targetsfortfmspecificcontentinpackage

@ViktorHofer
Copy link
Member

Triage: We want to keep this in 5.0 as we want to fix the parameter name mismatches.

@ViktorHofer
Copy link
Member

ViktorHofer commented Mar 9, 2023

CC. @ericstj - Is there an easy way to run the tooling for "everything" to try and find out what is actually out of sync?

@tannergooding by building with the /p:GenerateReferenceAssemblySource=true property, you can re-generate all reference source files. BUT, doing that would cause duplicate file writes and most likely file locks as TFMs like net8.0-windows and net8.0 would write to the same location.

If you just care about net8.0, you could add the following to the root Directory.Build.targets file:

<PropertyGroup>
    <!-- Hook onto the TargetsTriggeredByCompilation target which only runs when the compiler is invoked. -->
    <TargetsTriggeredByCompilation Condition="'$(TargetFramework)' == 'net8.0'">
      $(TargetsTriggeredByCompilation);
      GenerateReferenceAssemblySource
    </TargetsTriggeredByCompilation>
</PropertyGroup>

That enables GenAPI to run for the net8.0 TFM in all our source projects. You then just need to build allconfigurations: build.cmd libs -allconfigurations.


All that said, to provide a comprehensive response I need to add the following (listed by priority):

  1. The existing GenAPI is CCI based and has a number of defects that we aren't planning to resolve. Reason for that is, that we have been working on a new Roslyn based GenAPI implementation for the last months. It just last week got enabled in SBRP and works nicely so far. There are a number of missing features that doesn't make it yet usable in dotnet/runtime: GenAPI is missing features that dotnet/runtime depends on sdk#31088.
  2. We have a big number of reference source projects that don't ship anymore, i.e. System.Data.Odbc. Since .NET 6, reference assemblies haven't been part of our packages anymore.
    The reference source projects which don't ship in a targeting pack (either Microsoft.NETCore.App.Ref, Microsoft.AspNetCore.App.Ref or Microsoft.WindowsDesktop.App.Ref) don't add much value anymore, given that we now have PackageValidation (from the new ApiCompat) enabled. We were considering removing these projects and I even had a PoC PR up last year which gave us a ton of feedback and revealed numerous defects in the product which we then fixed.
  3. One aspirational but still standing goal is to use Roslyn's RefOut feature to generate our shipping reference assemblies instead of compiling them ourselves: Let roslyn generate reference assemblies from source projects and remove reference projects #58163. Note that AspNetCore and WindowsDesktop already leverage Roslyn's feature to generate their reference assemblies which then end up in their targeting packs.

While we are quite short on resources at the moment, I would be happy to drive an independent effort (not tied to the current release) to make progress on these topics above. If you want to help with that @tannergooding (or anyone else), feel free to ping me offline.

cc @ericstj @andriipatsula @carlossanlop @eerhardt

@stephentoub stephentoub closed this as not planned Won't fix, can't repro, duplicate, stale Dec 6, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jan 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

10 participants