-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
sqlitecpp: Add has_codec option #22795
base: master
Are you sure you want to change the base?
Conversation
This enable the database encryption support
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.
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.
Hooks produced the following warnings for commit a0b479csqlitecpp/2.5.0@#f550f50d701e9a0122c31e886057ae07
sqlitecpp/2.4.0@#b6846a53de52954edbbe00b16502cacf
|
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 for your patience while this PR got reviewed, I have a minor last question before approving :)
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 enable the database encryption support
Specify library name and version: sqlitecpp/X