-
Notifications
You must be signed in to change notification settings - Fork 114
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
V2 #255
Comments
Over in #253, @bricelam asked:
But I want to answer it here, so: "planned"? No. Not yet anyway. But I'm experimenting with some ideas that would be considered breaking changes. Well, they would be breaking changes at the SetProvider() layer anyway, even though I could probably hide them behind an unchanged Batteries.Init(). For example, currently, SetProvider takes an object that implements ISQLite3Provider, and here are quite a few different implementations, and somebody (usually the Batteres.Init() call) needs to:
In the current experimental v2 code, I (think I) only need one implementation of that class, and configuring which instance of the native code can be done lower. So the initialization could look something more like this:
And SQLitePCL.Setup could potentially have a variety of methods to handle different use cases. Another thing worth mentioning is that I believe all the nuget packages with names SQLitePCLRaw.provider.* would become unnecessary. Whether that is a breaking change depends on one's perspective I guess. Anyway... I won't actually finalize any decisions about breaking changes without seeking input from you and others. I will want to ask questions like, "Here is what I want to change, and why I think it's better, even though it will require a bit of work for others. Do you think this is worth the trouble?" |
Shorter and pithier: I don't have a "plan" for version 2.0 yet. I just have some ideas and activity, and I want to make those things visible and open for discussion. |
And to be more consistent with the comment I just posted, I edited the top comment to add a clarifying remark. Bottom line: Windows Phone 8.0 has gotta go, but I'm open-minded about everything else. :-) |
That all sounds good to me. If your 2.0 ships before our 3.0. These cleanup breaking changes are totally acceptable an in line with what we’re doing. |
Plz include this as well #244 |
@AwsomeCode Could you say more about how #244 is important to you? Strictly speaking, I will probably not consider that item part of the v2 effort, because a change of the sqlcipher native code would be orthogonal to the other changes likely to appear in v2. That said, I may get it done in the same time frame. But it's problematic. The sqlcipher 4 code base is not, by default, compatible with files written by sqlcipher 3. That's gonna cause problems. |
@ericsink Main reason is that iOS having issue with SQLCipher 3. |
@AwsomeCode Oh yeah, that issue. Thanks for reminding me. |
@ericsink Thanks for your hard work 👍 |
@AwsomeCode - I don't want this to sound like a plug, but SQLCipher 4 commercial edition for Xamarin.iOS would provide an immediately available fix for that issue. It is compatible with SQLitePCL.raw and (and dependent libraries) via bundle_zetetic, thanks to @ericsink. |
@sjlombardo I wish I had posted that reminder myself. |
Hey @bricelam -- shall I assume that support for winsqlite3.dll is still important for you folks? The new approaches I am considering for v2 would allow me to remove a ton of complexity, which seems very cool. But keeping support for cdecl vs stdcall would force me to put some of that complexity back, and I'm feeling crabby about that. More detail: AFAICT the only way to specify the calling convention for an unmanaged function pointer delegate is using an attribute, which, requires constant arguments, which means it is determined at compile time, which means supporting both calling conventions requires compiling two copies of the code. Note also that AFAICT stdcall is only an x86 thing. It has no effect on x64 and ARM. It is of course possible that dropping support for stdcall could affect other use cases as well, but winsqlite3 is the only one I know of. And then there's my (not always popular) belief that, regardless of the platform, virtually all attempts to use a sqlite library provided by the system (instead of bundling one with the app) eventually result in sadness and regret. Bottom line: keeping support for winsqlite3 seems to be extremely low bang-for-the-buck. So I would love to drop it if I can. |
Would there still be a hook (like ISQLite3Provider) for someone to implement it even if you didn’t enable it out of the box? |
(I'm interpreting your response as "Yes, we need this, but it's okay if people who use winsqlite3 have to do extra steps.") Providing the ISQLite3Provider flexibility still involves more complexity than I wanted. But I also am not expecting to get everything I want. Even though I tossed you this question, I've been proceeding on the assumption that it wasn't actually likely that I could go forward with support for only one calling convention. :-) Still, if you folks were to say, "Feel free to completely drop support for winsqlite3, and good riddance to it", then I would seriously consider Cdecl-only. But like I said, my default path here is to find the least-ugly way to support both. And in the end, the result is still going to be far less ugly than the pre-v2 stuff. |
Personally, I’d love to say drop it. I agree that it usually leads to sadness and regret. Having a consistent library like e_sqlite3 across platforms is far more beneficial. Let us follow up with some of our partners that we know are using it. |
Very good. But if you run into significant resistance, don't worry about it. I am proceeding on the assumption that stdcall/winsqlite3 support stays in, and the crabbiness is fading a bit. :-) The fact is that there is a difference between the following two issues:
I personally don't care much about (1) because of the sadness and regret issue. But (2) might simply be going too far. Removing that flexibility involves a risk that we will want it back. So I am trying to find the right middle ground. |
@bricelam Ha! So I've been lamenting the somewhat poor state of T4 on .NET Core and SDK-style csproj. And in my searching for answers I found this: https://www.bricelam.net/2015/03/12/t4-on-aspnet5.html Which seemed kind of ironic. |
Lol, I’m going to post another related one soon too. I love T4, but it’s definitely in a sad state. |
Regarding winsqlite3, we (the EF team) are OK if you drop support for it if there's still a way (e.g. ISQLite3Provider) for customers to unblock themselves. If we get a lot of feedback wanting it, we could consider providing our own implementation... |
FWIW., at this point I plan to keep winsqlite3 support in. |
Heads-up on a minor breaking change I want to make for V2: Short explanation: I want the default for the enabling of sqlite3_next_stmt() to be OFF. More details: Properly implementing sqlite3_next_stmt() is weird. To avoid having two managed objects that reference the same native object, raw contains code that takes the IntPtr from the underlying C call and finds the corresponding sqlite_stmt object in a dictionary it keeps for just that purpose. Maintenance of that dictionary has a performance cost for every prepared statement, whether sqlite3_next_stmt() is going to be used or not. This cost is sad, because sqlite3_next_stmt() is (AFAICT) not commonly used. I personally have never had a use for it except when I was trying to debug a problem with unfinalized statements. So my code has a switch to turn the feature off. If you call sqlite3_next_stmt() with the feature turned off, it throws an error which contains an explanation of how to turn it on. The proposed change is simply to make the default OFF instead of ON. The result should be a little performance increase for almost everybody. For anybody who needs sqlite3_next_stmt(), the workaround is easy: just add a call to enable_sqlite3_next_stmt(true). Objections welcome. If I am mistaken and sqlite3_next_stmt() is used more than I think, we can leave the default the way it is. |
…stmt to OFF. and update the test case accordingly. see #255
A prerelease build of SQLitePCLRaw 2.0 has been pushed up to nuget.org. Draft release notes are in v2.md at the top level of the tree in the v2 branch: |
I have started a branch for a version 2.0. This branch will cause some compatibility breaks. (Edit: To clarify, the only compatibility breaks I'm fairly certain about at this time are the deprecation of old platforms.)
The v2 branch:
These are changes I've been wanting to make for a long time. Getting rid of the C++/CLI code simplifies things. Multi-targeting makes the build system easier.
And dynamic loading of the native code is SO much more flexible than DllImport. I should no longer have to build so many different copies of the pinvoke code, one for each possible name of the native library.
Unless I bump into an unexpected problem, I believe most of the complexity of the build system is going to go away.
I'm just getting started with this, but the new code already passes the xunit tests on Windows, so it looks promising.
Progress of this effort will be tracked here in this issue.
The text was updated successfully, but these errors were encountered: