-
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
add co/2.0.2 #7510
add co/2.0.2 #7510
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.
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.
Co-authored-by: Sparik Hayrapetyan <sparikhayrapetyan@gmail.com>
Co-authored-by: Sparik Hayrapetyan <sparikhayrapetyan@gmail.com>
Co-authored-by: Sparik Hayrapetyan <sparikhayrapetyan@gmail.com>
Co-authored-by: Sparik Hayrapetyan <sparikhayrapetyan@gmail.com>
Co-authored-by: Sparik Hayrapetyan <sparikhayrapetyan@gmail.com>
Co-authored-by: Sparik Hayrapetyan <sparikhayrapetyan@gmail.com>
Co-authored-by: Sparik Hayrapetyan <sparikhayrapetyan@gmail.com>
Co-authored-by: Sparik Hayrapetyan <sparikhayrapetyan@gmail.com>
Co-authored-by: Sparik Hayrapetyan <sparikhayrapetyan@gmail.com>
Co-authored-by: Sparik Hayrapetyan <sparikhayrapetyan@gmail.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
All blue! Can it be merged into one piece now? Thanks for your help! |
Absolutely they can! https://docs.conan.io/en/latest/using_packages/conanfile_txt.html#overriding-requirements |
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.
And one more questions
Does this impact the features?
https://github.com/idealvin/co/blob/7be5367623a85ca8a0a4e2095000d7b704cc9f89/src/CMakeLists.txt#L94
How can a consumer know if this was enabled?
|
||
|
||
class CoConan(ConanFile): | ||
name = "co" |
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.
Are we fine with
co
? Perhaps it 's safer to namespace it?idealvin-co
?
I think it's a great idea, people who aren't familiar with the project we get confused
recipes/co/all/conanfile.py
Outdated
|
||
def package_info(self): | ||
self.cpp_info.libs = ["co"] | ||
self.cpp_info.names["cmake_find_package"] = "CO" |
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.
Why is this upper case?
Look at at the code it should be lowercase https://github.com/idealvin/co/blob/7be5367623a85ca8a0a4e2095000d7b704cc9f89/src/CMakeLists.txt#L112?
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.
Fixed. Thanks.
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.
Are we fine with co? Perhaps it 's safer to namespace it? idealvin-co
Unfortunately, I chose the name co
at the beginning, and it’s too late when I want to rename it.
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 didn't find other C/C++ projects using same name. As your project has 2k2 stars on GIthub, it's popular, so I think is okay accepting it as co
only.
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 didn't find other C/C++ projects using same name. As your project has 2k2 stars on GIthub, it's popular, so I think is okay accepting it as
co
only.
Thanks!
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
This comment has been minimized.
This comment has been minimized.
This is described in the documents, should be ok. |
This comment has been minimized.
This comment has been minimized.
Then libstacktrace needs to be an option. Otherwise Conan wont be able to track it |
This is a little different, check_include_files(backtrace.h HAS_BACKTRACE)
if(HAS_BACKTRACE)
target_compile_definitions(co PRIVATE HAS_BACKTRACE_H)
target_link_libraries(co PUBLIC backtrace)
endif() It seems that it is not necessary for conan to track it. |
This comment has been minimized.
This comment has been minimized.
Okay, there's a key feature about Conan that makes it so powerful. Now what the hell does that mean? TL;DR it is related to the ABI of the binary. Well mostly. That means anything that affect the binary, add/removes API, compiler settings, platform, etc need to be tracked by conan. I'll give you an example where not adding the option may ruin someone's day. We have two systems
When we build our app we are going to compile and link against libbacktrace. Unit tests will pass. Everything is ready. The opposite is less annoying but easily avoided... let pretend we forgot to install the extra library. if the option |
@prince-chrismc I think there is no problem at present. However, I'll consider make it as a feature in the next version of co. |
Co-authored-by: Uilian Ries <uilianries@gmail.com>
All green in build 29 (
|
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.
LGTM
Specify library name and version: lib/1.0
This is also a good place to share with all of us why you are submitting this PR (specially if it is a new addition to ConanCenter): is it a dependency of other libraries you want to package? Are you the author of the library? Thanks!
conan-center hook activated.