Skip to content

Conversation

erikbra
Copy link
Member

@erikbra erikbra commented Oct 29, 2017

Can we find a workaround?

There's a missing feature in netstandard2.0, the DbProviderFactories.GetFactory(string). This one is used in AdoNetDatabase.cs. Can we work around this? There seems to be very much discussion and non-code-related discussions in the CoreFX repo regarding this, see e.g.

dotnet/corefx#19885

seems like fixing is pushed to netstandard2.1 (not yet announced).

Can we do something to work around this, so that we can get closer to solving #276 ?

@jbogard
Copy link
Contributor

jbogard commented Oct 30, 2017

You can ping @FransBouma, he's probably hit this I think.

Another option is to target netcoreapp2.0 perhaps? Is there a TFN where this is available?

@FransBouma
Copy link

FransBouma commented Oct 30, 2017

The api is currently under review. I already wrote the code for .NET core, but they first wanted to create and debate an API. The first version they came up with was completely different from what I implemented in my implementation. API review: https://github.com/dotnet/corefx/issues/20903. PR: dotnet/corefx#19885
The current API proposal is OK by me with some small adjustments, but it takes ages for them to decide these trivial things. No-one else has participated in the discussions as well, it seems there aren't a lot of people who still need this API. (which isn't a surprise, considering everyone who ported their ORM to .netstandard 2.0 has had to implement a DbProviderFactory registration system one way or the other by now)

@erikbra
Copy link
Member Author

erikbra commented Oct 30, 2017

Thank you, @FransBouma, for commenting. I have read your issues and pull requests in the CoreFX repo, and I felt sorry for you. It seems like they are stuck in analysis paralysis. I am very aware that changing an API used by thousands of users is different than one used by only a few. Of course the quality and API design is important, but it must be frustrating trying to contribute when you perform the work, and then it just gets put off, waiting for reviews.

A couple of questions:

  1. Is there anything we as developers-using-the-api-and-wanting it back can do to support you pushing the change through?
  2. Do you think it would be possible to release the PR as a Nuget Library with extension methods waiting for the merge, or is it too intertwined with Core code for this to be practical?

@FransBouma
Copy link

@erikbra :) Yes it was very frustrating, also because no-one else outside MS participated. At the end it felt like "why am I even investing my precious free time in this when they make it unpleasant?" so I gave up on that. Their loss. Anyway, IF they ever finalize the api review it's easy to change the implementation a bit to meet it so the most work is done already.

To answer your questions:

  1. The best you can do is perhaps review the API proposal as it is now (so the one at the bottom) and if you have anything to say, even if it's OK by you, then state that or upvote things so they know outsiders agree with that statement/proposal.
  2. I wouldn't do that, as you then have something which likely won't match what's going to be in the API in the future. If you need DbProviderFactory today, you should simply create a method and a static ConcurrentDictionary<string, DbProviderFactory> today so your users first register the factory there with the method and your code simply pulls the factory from that dictionary on .NETstandard. That will work today on .netstandard 2.0. See for an example what we added to our orm for that:
    https://www.llblgen.com/Documentation/5.3/LLBLGen%20Pro%20RTF/Using%20the%20generated%20code/gencode_runtimeconfiguration.htm#dbproviderfactory

