Conversation
|
Gentle ping |
|
@apolukhin thank you that you pushing this idea again :) |
jcar87
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
Apologies this has taken so long.
We are happy to prioritize this - I've added review comments as this recipe differs considerably from Conan Center conventions - please let us know if you have any questions and we're happy to help!
|
@jcar87 I've fixed some of the issues and asked questions for the remaining issues. Please take a look |
|
@jcar87 gentle reminder |
Tests: протестировано github CI, на прод не влияет Conan center PR conan-io/conan-center-index#28173 --- Pull Request resolved: #1015 Co-authored-by: antoshkka <antoshkka@userver.tech> Co-authored-by: antoshkka <antoshkka@userver.tech> Co-authored-by: Antony Polukhin <antoshkka@gmail.com> commit_hash:25ab73cb03ff9b746536c13bab83aa668b35d175
recipes/userver/all/conanfile.py
Outdated
| def build(self): | ||
| # pg_config is required to build psycopg2 from source without system package. | ||
| # However, this approach fails on later stage, when venv for tests is built. | ||
| libpq = self.dependencies['libpq'] |
There was a problem hiding this comment.
If self.options.with_postgresql == False then this line will fail (there'll be no 'libpq' key). I assume the conditional in the next line could be changed to something like:
if self.options.with_postgresql and (libpq := self.dependencies['libpq']):
Or even simply:
if self.options.with_postgresql:
because the test on libpq is redundant if the option is enabled.
|
@apolukhin Hello! Thank you for not only maintaining Userver, but also bringing it as a pull request here. I see this recipe is huge and complex to maintain, so I would like to know if it would be possible to start with something really simple, as Arch Linux has done with their package: https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=userver-community, a really simple build with a few requirements only. I may understand, as you are one of the maintainers of that project, it would be favorable to give all those features available in the recipe as well, but as a side step, it will increase the maintainability of these recipes exponentially, and users may not be interested in consuming all those features in the end. Regards. |
|
@rklepov I've relaxed a bunch of dependency requirements. There are some issues with Boost other than 1.86 and something is wrong with concurrentqueue=1.0.4. We're investigating that, but that should not block from reviewing this PR. @uilianries I followed your advice and disabled Mongo and SQLite parts of the framework by default. Those technologies are not used in majority of projects I also updated the userver release. Please take a look |
…er PR review Fix issues noted in conan-io/conan-center-index#28173 --- Pull Request resolved: #1119 commit_hash:419d462bf40c3c306c38e92e9622429b6324d5f7
@apolukhin I've added some comments from my side. In fact nothing critical there. So I'm absolutely not opposed to merging the PR and eager to see userver recipe added to conan-center. Unfortunately I'm not the repository maintainer and so have no word here. Also I see that there're some red checks below. Not sure if that's important but likely not a positive sign. |
recipes/userver/all/conanfile.py
Outdated
| def build(self): | ||
| # pg_config is required to build psycopg2 from source without system package. | ||
| # However, this approach fails on later stage, when venv for tests is built. | ||
| if libpq := self.dependencies.get('libpq'): |
There was a problem hiding this comment.
Actually, doesn't seem that it will work. self.dependencies in fact is not a Python dict but rather an instance of ConanFileDependencies class. I get basically the same error as previously:
ERROR: userver/2.15: Error in build() method, line 246
if libpq := self.dependencies.get('libpq'):
KeyError: "'libpq' not found in the dependency set"
So I'm afraid there's no way but to follow the suggestion from my previous comment and check self.options.with_postgresql option.
There was a problem hiding this comment.
Fixed. Please check that it works fine in your environment (all the versions of those lines worked fine in my environment)
There was a problem hiding this comment.
Thanks! Works for me. I wonder what version of conan you're on? I'm simply using the latest one currently available: Conan version 2.25.2.
Anyway, I believe that this form of the check is actually more idiomatic.
| 'with_s3api': True, | ||
| 'with_grpc_reflection': True, | ||
| 'with_grpc_protovalidate': False, | ||
| 'mongo-c-driver/*:with_sasl': 'cyrus', |
There was a problem hiding this comment.
The items following this in default_options dict are in fact the options for the external dependencies.
However, as a consumer of the userver recipe, do I really need to see them here? They will be overridden by the values from my build profile anyway. Here's an example of conan output for that matter:
Options conflicts
grpc/1.69.0:php_plugin=False (current value)
userver/2.15->php_plugin=True
It is recommended to define options values in profiles, not in recipes
WARN: risk: There are options conflicts in the dependency graph
I assume the options are important for a developer of userver itself when they work on a component that needs particular facilities from one or more external libraries. But I suppose that it's preferable to keep the options in a local build profile too (probably also under a version control).
Going into details, for example, we can look at the options for grpc. Is it really a problem if I link userver to the version of grpc library with the listed plugins enabled (imagine that I already have that one in my own environment)? I assume it's not.
On the other hand, I realize that the options, for example, for librdkafka might be crucial (ssl etc.). However, arguably the better location to check the expected values of the options is validate() function below. And I see that you already do the checks for mongo-c-driver there.
There're the discussions that expand on the matter here and here.
To summarize, I suggest that we should consider removing the options for the external dependencies from default_options.
There was a problem hiding this comment.
Experimenting with reduced dependencies in this PR userver-framework/userver#1126
If our Conan CI shows no degradation, I'll update this PR
There was a problem hiding this comment.
Yep, that worked. Pushed in the changes to this PR
| tool_ch.cache_variables['USERVER_FEATURE_EASY'] = self.options.with_easy | ||
| tool_ch.cache_variables['USERVER_FEATURE_S3API'] = self.options.with_s3api | ||
| tool_ch.cache_variables['USERVER_FEATURE_GRPC_REFLECTION'] = self.options.with_grpc_reflection | ||
| tool_ch.cache_variables['USERVER_FEATURE_GRPC_PROTOVALIDATE'] = self.options.with_grpc_protovalidate |
There was a problem hiding this comment.
Not all the features and build options that are available in CMakeLists.txt are actually listed here and therefore available for configuration via a build profile or conan command line. Is that intentional? For example, for some particular reasons I'm personally interested in USERVER_DISABLE_PHDR_CACHE option among the others.
There was a problem hiding this comment.
This contradicts the "start with something really simple" review comment.
However, the USERVER_DISABLE_PHDR_CACHE option seems trivial, so I went further and added it.
Summary
Added a recipe for https://github.com/userver-framework/userver
Motivation
This PR adds a popular (2.8k stars) project 🐙 userver to the Conan Center.
Details
The recipe is extensively tested in the https://github.com/userver-framework/userver repository in the following combinations:
Unlike the previous PR #20712 this PR is much shorter, as the recipe now reuses the CMake installation.
🐙 userver heavily relies on CMake. It can not work without it, so the recipe does not attempt to be usable with other build systems for now (maybe later we will support other build systems in userver and in that case we'll update the recipe)
Unfortunately, one of the Python modules used by userver builds from sources. We failed to provide that dependency to the stage when users build Python venv with userver from Conan. Because of that the recipe has a
libpq-devsystem package requirement. We'd be happy to improve that place, but our last attempt took two weeks and failed.Tests
Tested locally with
conan create all/conanfile.py --version=2.13 --build=missing. Without--build=missingmany packages were not foundTests pass well, including Python based tests:
P.S.: Any recipe change from this PR will be synchronized to the userver-framework/userver to get extensive testing.