Skip to content
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 GMTL - Generic Math Template Library recipe #13674

Merged
merged 25 commits into from
Nov 3, 2022

Conversation

psyinf
Copy link
Contributor

@psyinf psyinf commented Oct 21, 2022

Specify library name and version: gmtl/0.7.1

The library is originally hosted at an sourceforge svn repository. It's however no longer maintained by the original authors. I've added some minor corrections and plan on further developing the library and make it available to a broader audience by adding it to conan.
Despite its age and relatively low maintenance in the last years I've used it over the last 10-15 years in various hobby- and professional projects. This is my first attempt to contribute to conan and I'd be happy to get some guidance.


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the conan-center hook activated.

@CLAassistant
Copy link

CLAassistant commented Oct 21, 2022

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@psyinf psyinf closed this Oct 21, 2022
@psyinf psyinf reopened this Oct 21, 2022
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

recipes/gmtl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/gmtl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/gmtl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/gmtl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/gmtl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/gmtl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/gmtl/all/test_package/CMakeLists.txt Outdated Show resolved Hide resolved
recipes/gmtl/all/test_v1_package/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@jwillikers jwillikers left a comment

Choose a reason for hiding this comment

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

Oops, wrong button. Still need some changes. Did you register for access on CCI @psyinf ?

Co-authored-by: Jordan Williams <jordan@jwillikers.com>
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

recipes/gmtl/all/conanfile.py Outdated Show resolved Hide resolved
Co-authored-by: Jordan Williams <jordan@jwillikers.com>
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

psyinf and others added 10 commits November 1, 2022 17:59
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
@psyinf
Copy link
Contributor Author

psyinf commented Nov 1, 2022

Overall its super good!

Just a few point

  • clean up from templates
  • expose official targets (there a question about your intentions for how this fork is used in there)

I left all the suggestions, I hopefully did break anything 🤞

Thank you very much for reviewing. Highly appreciated!

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

psyinf and others added 2 commits November 1, 2022 18:05
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
@conan-center-bot

This comment has been minimized.

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline

All green in build 28 (ceb2ca9cffd5bc8f5684fc69737fad009106b307):

  • psyinf-gmtl/0.7.1@:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit 3adeb9c into conan-io:master Nov 3, 2022
Comment on lines +8 to +11
find_package(psyinf-gmtl REQUIRED CONFIG)

add_executable(${PROJECT_NAME} ../test_package/test_package.cpp)
target_link_libraries(${PROJECT_NAME} PRIVATE psyinf-gmtl::psyinf-gmtl)
Copy link
Contributor

@SpaceIm SpaceIm Nov 3, 2022

Choose a reason for hiding this comment

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

names in CMakeDeps & cmake_find_package[_multi] are not consistent

Copy link
Contributor Author

@psyinf psyinf Nov 3, 2022

Choose a reason for hiding this comment

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

I just tried to use the recipe with cmake multi environment. Do we need to set
self.cpp_info.names["cmake_find_package"] = "gmtl"
self.cpp_info.names["cmake_find_package_multi"] = "gmtl"
to use it as a drop-in replacement?

Also, should'nt I use
self.cpp_info.set_property("cmake_target_name", "gmtl::gmtl") ?

import os


required_conan_version = ">=1.52.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

1.50.0 is enough

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.

7 participants