-
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
Add recipe for sqlite3mc #21354
Add recipe for sqlite3mc #21354
Conversation
This comment has been minimized.
This comment has been minimized.
Remove C++ compiler options, because this library is a C library
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.
recipes/sqlite3mc/all/conandata.yml
Outdated
"1.8.0": | ||
url: "https://github.com/utelle/SQLite3MultipleCiphers/archive/refs/tags/v1.8.0.tar.gz" | ||
sha256: "13D9B939BEF7C7371D58A3874F83B18CF330EB2171205B3680ACDDB2215BE0E5" | ||
strip_root: 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.
strip_root: true |
recipes/sqlite3mc/all/conanfile.py
Outdated
self.settings.rm_safe("compiler.libcxx") | ||
|
||
def source(self): | ||
get(self, **self.conan_data["sources"][self.version]) |
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.
get(self, **self.conan_data["sources"][self.version]) | |
get(self, **self.conan_data["sources"][self.version], strip_root=True) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 @utelle thanks a lot for taking the time to contribute the recipe for your library, we're always happy to get author's involvment with their recipes as that usually makes the resulting packages much better
We'll now take a look at the recipe and try to build some permutation of some of the options you've added for the library, thanks a lot for your patience :)
Co-authored-by: SpaceIm <[email protected]>
Builds look good on my side, will properly review once the CI finishes this run, thanks a lot for your patience! :) |
This comment has been minimized.
This comment has been minimized.
Over 2 months have passed ... and I see no progress at all. Is there anything I can or should do from my side? |
This comment has been minimized.
This comment has been minimized.
recipes/sqlite3mc/all/conanfile.py
Outdated
self.cpp_info.libs = ["sqlite3mc"] | ||
else: | ||
self.cpp_info.libs = ["sqlite3mc_static"] | ||
if self.settings.os in ("Linux", "Macos"): |
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.
if self.settings.os in ("Linux", "Macos"): | |
if self.settings.os in ("Linux", "FreeBSD"): |
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.
Could you please explain, why "Macos"
should be replaced by "FreeBSD"
? IMHO this can't be correct, because
- the libraries
pthread
,dl
, andm
are required forMacos
, too, and - in line 224
self.settings.os
is explicitly checked for"Macos"
again, because an additional attribute is required underMacos
.
I could understand, if you would want the following change:
if self.settings.os in ("Linux", "FreeBSD", "Macos"):
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 see that these are indeed linked against by the project on macOS: https://github.com/utelle/SQLite3MultipleCiphers/blob/7e8ecba9a82ea1336b118a21ccb733f4ed292efa/CMakeLists.txt#L262-L266
Excuse the dumb question (I'm not that familiar with the platform): do m
and dl
even exist on macOS? It's the first time I'm seeing these being linked against outside of Linux/FreeBSD. As far as I know, these are only provided by glibc and similar libc implementations, which is not available on macOS.
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 see that these are indeed linked against by the project on macOS: https://github.com/utelle/SQLite3MultipleCiphers/blob/7e8ecba9a82ea1336b118a21ccb733f4ed292efa/CMakeLists.txt#L262-L266
Excuse the dumb question (I'm not that familiar with the platform): do
m
anddl
even exist on macOS? It's the first time I'm seeing these being linked against outside of Linux/FreeBSD. As far as I know, these are only provided by glibc and similar libc implementations, which is not available on macOS.
Actually, I'm not very familiar with macOS either. However, the GitHub CI action (for example this run) runs without errors on macOS, and up to now I haven't received any complaints from users of my component.
recipes/sqlite3mc/all/conanfile.py
Outdated
tc.variables["SQLITE_ENABLE_CSV"] = self.options.enable_csv | ||
tc.variables["SQLITE_ENABLE_EXTFUNC"] = self.options.enable_extfunc | ||
tc.variables["SQLITE_ENABLE_GEOPOLY"] = self.options.enable_geopoly | ||
tc.variables["SQLITE_ENABLE_JSON1"] = self.options.enable_json1 |
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 tried to find an usage for this option, but I failed to find it. I see it in the CMakeLists, but I don't see it in the code, I mean, a required definition like ifdef
:
Could you please explain a bit more how does it work, same to other options?
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 tried to find an usage for this option, but I failed to find it. I see it in the CMakeLists, but I don't see it in the code, I mean, a required definition like
ifdef
:Could you please explain a bit more how does it work, same to other options?
Several SQLite extensions can be included optionally at compile time using #ifdef
statements. Mostly this happens in the main C file sqlite3mc.c (like for SQLITE_ENABLE_CSV
and SQLITE_ENABLE_EXTFUNC
).
But there are also extension that are already included in the original (resp patched) SQLite source amalgamation C file sqlite3patched.c (like for SQLITE_ENABLE_GEOPOLY
).
The symbol SQLITE_ENABLE_JSON1
is different. This symbol does indeed not exist any more. In prior SQLite versions it was available, but in recent versions the JSON extension is included per default, but it could be explicitly excluded by specifying the symbol SQLITE_OMIT_JSON
.
For the original project SQLite3 Multiple Ciphers I decided to expose many common SQLite configuration options to give users a high degree of freedom to choose the options and extensions most appropriate for their project. Per default many extensions are enabled, but in some projects there exist memory restrictions, so that users want to be able to disable unneeded extensions.
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 @utelle thanks a lot for your contribution, and sorry about the delay, we appreciate your patience a lot, some PRs get longer to get reviewed than other, sorry about that :)
|
||
int main(int argc, char *argv[]) | ||
{ | ||
sqlite3 *db; |
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! Could we get a simplified version of this? It's not in CCI's policy to have extensive tests in the test_package, it should be as minimal as possible while testing the correct package of the library :)
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! Could we get a simplified version of this? It's not in CCI's policy to have extensive tests in the test_package, it should be as minimal as possible while testing the correct package of the library :)
Well, this is the most simple test I could think of. To verify that the package was build correctly it is necessary to create an encrypted database with some content and to access it with the right passphrase and a wrong passphrase afterwards.
If that test is too complex, we don't need any test at all.
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 removed the configuration option SQLITE_ENABLE_JSON1
, because this option is no longer used by SQLite. And I added FreeBSD
to the os list (instead of replacing Macos
by FreeBSD
), because this seems to be the only logical modification.
Regarding the "extensive" test I have no idea what you expect. What could and/or should be tested, if not the proper build of the package?
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,
we could remove the error handlings from the test, use logical OR on the result during operations, and check it once in the end. That would make it 35-40 lines shorter. Would that work?
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.
we could remove the error handlings from the test, use logical OR on the result during operations, and check it once in the end. That would make it 35-40 lines shorter. Would that work?
It is certainly possible to reduce the number of lines of code by improving the error handling code (for example, by introducing a small function that checks the return code and emits an error message if necessary), but I'm not sure if that is what @RubenRBS intends.
Conan v1 pipeline ✔️All green in build 10 (
Conan v2 pipeline ✔️
All green in build 10 (
|
@RubenRBS
Another month went by ... and my latest modifications and comments didn't get any helpful response. I have to admit that I gradually loose interest in this endeavor. |
Sorry about this, I know that feeling ignored after taking the time to contribute a PR with all the effort it requires can be discouraging - nothing further from the truth, you contribution is truly appreciated. Sometimes some PRs fall thru the cracks due to the sheer volume of PRs that CCI recives - we're working on systems to be able to better handle that load, so that things like this don't happen again, but that'd still take some time I've approved to unblock this and will ask the team to send a follow-up pr if necessary to get the final touches if any are needed, but again, sorry about the delay :) |
Well, I think I really showed a lot of patience while the PR was queued for processing, but once review(s) began I had expected a somewhat more continuous process. Working on a PR with long breaks inbetween requires more time and energy on both sides to pick up the loose ends where they were left off.
I fully understand that it is difficult to handle such a high volume of PRs, especially as the work is mostly done by volunteers in their sparetime. And of course it is important to carefully check the quality of new recipes to give Conan users a pleasant experience.
Thanks. In this context I have a question: how should updates of the recipe be handled? Actually, the current version of SQLite3 Multiple Ciphers is already 1.8.4. Which versions should be added to the Conan recipe? All intermediate versions? Or just the latest? Should old versions be dropped from the recipe? Or kept forever? |
* Add recipe for sqlite3mc * Adjust recipe Remove C++ compiler options, because this library is a C library * Add CMake version requirement * Add CMake requirement to test package * Add missing header in test * Adjust according to review * Fix conan v1 related issues * Apply suggestions from code review Co-authored-by: SpaceIm <[email protected]> * Remove option SQLITE_ENABLE_JSON1, add FreeBSD to os list --------- Co-authored-by: Rubén Rincón Blanco <[email protected]> Co-authored-by: SpaceIm <[email protected]>
Specify library name and version: sqlite3mc/1.8.0
I'm the author of the project SQLite3 Multiple Ciphers - sqlite3mc in short. The project adds an encryption extension to the SQLite library allowing to transparently encrypt the database files. As the name suggests the extension supports several common cipher schemes (used by other Open Source projects):
The actual cipher scheme used for database encryption can be chosen at runtime.
I was asked by a user of my project, @Myroendan, whether a Conan recipe could be provided. So, here it is.