-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
Comments
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. |
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. Which ones shouldn't be used by users? I would only move the public ones to the include folder. |
Great. TIA.
The "problem" with SQLite is that there are so many configuration options.
Only the flagged header files should be made public, all not flagged header files are used internally only. |
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.
Regarding zlib the user can choose to use the included |
- Closes issue utelle#124: Conan package
Thank you for the information. 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: 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. |
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 will try to study the guidelines within the next days.
What sort of test package? For example, my GitHub workflow executes some SQL scripts testing to access encrypted database files.
Ok, I'll wait with merging, until I get the green light from you.
Meaning what? IMHO it's not a bad idea to keep these files in a separate directory in the repo. |
- Closes issue utelle#124: Conan package
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.
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.
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. |
I have found an issue during testing when I include sqlite3mc.h instead of sqlite3.h: 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? |
In the conan recipe no header files are mentioned. Maybe the header file
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. |
Ok.
Ok.
I will check with the conan docs.
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. |
Yeah that was the issue. In this case, I have to modify the CMakeLists.txt to copy every header file during install.
I will add it to the recipe. |
Really EVERY header file? It would be good if that could be restricted to the header files required by applications using sqlite3mc.
To the CMake recipe I guess. I will still wait a bit before merging your PR. |
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.
Yes, I will update my PR this evening. |
- Closes issue utelle#124: Conan package - Add sqlite3mc_version.h to cmake install
- Closes issue utelle#124: Conan package - Add sqlite3mc_version.h to cmake install
I've pushed the latest commit, I think this one can be merged if no errors found. |
- Closes issue utelle#124: Conan package - CMake: add sqlite3mc_version.h to public headers
@Myroendan: I created a PR in the Conan repository. Unfortunately, at least 2 checks failed. |
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. |
Could you please clarify point 2 and 3 with them? |
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.
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
In the Conan documentation I see nowhere mentioned that CMake support is restricted to version below 3.15.7.
I opened issue conan-io/conan-center-index#21358. We will see what answer we get. |
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. |
Coming from conan-io/conan-center-index#21358 conancenter recipes adjust This |
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.
Sorry, this remark was unnecessary and inadequate.
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. |
Finally, both Conan Pipelines succeeded. Now, we have to wait for the required 2 approving reviews. |
Great! Thank you. |
HI, |
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. |
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. |
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. |
Finally, the Conan package is available: sqlite3mc/1.8.0.
Me too. I guess a PR has to be done to update the Conan recipe for each new version.
I really hope that adding new versions will be a smoother process. |
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.
The text was updated successfully, but these errors were encountered: