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

Conan package #124

Closed
Myroendan opened this issue Nov 14, 2023 · 29 comments
Closed

Conan package #124

Myroendan opened this issue Nov 14, 2023 · 29 comments

Comments

@Myroendan
Copy link
Contributor

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
Copy link
Owner

utelle commented Nov 14, 2023

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
Copy link
Contributor Author

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
Copy link
Owner

utelle commented Nov 16, 2023

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:

  • 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 - the include file of the original SQLite library
  • src/sqlite3ext.h - optional, required only when creating own SQLite extensions
  • src/sqlite3mc.h - main include file (includes all necessary header files)
  • src/sqlite3mc_vfs.h - public API of the encryption VFS
  • src/sqlite3userauth.h - optional, only used if user authentication extension is enabled
  • src/test_windirent.h

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
Copy link
Contributor Author

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
Copy link
Owner

utelle commented Nov 16, 2023

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 added a commit to Myroendan/SQLite3MultipleCiphers that referenced this issue Nov 17, 2023
- Closes issue utelle#124: Conan package
@Myroendan
Copy link
Contributor Author

Myroendan commented Nov 17, 2023

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
Copy link
Owner

utelle commented Nov 17, 2023

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 added a commit to Myroendan/SQLite3MultipleCiphers that referenced this issue Nov 19, 2023
- Closes issue utelle#124: Conan package
@Myroendan
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

utelle commented Nov 20, 2023

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
Copy link
Owner

utelle commented Nov 20, 2023

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
Copy link
Contributor Author

Myroendan commented Nov 20, 2023

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
Copy link
Owner

utelle commented Nov 20, 2023

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
Copy link
Contributor Author

Myroendan commented Nov 20, 2023

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 added a commit to Myroendan/SQLite3MultipleCiphers that referenced this issue Nov 20, 2023
- Closes issue utelle#124: Conan package
- Add sqlite3mc_version.h to cmake install
Myroendan added a commit to Myroendan/SQLite3MultipleCiphers that referenced this issue Nov 20, 2023
- Closes issue utelle#124: Conan package
- Add sqlite3mc_version.h to cmake install
@Myroendan
Copy link
Contributor Author

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

Myroendan added a commit to Myroendan/SQLite3MultipleCiphers that referenced this issue Nov 21, 2023
- Closes issue utelle#124: Conan package
- CMake: add sqlite3mc_version.h to public headers
@utelle
Copy link
Owner

utelle commented Nov 24, 2023

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

@utelle
Copy link
Owner

utelle commented Nov 25, 2023

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
Copy link
Contributor Author

  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

  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.

Could you please clarify point 2 and 3 with them?

@utelle
Copy link
Owner

utelle commented Nov 25, 2023

  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 conan-io/conan-center-index#21358. We will see what answer we get.

@Myroendan
Copy link
Contributor Author

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
Copy link

SpaceIm commented Nov 25, 2023

Coming from conan-io/conan-center-index#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
Copy link
Owner

utelle commented Nov 25, 2023

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
Copy link
Owner

utelle commented Nov 26, 2023

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

@Myroendan
Copy link
Contributor Author

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

Great! Thank you.

@Myroendan
Copy link
Contributor Author

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

@utelle
Copy link
Owner

utelle commented Jan 20, 2024

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
Copy link
Owner

utelle commented Mar 17, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

utelle commented Mar 21, 2024

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.

@utelle utelle closed this as completed Mar 21, 2024
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

No branches or pull requests

4 participants
@utelle @SpaceIm @Myroendan and others