-
Notifications
You must be signed in to change notification settings - Fork 246
Porting to netstandard2.0/netcoreapp2.0 #294
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
Conversation
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? |
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 |
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:
|
@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:
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(). |
Thanks for your explanation, @FransBouma. I will definitely look into having a dictionary of string to |
I have checked in a proposed workaround to the missing 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) |
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). |
I'd be happy to - may I recommend MyGet to push your CI-level packages to :) |
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 |
@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? |
@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. |
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. |
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. |
87a40ec
to
69dc769
Compare
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 |
I'm getting an exception when doing a related reflection-based startup (I may need to fix that other library), but I get a
It's in a unit test project that targets
|
Thanks for testing, @jbogard. Is this maybe the cause of your I see we're building against an ancient NHibernate. That may be a reason... |
Yeah I think so, those ancient versions because of the shims bring in all of .NET Framework |
If we nuke NHibernate (as we have discussed elsewhere), would that simplify this task? |
So one thing I would make sure is for your dependencies, make sure they're .NET Standard also, not full .NET. |
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 |
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. |
I'll have a look at the code, @jbogard |
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
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:
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 |
@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 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:
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 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? |
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). |
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. |
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 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? |
I hope that I am not making an absolute hash of this, but I have:
Huge thanks to @erikbra for getting this together. |
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. 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? |
I would open a new P/R from the 1.0.0 branch. |
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/ |
I just pushed with 1.0.0-rc1, let's see how that works out |
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. |
@erikbra I'm looking for package on the nuget feed but I can't see it, am I missing something? |
@cfletcher sorry, we haven't pushed it to NuGet yet. We do have some issues still, but I think that's mostly with the 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 |
With that blocker now fixed, would you consider uploading an RC to nuget.org? Either way, thanks for your work on this. |
@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 |
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. |
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. |
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?q=roundhouse (all of them) |
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. |
Nice!
…On Tue, Dec 4, 2018 at 7:45 AM Erik A. Brandstadmoen < ***@***.***> wrote:
Don't know if you're still waiting around for this, @jbogard
<https://github.com/jbogard>, but the RCs of RoundhousE for .net core are
up on NuGet now.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#294 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGYMs8Dq5qgXVQgpB1CHwQPRL45BYgyks5u1nySgaJpZM4QKgd->
.
|
Can we find a workaround?
There's a missing feature in netstandard2.0, the
DbProviderFactories.GetFactory(string)
. This one is used inAdoNetDatabase.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 ?