-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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.
Hooks produced the following warnings for commit d061d1duserver/cci.20240219@#c5a0d7390d2a66f7ee41123a52a3f25c
|
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit a50b380userver/cci.20240219@#c5a0d7390d2a66f7ee41123a52a3f25c
|
disable DWCAS
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit d50f904userver/cci.20240219@#c16fae17b9ce7248dbdcd46316f3396a
|
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 07f23e8userver/cci.20240219@#a45b62ac9dbfc1add5f719aff4dafc2c
|
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit d6abf62userver/cci.20240219@#078e749cd0ec6cf84f43f0df37c307e7
|
Conan v1 pipeline ❌Failure in build 33 (
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 33 (
|
Hooks produced the following warnings for commit 059422cuserver/cci.20240219@#778ffbba5410f4380729522e6d28ca26
|
I disabled all additional modules now. When (or if ) pr is merged, I'll enable modules one by one. Some can work in this recipe without changes, some not. Everyone can try to build recipe locally for another platforms with additional modules, I left tests for that purposes. |
@RubenRBS can you review this? |
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? |
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.