-
Notifications
You must be signed in to change notification settings - Fork 2k
sqlitecpp: Add has_codec option #22795
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
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit f4e1ab6sqlitecpp/2.4.0@#2be6b31a3d5a21dbf5d19e5ebed2a040
sqlitecpp/2.5.0@#35083e3da81df24fd116b89a1df56aa4
|
@Alex-PLACET the has_codec option doesn't actually work. I believe CMakeLists of SQLiteCpp needs to be patched to use find_package for sqlcipher. As it stands it's trying to use pkgconfig and failing. |
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit ee03adcsqlitecpp/2.5.0@#85d07950cfca14785aa195b0129b27d8
sqlitecpp/2.4.0@#e14439bbecdd6c6c88489544974214d5
|
@RubenRBS When you have time, this PR requires a second approval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to remove the option, because does not make sense affecting the package ID in case it can not be used. Please, update the config_options:
def config_options(self):
if Version(self.version) < "3.3.1":
del self.options.has_codec
Still, users can override the sqlcipher options, so you should validate its option:
def validate(self):
if self.options.get_safe("has_codec") and not self.dependencies["sqlcipher"].options.enable_column_metadata:
raise ConanInvalidConfiguration(f"{self.ref} option has_codec=True requires 'sqlcipher/*:enable_column_metadata=True'")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thanks a lot, I have a few final requests, but otherwise looks great :)
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit b926f33sqlitecpp/2.5.0@#a2abbfe264e0df781cb507d68e89c904
sqlitecpp/2.4.0@#b5c738dcb349dd4b012a00e0acbc13b3
|
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 604980asqlitecpp/2.4.0@#39f934546b4f7520de8b01efda7e1b0f
sqlitecpp/2.5.0@#c9096c03739740a9581fedac30b5ac48
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ✔️All green in build 9 (
Conan v2 pipeline ✔️
All green in build 9 (
|
Hooks produced the following warnings for commit 9c7739fsqlitecpp/2.4.0@#e893f3ee7261c47f30879ec371dc289a
sqlitecpp/2.5.0@#871b640da9b7b4a3520858437c6c6da5
|
@RubenRBS I addressed your point. I let you review and approve it if it's ok |
@valgur @uilianries I need an approval to be able to merge. |
@Alex-PLACET I tried to build with this new option, and sqlitecpp is failing. Do you have a build log with sqlitecpp working?
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
not stale |
recipes/sqlitecpp/all/conanfile.py
Outdated
if self.options.get_safe("with_sqlcipher"): | ||
self.requires("sqlcipher/4.5.6") | ||
else: | ||
self.requires("sqlite3/3.45.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uilianries is it allowed to put a version range here? poco already uses this range and sqlite3 3.45.0 is not even in the conandata.yml anymore.
self.requires("sqlite3/3.45.0") | |
self.requires("sqlite3/[>=3.45.0 <4]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ask because I planned to create a PR and didn't want to conflict with this commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sqlite3 can have a version range like that, yes :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we do it here or in a own pr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I don't have time to fix this PR, I let you copy/cherry pick my commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the conan team can decide: #27088
This enable the database encryption support
9c7739f
to
c06c07c
Compare
Apologies for the delay.
|
This enable the database encryption support
Specify library name and version: sqlitecpp/X