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

Add packages for SQLite3 Multiple Ciphers #563

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

bricelam
Copy link
Contributor

@bricelam bricelam commented Nov 9, 2023

Depends on ericsink/cb#19

/cc @utelle

Comment on lines +2 to +6
"sdk":
{
"version": "6.0.100",
"rollForward": "latestFeature"
},
Copy link
Contributor Author

@bricelam bricelam Nov 9, 2023

Choose a reason for hiding this comment

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

This is to make the repo easier to work with locally. It won't use the .NET 8 SDK which errors for net6.0-ios and android targets. It reflects the 6.0.x semantics in the GitHub action.

Copy link
Owner

Choose a reason for hiding this comment

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

I've been adding something similar to my local work directory.

I'm a little bit tempted to deprecate net6.0 next week.

But net6.0 is still in support for another year.

But the net6.0 mobile targets are a special case.

Copy link
Owner

Choose a reason for hiding this comment

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

While I'm confessing my temptations, I'd love to deprecate support for .NET Framework too, but that's a whole different can of worms. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In our fork, I compromised by supporting the base net6.0 TFM, but requiring net7.0-android and -ios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still want .NET Framework support, but only for VS.

Copy link
Owner

Choose a reason for hiding this comment

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

Speaking broadly, not just about SQLite...

I always wonder how the support story for .NET Framework would be different without VS in the picture. I assume it'll probably never move to [.NET Core and its descendants]. Lots of enterprise apps are in the same boat, but VS is a special case.

No comments expected -- I'm just musing a bit.

@bricelam
Copy link
Contributor Author

bricelam commented Nov 9, 2023

I’ll update this to use the e_ prefix to avoid clashes.

@ericsink
Copy link
Owner

ericsink commented Nov 9, 2023

I've reviewed these diffs and I don't see any obvious problems.

I do think the e_ prefix for sqlite3mc is the way to go.

The PR for cb has been merged and new builds are done, so the next time this PR is checked, I think it'll pass.

@bricelam
Copy link
Contributor Author

bricelam commented Nov 9, 2023

Updated to prefix with "e_". I also added a provider (but no bundle) package without the prefix to enable using the version installed from source (and any future OS packages).

@bricelam
Copy link
Contributor Author

bricelam commented Nov 9, 2023

The PR for cb has been merged and new builds are done, so the next time this PR is checked, I think it'll pass.

Looks like you still need to merge from staging to master.

@ericsink
Copy link
Owner

ericsink commented Nov 9, 2023

Oops! I thought I did that...

done. I'll rerun the checks.

@bricelam
Copy link
Contributor Author

bricelam commented Nov 9, 2023

Failed SQLitePCL.Tests.test_cases.test_sqlite3_soft_heap_limit64
Expected: 20971520
Actual:   10485760

@ericsink
Copy link
Owner

ericsink commented Nov 9, 2023

Hmmm. And this error wasn't happening when you ran the test suite using the builds you grabbed from the cb fork?

@bricelam
Copy link
Contributor Author

bricelam commented Nov 9, 2023

I was using the binaries from the sqlite3mc branch of https://github.com/utelle/SQLite3MultipleCiphers-cb and didn't get the failure locally.

@ericsink
Copy link
Owner

ericsink commented Nov 9, 2023

I see reasons to ask questions about how the heap limit functions might have different behavior for sqlite3mc.

But I see nothing to explain why the error wasn't happening for you before. It's like something is different about the new builds from the merged cb.

@utelle

@ericsink
Copy link
Owner

ericsink commented Nov 9, 2023

I pushed a commit into this PR to disable the failing tests, just for now.

@ericsink ericsink merged commit 07c8837 into ericsink:master Nov 9, 2023
1 check passed
@utelle
Copy link
Contributor

utelle commented Nov 9, 2023

I see reasons to ask questions about how the heap limit functions might have different behavior for sqlite3mc.

I have absolutely no idea. What I can say is that sqlite3mc does not actively interfer with the heap limits. sqlite3mc implements a VFS shim which sits between SQLite and the real VFS. If encryption is not enabled it passes the page data through to the real VFS unchanged.

But I see nothing to explain why the error wasn't happening for you before. It's like something is different about the new builds from the merged cb.

I will try to perform this heap limit test directly with sqlite3mc alone, outside of any .NET environment.

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.

3 participants