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

V2 #255

Closed
ericsink opened this issue Feb 15, 2019 · 22 comments
Closed

V2 #255

ericsink opened this issue Feb 15, 2019 · 22 comments

Comments

@ericsink
Copy link
Owner

ericsink commented Feb 15, 2019

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:

  • Uses SDK-style projects with multi-targeting
  • Deprecates WP8, WP8.1, and pre-netstandard PCL profiles
  • Uses Marshal.GetDelegateForFunctionPointer instead of DllImport

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.

@ericsink
Copy link
Owner Author

Over in #253, @bricelam asked:

Are any other breaking changes planned for 2.0?

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:

SQLitePCL.raw.SetProvider(new SQLitePCL.SQLite3Provider_e_sqlite3())

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:

SQLitePCL.Setup.Load("e_sqlite3.dll")

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?"

@ericsink
Copy link
Owner Author

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.

@ericsink
Copy link
Owner Author

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. :-)

@bricelam
Copy link
Contributor

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.

@AwsomeCode
Copy link

Plz include this as well #244

@ericsink
Copy link
Owner Author

@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.

@AwsomeCode
Copy link

@ericsink Main reason is that iOS having issue with SQLCipher 3.
App is getting suspended/terminated whenever it's minimises, Switching Apps or Home button pressed.
For eg. Issue is if your filling form and if we switch app to check something on other app and switch back to main app and its get restarted. I think this is important for everyone.

#241
sqlcipher/sqlcipher#255

@ericsink
Copy link
Owner Author

@AwsomeCode Oh yeah, that issue. Thanks for reminding me.

@AwsomeCode
Copy link

@ericsink Thanks for your hard work 👍

@sjlombardo
Copy link

@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.

@ericsink
Copy link
Owner Author

@sjlombardo I wish I had posted that reminder myself.

@ericsink
Copy link
Owner Author

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.

@bricelam
Copy link
Contributor

bricelam commented Feb 18, 2019

Would there still be a hook (like ISQLite3Provider) for someone to implement it even if you didn’t enable it out of the box?

@ericsink
Copy link
Owner Author

(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.

@bricelam
Copy link
Contributor

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.

@ericsink
Copy link
Owner Author

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:

  1. Dropping support for winsqlite3
  2. Dropping all support for ISQLite3Provider pluggability

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.

@ericsink
Copy link
Owner Author

@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.

@bricelam
Copy link
Contributor

Lol, I’m going to post another related one soon too. I love T4, but it’s definitely in a sad state.

@bricelam
Copy link
Contributor

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...

@ericsink
Copy link
Owner Author

FWIW., at this point I plan to keep winsqlite3 support in.

@ericsink
Copy link
Owner Author

ericsink commented Mar 1, 2019

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.

@ericsink
Copy link
Owner Author

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:

https://github.com/ericsink/SQLitePCL.raw/blob/v2/v2.md

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

No branches or pull requests

4 participants