That simply registers the type and implies the invariant name from the namespace (as that's what DbProviderFactories does today on .net full anyway) Internally I then obtain the factory from the concurrentdictionary and if it's not there on .net full try to read it from the DbProviderFactories class with GetFactory().

@erikbra
Copy link
Member Author

erikbra commented Oct 30, 2017

Thanks for your explanation, @FransBouma. I will definitely look into having a dictionary of string to DbProviderFactory implementations. Do you know it there is an authorative list on what string should map to what? Maybe the best place to look is my Windows registry? But where?

@erikbra
Copy link
Member Author

erikbra commented Oct 30, 2017

I have checked in a proposed workaround to the missing DbProviderFactories.GetProvider, as we always subclass the class using the factory, we might as well delegate the creation of the the factories to the subclasses. Any thoughts, @ferventcoder or @BiggerNoise?

I have a few concerns with changing these things, though, as I don't have any good test environments. Do you know if the integration tests are ship-shape, and run fine on the master branch if enviroments are set up? I would definitely like to integration test this before we merge (when the quality is good enough)

@jbogard, would you mind pre-alpha-testing if the netstandard support of roundhouse.lib would suit your needs? The built artifact is here: https://ci.appveyor.com/project/chucknorris/roundhouse/build/0.8.9-PullRequest.294+32.26/artifacts

(Also available on the following feed, but we're not advertising that a lot yet: https://ci.appveyor.com/nuget/roundhouse package roundhouse.lib, version roundhouse 0.8.9-PullRequest.294)

@erikbra erikbra changed the title Porting to netstandard2.0 (blocked by missing API) {WIP} Porting to netstandard2.0 (blocked by missing API) Oct 30, 2017
@FransBouma
Copy link

@erikbra

Do you know it there is an authorative list on what string should map to what? Maybe the best place to look is my Windows registry? But where?

That's not needed :) The user has to register the type in all cases, as there's no central registration system on netstandard/netcore, so the only source for this information, what type of DbProviderFactory to register and with which name, is up to the user. The user has to register a type as the assembly the type is in has to be present, even for SqlClient (which is a common problem, as users think they don't need to reference it like on .net full).

Your code uses invariant names to obtain the factory from DbProviderFactories on .net full. These invariant names, like System.Data.SqlCient or Npgsql, are in general the namespace of where the DbProviderFactory derived type is located. I don't know of any provider which doesn't use that system for the invariant name (DbProviderFactories uses that scheme as a fallback as well). But in any case: your code uses name X, the name the ADO.NET provider writer wants to use, like Npgsql or System.Data.SqlClient, to obtain the factory. The user, who wants to register the factory, then needs to use that same name X, so the factory is registered under the name your code will use. that's how it also works on .NET full. You can make the registration burden for users a bit easier to simply only require to specify a type (like we do) and derive X from the namespace of the type (as they all use that).

@jbogard
Copy link
Contributor

jbogard commented Oct 30, 2017

I'd be happy to - may I recommend MyGet to push your CI-level packages to :)

@erikbra
Copy link
Member Author

erikbra commented Oct 30, 2017

Lovely, @jbogard. We'll probably push CI-packages to MyGet eventually, but we're not doing too much now before @ferventcoder has set up the proper build system. Right now it's only a couple of powershell scripts, and we're planning on using Cake. I think it might be easier to push CI packages if we use a develop branch, people. Because then GitVersion produces more sensible -alpha (or -unstable in GitVersion 3.x) and -beta version numbers. Right now every package which is not a pull request looks like a release because of the "clean" version number it gets from being build from master.

@erikbra
Copy link
Member Author

erikbra commented Oct 30, 2017

@FransBouma Thanks for the explanation. So, this is like a ServiceLocator pattern, with names registrations, am I right? Is the common use case to take the provider name from a Connection String, and then return a factory? I see we can solve it differently in RoundhousE, as we have a subclass per provider. But, I was wondering how people use this now in .NET Desktop. Are there no lists in the registry, etc in full Framework either?

@FransBouma
Copy link

@erikbra in .net desktop, factories are registered in machine.config, or if needed in the app/web.config file. Most ado.net providers still come with an installer which registers a line in machine.config, which section is read by DbProviderFactories to build its datatable with name-factory instances.

On .net core there's no machine.config so your code can't locate the DbProviderFactory instance. It therefore needs to be registered somewhere, by the dev using your code. This is where your registration method comes in.

@erikbra
Copy link
Member Author

erikbra commented Oct 31, 2017

Yes, that's what I thought, @FransBouma. That there was some kind of central registry. But there is no such concept in .net core. And of course, then, they need to register it. I just wonder, is it a very common use case to have a string (that is effectively the full name of the provider), and want a factory? I'm just wondering, as I'm not familiar with the scenarios.

@erikbra
Copy link
Member Author

erikbra commented Oct 31, 2017

I'll write some tests on this PR, to see that we get the same Factory implementations for each type before and after the conversion.

@erikbra erikbra force-pushed the netcore branch 2 times, most recently from 87a40ec to 69dc769 Compare October 31, 2017 21:54
@erikbra
Copy link
Member Author

erikbra commented Oct 31, 2017

Added unit tests (before conversion to netstandard2.0), all tests green except Access - couldn't understand how to get the invariantProviderName to work). Converted to new strategy - with "hard-coding" the provider in each of the database-specific database implementations, and then all tests ran green.

@jbogard - could you please test your usecase with netstandard projects (you use the roundhouse.lib nupkg, don't you), d/l from here: https://ci.appveyor.com/project/chucknorris/roundhouse/build/0.8.9-PullRequest.294+34.28/artifacts

@ferventcoder and @BiggerNoise - do we want to merge this one? I think at least we should wait until we get the EnvironmentName vs EnvironmentNames sorted out (#296). Do we have some strategy for doing testing on all the providers?

@jbogard
Copy link
Contributor

jbogard commented Nov 1, 2017

I'm getting an exception when doing a related reflection-based startup (I may need to fix that other library), but I get a ReflectionTypeLoadException for these assemblies:

  • System.Configuration.ConfigurationManager, Version=4.0.0.0
  • System.ServiceModel, Version=3.0.0.0

It's in a unit test project that targets netcoreapp2.0 and the exception comes from me looping over the AppDomain's loaded assemblies. Command line is a little different (it runs a different runner for xunit):

      System.TypeInitializationException : The type initializer for 'ContosoUniversity.IntegrationTests.SliceFixture' threw an exception.
      ---- System.IO.FileNotFoundException : Could not load file or assembly 'System.Configuration.ConfigurationManager, Version=4.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. The system cannot find the file specified.
      Stack Trace:
        C:\dev\ContosoUniversityDotNetCore\ContosoUniversity.IntegrationTests\SliceFixture.cs(45,0): at ContosoUniversity.IntegrationTests.SliceFixture.ResetCheckpoint()
        C:\dev\ContosoUniversityDotNetCore\ContosoUniversity.IntegrationTests\IntegrationTestBase.cs(20,0): at ContosoUniversity.IntegrationTests.IntegrationTestBase.<InitializeAsync>d__2.MoveNext()
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        ----- Inner Stack Trace -----
           at System.Reflection.RuntimeAssembly.GetExportedTypes(RuntimeAssembly assembly, ObjectHandleOnStack retTypes)
           at System.Reflection.RuntimeAssembly.GetExportedTypes()
           at System.Linq.Enumerable.SelectManySingleSelectorIterator`2.MoveNext()
           at MediatR.ServiceCollectionExtensions.AddMediatRClasses(IServiceCollection services, IEnumerable`1 assembliesToScan)
        C:\dev\ContosoUniversityDotNetCore\ContosoUniversity\Startup.cs(51,0): at ContosoUniversity.Startup.ConfigureServices(IServiceCollection services)
        C:\dev\ContosoUniversityDotNetCore\ContosoUniversity.IntegrationTests\SliceFixture.cs(38,0): at ContosoUniversity.IntegrationTests.SliceFixture..cctor()

@erikbra
Copy link
Member Author

erikbra commented Nov 1, 2017

Thanks for testing, @jbogard. Is this maybe the cause of your System.Configuration.ConfigurationManager problem? Or maybe not. This is when targeting net461: https://stackoverflow.com/questions/46360716/cant-use-system-configuration-configuration-manager-in-a-net-standard2-0-libra

I see we're building against an ancient NHibernate. That may be a reason...

@jbogard
Copy link
Contributor

jbogard commented Nov 1, 2017

Yeah I think so, those ancient versions because of the shims bring in all of .NET Framework

@BiggerNoise
Copy link
Contributor

If we nuke NHibernate (as we have discussed elsewhere), would that simplify this task?

@jbogard
Copy link
Contributor

jbogard commented Nov 1, 2017

So one thing I would make sure is for your dependencies, make sure they're .NET Standard also, not full .NET.

@erikbra
Copy link
Member Author

erikbra commented Nov 1, 2017

I updated all the nuget refs to newest version (bumped NHibernate two major versions), let's hope it betters things. Do you have a public repo you're testing on, @jbogard? Or a minimal repro of the error? So that I could test too.

But this might not be us. I know you saw this one too... dotnet/standard#567

@jbogard
Copy link
Contributor

jbogard commented Nov 1, 2017

Yep, it's a branch for this PR jbogard/ContosoUniversityDotNetCore#3

I had to pull the package down because I don't see a NuGet feed for the AppVeyor artifacts.

@erikbra
Copy link
Member Author

erikbra commented Nov 1, 2017

I'll have a look at the code, @jbogard
btw, there's a nuget feed off AppVeyor here: https://ci.appveyor.com/nuget/roundhouse

@erikbra
Copy link
Member Author

erikbra commented Nov 5, 2017

I found the problem, and it's with NHibernate, which doesn't work with .NET Core yet. They are very close, and I think version 5.1 will work on core.

@jbogard Linq is great when everything goes as expected. When something unexpected fails, it's not that easy to find errors. I had to download and change AutoMapper.Extensions.Microsoft.DependencyInjection a bit to find the error. And I found that it was only NHibernate that was a problem.

I changed ServiceCollectionExtenstions.AddAutoMapperClasses (lines 70ff) to this to find the assembly that was the problem.

  List<TypeInfo> allTypes = new List<TypeInfo>();

            foreach (var a in assembliesToScan)
            {
                try
                {
                    if (a.GetName().Name != nameof(AutoMapper))
                    {
                        allTypes.AddRange(a.DefinedTypes);
                    }

                }
                catch (Exception e)
                {
                    Console.WriteLine("Problems with assembly " + a.FullName + ": "+ e);
                }
            }

I cheched out the NHibernate repo, and found out they weren't quite ready for .NET Core yet, but very close: nhibernate/nhibernate-core#633, I think they're good in the next release. I then changed to multi-target net461 and netstandard2.0 in roundhouse.lib, and depended on a CI build of NHibernate for netstandard2.0, and everything seems to work. I don't like depending on something that fragile, but it indicates that roundhouse can actually run on netstandard2.0.

I got some green tests, @jbogard, but some red, but I don't know how much setup you would expect to do to run all tests green. Would you mind testing with the latest CI build of this pull request? I still get some errors, but I hope it's not RoundhousE.lib's fault, but the setup lacking something:

A type could not be created from the object you passed. "roundhouse.databases.sqlserver.SqlServerDatabase, roundhouse.databases.sqlserver" resolves to null.

Could you please check out if the RoundhousE part of things work now, with this build? https://ci.appveyor.com/project/chucknorris/roundhouse/build/0.8.9-PullRequest.294+37.33/artifacts

@erikbra erikbra changed the title {WIP} Porting to netstandard2.0 (blocked by missing API) {WIP} Porting to netstandard2.0 (Waiting for NHibernate 5.1) Nov 5, 2017
@erikbra
Copy link
Member Author

erikbra commented Apr 29, 2018

@BiggerNoise I think this one's done now. But I am in lack of testing environments, and don't have the knowledge of MySql, PostgreSQL, Oracle, etc, to test.

I have now made two "command line" distributions of RoundhousE, one as was, and one for .net core. I have tried documenting the usage in the README.markdown file. I am very open to discussing what ways we should distribute RoundhousE for non-windows platforms

I have also tried to make the roundhouse.lib nuget package multi-targeted, i.e. for both netstandard2.0 and net461. I hope I didn't break anything.

I wasn't able to make the Oracle and Access providers to compile on netstandard, so those are excluded when building the netcoreapp2.0 one. But the other ones compiled, at least. And I have tested the SqlServer one rather properly on macOS, running against SQL Server in a docker container (imagine that).

There are two major changes:

  1. Conversion of most code to .netstandard2.0
  2. Swapped out Microsoft.EnterpriseLibrary.TransientFaultHandling with Polly, and some custom code. I hope someone would like to test this a bit. I have done my best, but you never know ;)

Testing help is much appreciated, as people have so many different usages and environments they use RoundhousE in.

@jbogard , I know you have been eager to try this, so if you could please give it a shot and see if it works as expected, I would really appreciate it.

@erikbra erikbra changed the title {WIP} Porting to netstandard2.0 (Waiting for NHibernate 5.1) Porting to netstandard2.0/netcoreapp2.0 Apr 29, 2018
@BiggerNoise
Copy link
Contributor

BiggerNoise commented Apr 30, 2018

@erikbra Thank you so much for this. I should be able to beat on this a bit later in the week. I can test SQL server and postgres reasonably well, but I doubt I can do much for MySQL beyond just verifying basic functionality.

I am thinking that this is probably a good point to put a stake in the ground and call this 1.0. Can we start cranking out 1.0 release candidates without merging to the master branch?

@erikbra
Copy link
Member Author

erikbra commented Apr 30, 2018

I guess the best way to go branch, wise, would to create a branch off master in chucknorris/roundhouse, e.g. "release-1.0" or something, and make this PR go in there instead. Whether I can change this PR from erikbra/roundhouse (branch netcor) to go against the new branch in chucknorris/roundhouse or I have to create a new one, I don't know. But we'll do what we can.

I guess we can release some RCs without merging to master. Maybe we need to adjust the appveyor.yml a bit, maybe not. Maybe we should put them on Myget? Or is Nuget fine for release RCs as well?

I guess we should make a .deb for distribution to *nixes eventually, but that should be a separate PR, I guess. If we make a release branch, I can see if I can look into it. Read a bit about .debs in the weekend, I have no prior experience, but it doesn't look impossible. I think we should set up a build step on a linux machine, though, so it should probably be a separate project in AppVeyor (you can have multiple .yml files).

@BiggerNoise
Copy link
Contributor

I can manage getting the new branch and getting this P/R merged into that one. My main concern is opening up the testing audience to people that don't feel like building RH from source.

I agree that proper distribution to Linux and MacOS should be separate P/Rs. I'd be willing to look at those once we have a solid 1.0 out the door.

@erikbra
Copy link
Member Author

erikbra commented Apr 30, 2018

Yes, of course you should be able to test without compiling. That's why I created a .tar.gz file in the artifacts. This can be tar -zxf'd to anywhere on a *nix box with dotnet core installed, and run. I wrote about it in README.markdown, but it doesn't seem like you can view the PR's on the main page. You can read it here, anyway (in my fork):

https://github.com/erikbra/roundhouse/tree/netcore#dotnet-core-linux-macos-etc

Is that a "good enough" experience for beta testers, do you think? If we upload the .tar.gz to the "Releases" page here on GitHub?

@BiggerNoise BiggerNoise changed the base branch from master to release/1.0.0 May 5, 2018 15:30
@BiggerNoise BiggerNoise merged commit ee04115 into chucknorris:release/1.0.0 May 5, 2018
@BiggerNoise
Copy link
Contributor

I hope that I am not making an absolute hash of this, but I have:

  • Added a release/1.0.0 branch
  • Merged this P/R into that branch
  • Tagged with 1.0.0.RC1

Huge thanks to @erikbra for getting this together.

@erikbra
Copy link
Member Author

erikbra commented May 5, 2018

Great work, @BiggerNoise.

Always exciting to do something remotely unfamiliar with git. :) I always get burned now and then.

I'm not sure if our GitVersion configuration understood the release branch name, it still marks it at 0.9.2-pr-something. I think there's somewhere you can tell it the format of a release branch (i.e. release-1.0.0 vs. release/1.0.0, etc)

I don't have much experience working with PRs a lot on the "receiving side". But, say I was to make a bugfix or something on the release branch? How would I do that? Can I just push to this PR, or is that link "disconnected" now?

@BiggerNoise
Copy link
Contributor

I would open a new P/R from the 1.0.0 branch.

@erikbra
Copy link
Member Author

erikbra commented May 5, 2018

I was looking at the wrong build in AppVeyor, it does understand the branch names. It just didn't respect the rc tag, it looks like. It's naming it 'beta', but 1.0.0 is picked up:

https://ci.appveyor.com/project/chucknorris/roundhouse/build/1.0.0-beta.1+22.build.95

Maybe the tag should have been 1.0.0-rc.1, with a dash instead of a dot btw the version and the suffix? The docs are a little unclear, but it could look like that should work: https://gitversion.readthedocs.io/en/latest/examples/

@BiggerNoise
Copy link
Contributor

I just pushed with 1.0.0-rc1, let's see how that works out

@erikbra
Copy link
Member Author

erikbra commented May 6, 2018

The version number got picked up by GitVersion. The package versions are now OK. I made an attempt at updating the instructions on how to get the preview packages. I couldn't get the package to show up in the public browsing part on MyGet, it might be something in the security settings, but I couldn't find it. However, the Nuget package feed is publicly available.

@cfletcher
Copy link

@erikbra I'm looking for package on the nuget feed but I can't see it, am I missing something?
https://www.nuget.org/packages/roundhouse/
The most recent package is 0.9.1 from January

@erikbra
Copy link
Member Author

erikbra commented Jul 29, 2018

@cfletcher sorry, we haven't pushed it to NuGet yet. We do have some issues still, but I think that's mostly with the rh.exe command-line utility. The RCs are available from
https://www.myget.org/F/roundhouse/api/v3/index.json

Unfortunately, neither of @BiggerNoise or I have had enough time to work on this the past months, so we haven't got the final 1.0.0 out yet. I think this one is a blocker: #327

@iainnicol
Copy link

With that blocker now fixed, would you consider uploading an RC to nuget.org? Either way, thanks for your work on this.

@erikbra
Copy link
Member Author

erikbra commented Nov 29, 2018

@iainnicol - definitely. We've had some people having to step down from maintaining Chocolatey now lately, and I'm trying to pick up the work as best as I can. I have an RC ready (made an rc2, including the fixes in the 0.9 (master) branch as well), it should be on MyGet as soon as Chocolatey is up and running (we use GitVersion from Chocolatey while building on AppVeyor), and on NuGet as soon as I hear from @ferventcoder.

It should then be available using dotnet tool install -g dotnet-roundhouse (you probably need to specify ther version explicitly as well, since it's pre-release). I'll write some proper release notes when I get it out on NuGet.

@erikbra
Copy link
Member Author

erikbra commented Nov 29, 2018

I haven't got stuff out on nuget yet, and I'm having a bit of trouble with the packages built on AppVeyor, so the MyGet auto-upload isn't any good for the dotnet tool. But the libs, etc, should be good.

Just add this to your NuGet feed if you are impatient, or wait a couple of days (hopefully), until we get it out on NuGet.

https://www.myget.org/F/roundhouse/api/v3/index.json

@iainnicol
Copy link

I'm not sure it's obvious, but the v1.0.0-rc packages are uploading to MyGet. See https://www.myget.org/feed/roundhouse/package/nuget/roundhouse/1.0.0-rc.2 for an overview. The download link on that page works.

Surprisingly, the v1.0.0-rc packages don't show appear in the index.json. I would guess MyGet privacy settings, or else a MyGet bug, not Appveyor's fault.

I saw in another thread you uploaded (manually?) to NuGet. Great! Thanks.

@erikbra
Copy link
Member Author

erikbra commented Dec 4, 2018

It's maybe not obvious. But they are on MyGet, yes. But the dotnet tool on MyGet is not working. Don't know why. But, now they're also all live on NuGet. E.g:
https://www.nuget.org/packages/dotnet-roundhouse/ (the dotnet global tool)

https://www.nuget.org/packages?q=roundhouse (all of them)

@erikbra
Copy link
Member Author

erikbra commented Dec 4, 2018

Don't know if you're still waiting around for this, @jbogard, but the RCs of RoundhousE for .net core are up on NuGet now.

@erikbra erikbra added release-1.0.0 Bugs or issues related to 1.0.0 release 3 - Done and removed 2 - Working labels Dec 4, 2018
@jbogard
Copy link
Contributor

jbogard commented Dec 4, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Done Enhancement release-1.0.0 Bugs or issues related to 1.0.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants