utelle / SQLite3MultipleCiphers

SQLite3 encryption extension with support for multiple ciphers
https://utelle.github.io/SQLite3MultipleCiphers/
MIT License
420 stars 77 forks source link

Conan package #124

Closed Myroendan closed 8 months ago

Myroendan commented 1 year ago

It would be nice to have a conan package available for this project. I've created a conan2 recipe, let me know if you would like to review it.

utelle commented 1 year ago

It would be nice to have a conan package available for this project. I've created a conan2 recipe, let me know if you would like to review it.

Admittedly, I haven't used Conan up to now. However, it is certainly a good idea to support package management systems to make the component available for more users/developers.

So, yes, I'd like to review your Conan recipe.

Myroendan commented 1 year ago

I will create a PR for Issue#109 regarding cmake install first. If the build system handles the copy of the files (headers, lib etc..), the conan recipe will be shorter and cleaner. I have one question to this topic. This is the list of headers which are added to the cmake target: src/cipher_common.h src/cipher_config.h src/fastpbkdf2.h src/mystdint.h src/rijndael.h src/sha1.h src/sha2.h src/sqlite3.h src/sqlite3ext.h src/sqlite3mc.h src/sqlite3mc_vfs.h src/sqlite3userauth.h src/test_windirent.h

Which ones shouldn't be used by users? I would only move the public ones to the include folder.

utelle commented 1 year ago

I will create a PR for Issue#109 regarding cmake install first.

Great. TIA.

If the build system handles the copy of the files (headers, lib etc..), the conan recipe will be shorter and cleaner.

The "problem" with SQLite is that there are so many configuration options.

I have one question to this topic. This is the list of headers which are added to the cmake target:

Which ones shouldn't be used by users? I would only move the public ones to the include folder.

Only the flagged header files should be made public, all not flagged header files are used internally only.

Myroendan commented 1 year ago

I will add to the recipe that zlib and icu are conditionally required, based on the compiler flags. But I haven’t found information about the minimum required versions. Do you have a suggestion?

utelle commented 1 year ago

I will add to the recipe that zlib and icu are conditionally required, based on the compiler flags. But I haven’t found information about the minimum required versions. Do you have a suggestion?

Well, actually the SQLite extensions should work with any prior version. Actually, I haven't tested whether there are prior versions of zlib and icu, which would not work. We can set the following minimum version numbers for now, although it is very likely that prior versions could be used as well.

  1. ICU: The first version used together with SQLite3 Multiple Ciphers was version 67.1.
  2. zlib: let's use version 1.2.9 as the minimum version. That version was released in 2017.

Regarding zlib the user can choose to use the included miniz implementation avoiding the external dependency.

Myroendan commented 1 year ago

Thank you for the information. I have added the conan recipe in the PR. I wrote (the non-existing) version 1.7.5, since that will be the first version that contains the latest cmake changes required by this recipe. The SHA hash of the new package file will be needed in the conandata.yml.

I think you should be the one to upload the package to conan center as the project owner. You can find the guideline for the process here: https://docs.conan.io/2/tutorial/conan_repositories/conan_center.html

But based on the guideline, a test_package is necessary. I'll have to add one to my recipe. I'll update my PR with it in a few days.

I believe the conan folder won't be necessary in the repo once the package is uploaded to the conan center.

utelle commented 1 year ago

I have added the conan recipe in the PR. I wrote (the non-existing) version 1.7.5, since that will be the first version that contains the latest cmake changes required by this recipe. The SHA hash of the new package file will be needed in the conandata.yml.

Thanks. We will see what the version number of the next release will be. Maybe 1.7.5, maybe 1.8.0 - depends on how my own activities will proceed.

I think you should be the one to upload the package to conan center as the project owner. You can find the guideline for the process here: https://docs.conan.io/2/tutorial/conan_repositories/conan_center.html

I will try to study the guidelines within the next days.

But based on the guideline, a test_package is necessary. I'll have to add one to my recipe.

What sort of test package? For example, my GitHub workflow executes some SQL scripts testing to access encrypted database files.

I'll update my PR with it in a few days.

Ok, I'll wait with merging, until I get the green light from you.

I believe the conan folder won't be necessary in the repo once the package is uploaded to the conan center.

Meaning what? IMHO it's not a bad idea to keep these files in a separate directory in the repo.

Myroendan commented 1 year ago

What sort of test package? For example, my GitHub workflow executes some SQL scripts testing to access encrypted database files.

It's for testing the package itself rather than the underlying source code. It ensures that consumers can link to the package and use it.

Ok, I'll wait with merging, until I get the green light from you.

I have added a new commit, I think it's okay now. The remaining necessary modifications are to update the version numbers once the 1.7.5/1.8.0 is released, and to add the SHA to the conanfile.yml. Then the process of adding the package to the conan center can be started. When you check out the repo mentioned in the guide, there will be the recipes folder. There you can create the folder for the project, and everything from this repo's conan folder can go there.

Meaning what? IMHO it's not a bad idea to keep these files in a separate directory in the repo.

As far as I remember, projects usually don't contain the conan recipe in their repo, only on the conan center. But if you want to have it here as well, that's fine by me.

Myroendan commented 1 year ago

I have found an issue during testing when I include sqlite3mc.h instead of sqlite3.h: sqlite3mc.h(16,10): error C1083: Cannot open include file: 'sqlite3mc_version.h': No such file or directory I'll have to check that.

A question: I have seen in many conan recipes, that they define the option fPIC true by default, and remove the option if the os is Windows. Do you think I should add this option to the recipe?

utelle commented 1 year ago

I have found an issue during testing when I include sqlite3mc.h instead of sqlite3.h: sqlite3mc.h(16,10): error C1083: Cannot open include file: 'sqlite3mc_version.h': No such file or directory I'll have to check that.

In the conan recipe no header files are mentioned. Maybe the header file sqlite3mc_version.h was not copied to wherever the header files are expected to be?

A question: I have seen in many conan recipes, that they define the option fPIC true by default, and remove the option if the os is Windows. Do you think I should add this option to the recipe?

Good question. AFAIK the autoconf/automake build files coming with sqlite3mc handle that automatically under the hood. But in CMake it may be necessary to specify the option explicitly.

utelle commented 1 year ago

What sort of test package? For example, my GitHub workflow executes some SQL scripts testing to access encrypted database files.

It's for testing the package itself rather than the underlying source code. It ensures that consumers can link to the package and use it.

Ok.

I have added a new commit, I think it's okay now. The remaining necessary modifications are to update the version numbers once the 1.7.5/1.8.0 is released, and to add the SHA to the conanfile.yml.

Ok.

Then the process of adding the package to the conan center can be started. When you check out the repo mentioned in the guide, there will be the recipes folder. There you can create the folder for the project, and everything from this repo's conan folder can go there.

I will check with the conan docs.

Meaning what? IMHO it's not a bad idea to keep these files in a separate directory in the repo.

As far as I remember, projects usually don't contain the conan recipe in their repo, only on the conan center. But if you want to have it here as well, that's fine by me.

On the one hand, it's logical to not have the recipe in the repo. At least not the final version due to the sha256 checksum.

On the other hand, it does no harm to have the recipe template in the repo. For now, I guess I will keep it in the repo.

Myroendan commented 1 year ago

In the conan recipe no header files are mentioned. Maybe the header file sqlite3mc_version.h was not copied to wherever the header files are expected to be?

Yeah that was the issue. In this case, I have to modify the CMakeLists.txt to copy every header file during install.

Good question. AFAIK the autoconf/automake build files coming with sqlite3mc handle that automatically under the hood. But in CMake it may be necessary to specify the option explicitly.

I will add it to the recipe.

utelle commented 1 year ago

In the conan recipe no header files are mentioned. Maybe the header file sqlite3mc_version.h was not copied to wherever the header files are expected to be?

Yeah that was the issue. In this case, I have to modify the CMakeLists.txt to copy every header file during install.

Really EVERY header file? It would be good if that could be restricted to the header files required by applications using sqlite3mc.

Good question. AFAIK the autoconf/automake build files coming with sqlite3mc handle that automatically under the hood. But in CMake it may be necessary to specify the option explicitly.

I will add it to the recipe.

To the CMake recipe I guess.

I will still wait a bit before merging your PR.

Myroendan commented 1 year ago

In the conan recipe no header files are mentioned. Maybe the header file sqlite3mc_version.h was not copied to wherever the header files are expected to be?

Yeah that was the issue. In this case, I have to modify the CMakeLists.txt to copy every header file during install.

Really EVERY header file? It would be good if that could be restricted to the header files required by applications using sqlite3mc.

You're right, I have checked and sqlite3mc_version.h is the only additional header that's needed, so I only add that to the list when installing.

Good question. AFAIK the autoconf/automake build files coming with sqlite3mc handle that automatically under the hood. But in CMake it may be necessary to specify the option explicitly.

I will add it to the recipe.

To the CMake recipe I guess.

I will still wait a bit before merging your PR.

Yes, I will update my PR this evening.

Myroendan commented 1 year ago

I've pushed the latest commit, I think this one can be merged if no errors found.

utelle commented 1 year ago

@Myroendan: I created a PR in the Conan repository. Unfortunately, at least 2 checks failed. Could you please take a look: https://github.com/conan-io/conan-center-index/pull/21354? TIA.

utelle commented 1 year ago

In the meantime I applied a few minor adjustments (i.e. removing C++ compiler settings, because the library is a C library). Nevertheless, some build check steps still fail. The reason seems to be, that in the Conan check environment only an old CMake version 3.15.7 is available while sqlite3mc's CMake file asks for version 3.24.0 or higher.

Myroendan commented 1 year ago
  1. I see the yaml linter threw an error for the "---" beginning. I included that in the first place due to their guidelines: the yaml linter warned me locally that it was missing, well nevermind. I see you removed it from the conandata.yml, the config.yml also has it.

  2. conan v1 pipeline: the recipe is not conan1 compatible. I don't know if its a requirement for a new recipe, but it would be weird, since they're trying to push everyone towards conan2.

    The reason seems to be, that in the Conan check environment only an old CMake version 3.15.7 is available while sqlite3mc's CMake file asks for version 3.24.0 or higher

  3. Yeah, this is also interesting. I think we could downgrade our CMake requirement to 3.15.7, I don't think we use features that require a higher version (I'll have to check it). But does this mean conan is not supporting projects using CMake with a version higher than 3.15.7? That's a 4 years old version.

Could you please clarify point 2 and 3 with them?

utelle commented 1 year ago
  1. I see the yaml linter threw an error for the "---" beginning. I included that in the first place due to their guidelines: the yaml linter warned me locally that it was missing, well nevermind. I see you removed it from the conandata.yml, the config.yml also has it.

Yes, I saw that, after the checks failed again. I'll remove the line from that file, too, once I get information about the other issue.

  1. conan v1 pipeline: the recipe is not conan1 compatible. I don't know if its a requirement for a new recipe, but it would be weird, since they're trying to push everyone towards conan2.

Personally, I don't see that failure as a real error, because all recipes should be using version 2. Interestingly, less than 20 recipes ofover 1600 recipes refer to version 2 at all in their required_conan_version, and if they do it is a combination like required_conan_version = ">=1.56.0 <2 || >=2.0.6".

  1. Yeah, this is also interesting. I think we could downgrade our CMake requirement to 3.15.7, I don't think we use features that require a higher version (I'll have to check it). But does this mean conan is not supporting projects using CMake with a version higher than 3.15.7? That's a 4 years old version.

In the Conan documentation I see nowhere mentioned that CMake support is restricted to version below 3.15.7.

Could you please clarify point 2 and 3 with them?

I opened issue https://github.com/conan-io/conan-center-index/issues/21358. We will see what answer we get.

Myroendan commented 1 year ago

Personally, I don't see that failure as a real error, because all recipes should be using version 2. Interestingly, less than 20 recipes ofover 1600 recipes refer to version 2 at all in their required_conan_version, and if they do it is a combination like required_conan_version = ">=1.56.0 <2 || >=2.0.6".

I think it's because conan2 was released this February, and most of these projects were available since conan1. The recipes were adjusted to support conan2 as well, but they didn't want to break the already existing conan1 support.

SpaceIm commented 1 year ago

Coming from https://github.com/conan-io/conan-center-index/issues/21358

conancenter recipes adjust required_conan_version depending on features they rely on. It turns out that 95% of conancenter recipes use "conan v2" features implemented in conan<=1.53.0, and therefore there is absolutely no reason to ask for a more recent conan version to users (except when you don't like them).

This required_conan_version = ">=1.56.0 <2 || >=2.0.6" (which should be required_conan_version = ">=1.60.0 <2 || >=2.0.5" actually) you can see in few recipes comes from usage of a feature (foo/<host_version> in build requirements when foo is also in requirements) implemented in conan 2.0.5 and backported to conan v1.

utelle commented 1 year ago

conancenter recipes adjust required_conan_version depending on features they rely on. It turns out that 95% of conancenter recipes use "conan v2" features implemented in conan<=1.53.0, and therefore there is absolutely no reason to ask for a more recent conan version to users

Well, all over the Conan place one reads about the transition to version 2, and that recipes have to be compatible with version 2. Staying compatible with version 1 means that no "version 2 only" feature can be used.

(except when you don't like them).

Sorry, this remark was unnecessary and inadequate.

This required_conan_version = ">=1.56.0 <2 || >=2.0.6" (which should be required_conan_version = ">=1.60.0 <2 || >=2.0.5" actually) you can see in few recipes comes from usage of a feature (foo/<host_version> in build requirements when foo is also in requirements) implemented in conan 2.0.5 and backported to conan v1.

I changed the version requirement, but the Conan v1 Pipeline check still fails. However, the Conan v2 Pipeline check finally completed successfully a few minutes ago.

utelle commented 1 year ago

Finally, both Conan Pipelines succeeded. Now, we have to wait for the required 2 approving reviews.

Myroendan commented 1 year ago

Finally, both Conan Pipelines succeeded. Now, we have to wait for the required 2 approving reviews.

Great! Thank you.

Myroendan commented 10 months ago

HI, did you receive any news from conan team since?

utelle commented 10 months ago

did you receive any news from conan team since?

No. The Conan pipelines all show green, but the PR is still waiting for a second review. On this page you can find a section "Time Spent in Review". The average is well above 30 days at the moment, with many entries way over 60 days. So, just keep your patience. I will close this issue, once the recipe will have been accepted.

utelle commented 8 months ago

Week after week is passing by ... nothing happens ... no comments to my/our remarks ... no new review ...

I have to admit that I'm gradually loosing interest in supporting a Conan recipe for my component.

Myroendan commented 8 months ago

Yeah, I didn't expect it would take this much time. I also wonder what's the process of adding new versions, once you have an existing conan package. Hopefully you don't have to wait months for each release.

utelle commented 8 months ago

Finally, the Conan package is available: sqlite3mc/1.8.0.

I also wonder what's the process of adding new versions, once you have an existing conan package.

Me too. I guess a PR has to be done to update the Conan recipe for each new version.

Hopefully you don't have to wait months for each release.

I really hope that adding new versions will be a smoother process.