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

Fixes for Windows mswin & sqlcipher #332

Merged
merged 1 commit into from
Aug 10, 2022

Conversation

MSP-Greg
Copy link
Contributor

@MSP-Greg MSP-Greg commented Aug 9, 2022

Changing the vcpkg input from "sqlcipher" to "sqlite3 sqlcipher" seems to allow compile & test to pass? Not.sure.

Also, ci log for 'bundle exec rake test' shows:

info: sqlite3-ruby version: 1.5.0.rc1/1.5.0.rc1
info: sqlite3 version: 3.39.1/3.37.2
info: sqlcipher?: true
info: threadsafe?: true

Not sure what 'sqlite3 version: 3.39.1/3.37.2' is about?

Lastly, CI is ran on Windows 'mingw', which is a Ruby head build, 'ucrt' is also available... Didn't change.

@flavorjones
Copy link
Member

flavorjones commented Aug 9, 2022

Hey, @MSP-Greg - thank you for putting this together!

I think the output "3.39.1/3.37.2" is telling us that this isn't working as expected -- I think it's compiling against sqlite3 and dynamically loading sqlcipher (or vice-versa). This is pointing at a lack of both compile-time and run-time assertions that we're pulling in the expected flavor of db library, a shortcoming that dates back to the original PR, #232.

I'm going to add a commit onto this PR that will introduce some checking and I think that will confirm my suspicions by failing CI.

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Aug 9, 2022

@flavorjones

Need.more.coffee.

Thank you!

Last night I looked here to double check whether it was running CI against mswin. I'm working on some custom Ruby builds, and wanted to see about extension gems. This morning I'm looking at the code...

I think the output "3.39.1/3.37.2" is telling us that this isn't working as expected

So that's runtime vs compile, correct? Question - if one builds with sqlcipher, can one use it just like 'normal' sqlite3? Or, is it mutually exclusive?

Regardless, I've got vcpkg locally, anything I can check about the sqlcipher install?

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Aug 9, 2022

Installed vcpkg sqlcipher locally. Two files sqlcipher/sqlite3.h and sqlcipher/sqlite3ext.h. Haven't installed vcpkg sqlite3 yet.

EDIT:

I've got two vcpkg installs, one using VS 2019 & OpenSSL 1.1.1, and one with VS 2022 & OpenSSL 3.0.x

Installed sqllite3 to one and sqlcipher to the other:

  1. pc file exists for sqlite3, no file for sqlcipher.
  2. include files for sqlite3 are at root, include files for sqlcipher are in a sqlcipher folder.

Hacked a few files and sqlcipher installed correctly and showed info: sqlite3 version: 3.37.2/3.37.2. Tests passed. Not quite sure how to make the code work with all platforms?

@flavorjones
Copy link
Member

I rebased onto master and added checks to extconf.rb and the test suite. Let's see what that tells us before I speculate on the vcpkg packages.

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Aug 9, 2022

I came up with a possible solution, not sure about naming, etc. Seems like all CI passes. I rebased a few hours ago, and added a commit. Might be a bit hacky...

Test suites:
https://github.com/MSP-Greg/sqlite3-ruby/actions/runs/2827064397
https://github.com/MSP-Greg/sqlite3-ruby/actions/runs/2827065516

sqlcipher log:
https://github.com/MSP-Greg/sqlite3-ruby/runs/7751455763?check_suite_focus=true#step:6:59

Seems like the main issue is that vcpkg sqlcipher is the only platform with the two header files in an sqlcipher folder?

@flavorjones
Copy link
Member

Your solution seems reasonable to me, though I'm realizing now there are a few problems that are interacting here:

I'm going to add your commit to this PR to see what happens.

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Aug 9, 2022

I'm going to add your commit to this PR to see what happens.

Sorry, forgot. The vcpkg input needs to be vcpkg: "sqlcipher", then it should work...

@flavorjones
Copy link
Member

Right, thanks. Fixed!

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Aug 9, 2022

  1. libname is called a few times, should that value be saved? Maybe lib_name, @lib_name, or something?

  2. extconf.rb - wondering about the statement elsif find_header("#{libname}/sqlite3.h"). maybe should be:
    elsif @lib_name == 'sqlcipher' && find_header("sqlcipher/sqlite3.h")?

Trying to tighten what runs it, as the only problem is mswin & sqlcipher?

@flavorjones
Copy link
Member

🤔 I'm going to remove my attempt in the extconf to determine whether the libraries is "really sqlcipher", because the sqlcipher library doesn't appear to present that information in a stable, predictable way.

That will get mswin green, I think. I'm still worried about macos versioning being strange, but I can handle that in a different PR.

@flavorjones
Copy link
Member

@MSP-Greg I'm not too worried about making this code perfect -- sqlcipher is an edge case that I don't think many people opt into, and if they do they're responsible for bringing their own (and using the config flags if necessary).

If you'd like to clean it up, I'd be happy to take it, but I'm a bit under water today and won't get to it.

and allow for header in sqlcipher folder

Co-authored-by: Mike Dalessio <mike.dalessio@gmail.com>
@flavorjones
Copy link
Member

I think this can be merged! @MSP-Greg thanks again for digging in on this. Are you OK with the changeset as-is?

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Aug 9, 2022

Are you OK with the changeset as-is?

Yes. Maybe later we can look at reducing the calls to some methods. Thanks for the quick review, etc...

JFYI, SQLCipher has a new release (4.5.2, uses SQLite 3.39.2), opened a PR in vcpkg to update it. Haven't checked MSYS2 yet.

@MSP-Greg MSP-Greg changed the title sqlite3-ruby.yml - mswin/sqlcipher - install sqlite3 Fixes for Windows mswin & sqlcipher Aug 9, 2022
@flavorjones flavorjones merged commit 94b7447 into sparklemotion:master Aug 10, 2022
@MSP-Greg MSP-Greg deleted the 00-sqlcipher-mswin branch August 10, 2022 14:15
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.

None yet

2 participants