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

Remove ref Projects #2414

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Conversation

benrr101
Copy link
Contributor

@benrr101 benrr101 commented Mar 18, 2024

Description: This is a relatively big change, so hold onto your hats. Maintaining separate ref implementations of SqlClient has probably been fairly tedious, complicated the build process, and these days just isn't necessary. The goal of having the ref projects was to generate a reference assembly that exposed the API without implementation, for compilation as well as intellisense-style code assistance. csproj projects have support for generating both reference assemblies and xml files for intellisense. Thus, it shouldn't be necessary to maintain a separate ref project when the implementation projects can generate the reference assemblies. This PR attempts to remove the ref projects while maintaining generating reference assemblies.

The good news:

  • Enabling reference assembly generation does in fact generate a reference assembly
  • Modifying the nuspec file to include the new reference assemblies and the original xml files works
  • Referencing this nuget package in the IDE works as expected
  • Comparing old and new xml highlighted a few mistakes in the xml file generation

The less good news:

  • Old and new xml files are not the same
    • I wrote a quick and dirty program to generate a version of the XML files for comparison. These two files are attached. Just load them up in a diff tool to see the difference.
    • The biggest difference is the xml file for the ref project now includes pretty much all xmldocs from the codebase. This means that even classes/methods that are unusable from outside the project (eg, internal methods) are documented in the xml file.
      • For intellisense purposes, this is fairly benign
      • I'm not sure how this factors into generating web documentation for the package. If it turns out we cannot have these in the xml files, then there are a couple options
        • Continue to maintain a separate ref project
        • Add a tag like <exclude /> to any xmldocs that should not be included in the output. This tag is non-standard, but will be included in the xml and can be post processed to remove the element from the xml file
    • A handful of differences were mistakes
      • ctor comments when the ctor never existed
      • Linking the xml doc for a different overload
  • NotSupported.targets and dependencies was too complicated to detangle, so I just removed it. According to @cheenamalhotra this was supposed to generate a NotSupported exception if any of the ref binaries were actually called. I'm not sure if this is necessary, please comment if it really is necessary.

Microsoft.Data.SqlClient-withoutref2.txt
Microsoft.Data.SqlClient-withref.txt

Testing:

  • Builds still run, tests still pass as expected
  • Nuget package still contains ref assemblies and xml docs
  • Including them in a project still provides intellisense

@benrr101
Copy link
Contributor Author

Not sure why most of the test runs passed, I'm running into unexpected build issues after rebasing. Currently working through those issues, specifically, I can't seem to get standard2.0 and standard2.1 to generate ref assemblies. Might not be an important issue since we're looking to drop standard2.0 and standard2.1 support. But I'll see what I can do.

@Wraith2
Copy link
Contributor

Wraith2 commented Mar 19, 2024

  • NotSupported.targets and dependencies was too complicated to detangle, so I just removed it. According to @cheenamalhotra this was supposed to generate a NotSupported exception if any of the ref binaries were actually called. I'm not sure if this is necessary, please comment if it really is necessary.

People using powershell or a few other situations where they don't use a compile step quite often end up trying to use the NotSupported version of the binaries because they haven't done a nuget restore. If you can be certain that the behaviour is maintained which prevents them using wrong-platform assembly versions in that situation then removing it is probably ok.

@benrr101
Copy link
Contributor Author

Hey @Wraith2, thanks for that feedback - I'd like to look into this a bit more to double check it's not an issue. I'm not 100% sure how to reproduce the scenario you mentioned. Is there an example you could provide me? Would this be like eg, building a project with the nuget, but replacing the implementation binary with the ref binary in the output folder?

@Wraith2
Copy link
Contributor

Wraith2 commented Mar 19, 2024

Just search this repo for issues with powershell in and you should find something

@benrr101
Copy link
Contributor Author

Backing this PR out to a draft since there's a couple more issues with it. Working on finding a good solution to them.

@benrr101 benrr101 marked this pull request as draft March 20, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants