-
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
Compile as C #23894
Compile as C #23894
Conversation
a00427d
to
5a51ce7
Compare
This comment has been minimized.
This comment has been minimized.
5a51ce7
to
786f926
Compare
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.
Hi @datalogics-staylor , thanks for your contribution. |
Hi @jcar87. That's a very good point. I looked around a bit for recipes that do both C and C++ and didn't find many, so I chose to go this route so that it doesn't build packages that aren't required. It would be a pretty minor change to just build both, so I'd be happy to go that route if that seems to make more sense. Conan and CCI are relatively new to me, so I'm still learning common usages. Thanks for the feedback! |
332dc26
to
f5ed5d6
Compare
This comment has been minimized.
This comment has been minimized.
@datalogics-staylor Your change looks correct in terms of CMake, but still need to update the Conanfile recipe in order to list the new library too. Could please update the pacakge_info as well? |
This comment has been minimized.
This comment has been minimized.
@@ -102,7 +102,7 @@ def package(self): | |||
|
|||
def package_info(self): | |||
self.cpp_info.libs = [ | |||
"qrcodegen" if Version(self.version) < "1.7.0" else "qrcodegencpp" | |||
"qrcodegen" if Version(self.version) < "1.7.0" else "qrcodegencpp", "qrcodegenc" | |||
] |
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.
] | |
] | |
self.cpp_info.components["qrcodegencpp"].set_property("cmake_target_name", "qr-code-generator::qrcodegencpp") | |
self.cpp_info.components["qrcodegencpp"].libs = ["qrcodegen" if Version(self.version) < "1.7.0" else "qrcodegencpp"] | |
self.cpp_info.components["qrcodegenc"].set_property("cmake_target_name", "qr-code-generator::qrcodegenc") | |
self.cpp_info.components["qrcodegenc"].libs = ["qrcodegenc"] |
I would suggest adding separated components too, so you can link to C++ or C only, by using cmake targets.
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.
Ah, that makes sense. I'm pretty new to both Conan and CMake, so I appreciate the feedback!
d44ea9f
to
b94fc89
Compare
This comment has been minimized.
This comment has been minimized.
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.
Looks good. Both C++ and C libraries are available as separated targets.
b94fc89
to
79ba58e
Compare
Conan v1 pipeline ✔️All green in build 12 (
Conan v2 pipeline ✔️
All green in build 12 (
|
@franramirez688 We're just waiting for one more review - the last approval was 2 weeks ago - can we get some eyes on this one for us? Thanks! |
Specify library name and version: qr-code-generator/1.8.0