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

Don't call Add-EFProvider or Add-EFDefaultConnectionFactory #953

Merged
merged 1 commit into from
Jun 30, 2019

Conversation

bricelam
Copy link
Contributor

Part of #231

@bricelam
Copy link
Contributor Author

Should we remove Add-EFProvider? (We still need Add-EFDefaultConnectionFactory to dynamically choose between SQL Express and LocalDB)

The only providers that I can see using it are Firebird, Npgsql, and SQLite. They could update to this pattern.

cc @cincuranet @roji @mistachkin

@divega
Copy link
Contributor

divega commented Jun 27, 2019

What is the experience if you install one of these existing providers on top of the EF 6.3 package and those scripts are missing?

Is our own old SQLCE provider using it?

@divega
Copy link
Contributor

divega commented Jun 27, 2019

Is our own old SQLCE provider using it?

Ah, I asked this before I looked at the changes 😄

We still need Add-EFDefaultConnectionFactory to dynamically choose between SQL Express and LocalDB

Is there a way to use these transforms so that on package upgrade we don't touch the defaultConnectionFactory if there is one there? Or does the uninstall transform always need to run followed by the install transform on package upgrade?

Asking because I think ideally we would stop using Add-EFDefaultConnectionFactory.

@bricelam
Copy link
Contributor Author

I realized we can internalize the logic for Add-EFDefaultConnectionFactory and remove it too.

@bricelam
Copy link
Contributor Author

bricelam commented Jun 28, 2019

What is the experience of these scripts are missing

All of this only works with packages.config so PackageReference is unaffected.

We could make them warn and no-op, but I’m not sure you can even get into that situation. There would have to be an update to the provider package that still used the cmdlets.

@bricelam
Copy link
Contributor Author

Or you’d have to install the EF 6.3 package then an older provider. Don’t we tell customers to just install the provider package?

@divega
Copy link
Contributor

divega commented Jun 28, 2019

All of this only works with packages.config so PackageReference is unaffected.

Yeah, I was reading about that just now. Thanks for explaining.

We could make them warn and no-op, but I’m not sure you can even get into that situation. There would have to be an update to the provider package that still used the cmdlets.

Ok, I might be missing it completely, but what I had in mind, is to install EF 6.3 and the install a provider that hasn't been updated and still relies on the cmdlets (but that it is otherwise compatible with EF 6.3 because we are not making breaking changes).

I realized we can internalize the logic for Add-EFDefaultConnectionFactory and remove it too.

Crazy idea (sorry for the randomization if it is too crazy): what if EF 6.3 didn't do any transform on install (even for package.config) but also didn't do any transform on uninstall?

I am speculating that this would result in:

  1. Upgrades from previous package versions preserve whatever configuration was in place and hence continue to work the same way.
  2. Installation of the 6.3 package in clean projects would just defer decisions to runtime.

Are we at a point in which the runtime (and providers) can choose reasonable defaults when there is zero configuration?

A couple of pieces I imagine could be missing are:

  1. Discover the provider assembly
  2. Choose a default connection factory different from LocaldDB if that one is also available. I assume that could be a problem always if installing providers always results in the SQL Server provider also being installed. For the cases in which there are legitimately multiple providers, I think requiring some additional code or configuration to choose the default is reasonable.

@divega
Copy link
Contributor

divega commented Jun 28, 2019

Or you’d have to install the EF 6.3 package then an older provider. Don’t we tell customers to just install the provider package?

I guess yes, but it is reasonable to do the above if you want some new feature from EF 6.3 but the provider hasn't been updated.

Copy link
Contributor

@divega divega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but:

  • I would love a conversation about alternatives, e.g. like the ideas I wrote below to stop doing this altogether and have the runtime work without any configuration (for any provider)
  • I actually missed why we are switching to the transformation files given that it only works for package.config anyway. I am sure there is a reason, just don't know what it is.

@bricelam
Copy link
Contributor Author

bricelam commented Jun 28, 2019

(Updated to revert uninstall behavior changes)

@bricelam
Copy link
Contributor Author

Why are we switching to the transformation files?

This PR was mostly to prove that providers can preserve existing behavior without Add-EFProvider and Add-EFDefaultConnectionFactory. I'd prefer to remove them rather than port them to the new architecture.

@ajcvickers
Copy link
Member

I know we tried transforms for this a long time ago and the experience wasn't great, but I don't remember the details. However, I think it's probably good enough anyway.

I'm going to approve this, but we can still discuss other options.

@bricelam
Copy link
Contributor Author

