Skip to content

Conversation

@robinchrist
Copy link

@robinchrist robinchrist commented Jul 14, 2025

The conan file used several old-style conan practices and has some issues.

The following changes were made:

  1. Fix issue with CDT_USE_AS_COMPILED_LIBRARY: The conan file sets tc.cache_variables[CDT_USE_AS_COMPILED_LIBRARY], however this leads to issues because this define is not included if CDT is consumed as a dependency. It must be set in package_info cpp_info.defines.append
  2. Automate boost as dependency - if detected C++ version is below 11, automatically include boost
  3. Make use of standard conan conventions for shared, header_only and fPIC (This style is considered best practice and enforced for conan center recipes)

I'm definitely not a conan expert, but do have some experience with it. While this conan file may not be perfect (conan experts to the rescue), it is definitely a step forward.

@artem-ogre
Copy link
Owner

Thank you @robinchrist. The changes look like they make sense. I will try to look at it this week.

Copy link
Owner

@artem-ogre artem-ogre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good, thank you for the effort.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add required_conan_version?

Copy link
Author

@robinchrist robinchrist Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a good idea.

@artem-ogre
Copy link
Owner

Please rebase on top of the master.

The conan file sets tc.cache_variables[CDT_USE_AS_COMPILED_LIBRARY], however this leads
to issues because this define is not included if CDT is consumed as a dependency
Additionally, the conan file used several old-style conan practices
…ge and native CMake package

The native CMake package uses CDT::CDT
The autogenerated conan CMake library alias is cdt::cdt
Force the library alias to CDT::CDT for backwards compat and consistency
@artem-ogre
Copy link
Owner

artem-ogre commented Jul 23, 2025

Thanks for the changes! I will look at them when I get some spare time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants