-
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
userver: add unspecified version #20712
base: master
Are you sure you want to change the base?
Conversation
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.
Waiting for #19971 in order to resolve version conflict. |
Retriggered again, linked PR has now been merged |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Waiting for #20091 |
@RubenRBS is there any way to build mogo-c-driver with_sasl=cyrus? |
@Jihadist for cci no, it's not currently possible to build configurations other than the defaul ones defined in the recipe - this is something we're currently looking into allowing, but for now checks should be made in the validate method to ensure that the expected options have been activated by the user. This means that no binaries will be made for that configuration, so please do provide local logs of your builds when testing them, as it makes reviewing and future maintenance way easier :) |
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.
Thanks a lot for your contribution @Jihadist - we really appreciate it.
This is a first review (Thanks for your patience while I got around to it!), and I have some comments and suggestions. My biggest doubt right now is about the need for a non default option on the dependency, which right now generates no binaries here, happy to discuss it further
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hi! Thanks for the ping, sorry that this one got lost among the rest of PRs: Note that opening and closing PRs makes Github Projects set the status to "Done", which we don't often check, so it's easier that the PR gets delayed if you do that :) if you ever need to rebuild, pressing the "Update branch" button usually works best! |
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.
Ok, after going again thru this, it looks mostly good, but I'll ask someone else from the team to check a few extra things tomororw, sorry for such a long delay, thanks a lot for your patience :)
default_options = { | ||
'fPIC': True, | ||
'lto': True, | ||
'with_jemalloc': False, # Disabled dy default due to jemalloc recipe does not support Conan v2 |
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.
Jemalloc is now available in Conan 2, feel free to make this True by default if upstream following https://github.com/userver-framework/userver/blob/13ca039791f9dbaa9da36e3e55e86451516dfa7f/CMakeLists.txt#L96 :)
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 know but I fixed this recipe so many tries that now I have no desire to do it again
Oh, now I understand why my other prs have been waiting review for months |
@AbrilRBS sorry for ping but looks like no one review this. Should I close it? |
Hi @xakod, the issue here is that the CLA has not yet been signed, thus we can not begind the process to get this into CCI. We'd need that all emails sign the CLA. If you commited under a different email, the best thing to do would be to squash all commits under your main account. How does that sound? |
@AbrilRBS you have been ignored this pr from April :) |
bump deps review fixes disable mongo, optional conan tests compilers_minimum_version fix pytest move scripts inside lib fix include dirs fix std for test package Update CMakeLists.txt option for testsuite Update conanfile.py Try to disable everything disable utest too Another attempt to fix and update Update config.yml Update CMakeLists.txt Fix testsuite test Update conanfile.py disable DWCAS Update conanfile.py another attempt to disable dwcas
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ❌Warning Conan Center will stop receiving updates for Conan 1.x packages soon - please see announcement. Failure in build 2 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. Conan v2 pipeline ✔️
All green in build 2 (
|
Hooks produced the following warnings for commit 0e4a828userver/cci.20240219@#778ffbba5410f4380729522e6d28ca26
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Specify library name and version: userver/cci.20231018
I understand recipe looks like a bit bloated and test package too, but both of them require a lot of thirdparty libraries and specific options so it's nontrivial task.