Discussed with @divega, and we're going to keep Add-EFProvider & Add-EFDefaultConnectionFactory

@bricelam
Copy link
Contributor Author

bricelam commented Jun 28, 2019

@ajcvickers This is using the "new" (since obsoleted by PackageReference) XDT transforms which are more powerful than app.config.transform which was the only thing available when we first tried.

@bricelam bricelam changed the base branch from bricelam/delete1 to master June 29, 2019 20:54
@bricelam
Copy link
Contributor Author

TL;DR for @cincuranet, @roji & @mistachkin:

The pattern of calling Add-EFProvider inside init.ps1 will continue to work. Optionally, you can update to the pattern shown in this PR using app.config.install.xdt instead.

@bricelam bricelam merged commit 158d0cc into dotnet:master Jun 30, 2019
@bricelam bricelam deleted the delete2 branch June 30, 2019 01:46
@cincuranet
Copy link

@bricelam Thanks.

I'm planning to update to XDT (ticket).

roji added a commit to roji/EntityFramework6.Npgsql that referenced this pull request Jul 1, 2019
roji added a commit to roji/EntityFramework6.Npgsql that referenced this pull request Jul 1, 2019
@roji
Copy link
Member

roji commented Jul 1, 2019

@bricelam any tips on how to get those four files into the nupkg correctly from a project that only has a csproj (am tired from fighting with nuget)?

  <ItemGroup>
    <Content Include="content/**" PackagePath="contentFiles/any">
      <IncludeInPackage>true</IncludeInPackage>
    </Content>
  </ItemGroup>

This gets the files in the nupkg, but in the nuspec they're listed under <contentFiles> which seems to make nuget simply copy them (rather than apply transformations).

Thanks...

@bricelam
Copy link
Contributor Author

bricelam commented Jul 1, 2019

@roji They need to go under content. It only works with packages.config (not PackageReference)

@cincuranet
Copy link

cincuranet commented Jul 30, 2019

@roji Any reason you included also the Xxx.config.transform files in i.e. roji/EntityFramework6.Npgsql@717e7fa?

@roji
Copy link
Member

roji commented Jul 30, 2019

@cincuranet I honestly don't remember any more... I think maybe I just followed the example from EF6?

cincuranet added a commit to cincuranet/FirebirdSql.Data.FirebirdClient that referenced this pull request Jul 30, 2019
@cincuranet
Copy link

@roji I now tested only with XDT and everything works correctly (for packages.config). Probably good for me. 🤷

@bricelam Any ideas for PackageReference (for the future maybe)?

cincuranet added a commit to cincuranet/FirebirdSql.Data.FirebirdClient that referenced this pull request Jul 30, 2019
cincuranet added a commit to cincuranet/FirebirdSql.Data.FirebirdClient that referenced this pull request Jul 30, 2019
@bricelam
Copy link
Contributor Author

bricelam commented Jul 30, 2019

@cincuranet Issue #920 will help with the ADO.NET part of things.

Does anyone actually use the default connection factory? I imagine most people use a provider-specific connection object or a connection string from config that also specifies the provider.

@cincuranet
Copy link

Never seen the default connection factory to be used in real code.

cincuranet added a commit to cincuranet/FirebirdSql.Data.FirebirdClient that referenced this pull request Jul 31, 2019
cincuranet added a commit to cincuranet/FirebirdSql.Data.FirebirdClient that referenced this pull request Jul 31, 2019
cincuranet added a commit to cincuranet/FirebirdSql.Data.FirebirdClient that referenced this pull request Jul 31, 2019
cincuranet added a commit to cincuranet/FirebirdSql.Data.FirebirdClient that referenced this pull request Jul 31, 2019
cincuranet added a commit to cincuranet/FirebirdSql.Data.FirebirdClient that referenced this pull request Jul 31, 2019
cincuranet added a commit to cincuranet/FirebirdSql.Data.FirebirdClient that referenced this pull request Aug 1, 2019
roji added a commit to npgsql/EntityFramework6.Npgsql that referenced this pull request Oct 10, 2019
* Take dependency on EF6 6.3.0 GA
* Take dependency on Npgsql 4.0.10. Also tested successfully with 4.1.1,
  but not taking dependency in order to preserve compatibility with
  .NET Framework 4.5.
* Package snupkg
* Nuget icon
* Use transforms instead of Add-EFProvider (in install.ps1).
  See dotnet/ef6#953
* Various build cleanups, project renames, etc.

Closes #137
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.

5 participants