-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Added conan packaging configuration. #643
Conversation
Thanks for contributing, |
Thank you, for accepting my packaging things 😄 |
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.
This review is for @dimi309 . The package is mostly good, but there are a few improvements, specially due to latest conan 0.25 release improvements, that might simplify things. Great work!
url="https://github.com/g-truc/glm" | ||
description="OpenGL Mathematics (GLM)" | ||
license = "https://github.com/g-truc/glm/blob/manual/copying.txt" | ||
exports = ["FindGLM.cmake", "lib_licenses/*", os.sep.join([".", "..", "..", "*"])] |
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.
You might take advantage of the exports_sources
functionality to store the sources and the .cmake file. I would only keep the license as exports
. It is much more efficient to use exports_sources
for source code.
|
||
def build(self): | ||
self.output.warn("No compilation necessary for GLM") | ||
self.output.warn(os.sep.join([".", "..", "..", "*"])) |
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.
What is the purpose of the second warning? I would remove it, and maybe change the first one for a output.info()
message, as it is not something that should concern the user.
from conans import ConanFile, CMake | ||
import os | ||
|
||
channel = os.getenv("CONAN_CHANNEL", "testing") |
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.
From conan 0.25 you can simplify this test_package recipe, removing the user/channel, and also the requires
, so it is not necessary to change them or change versions when testing a different one. Then, use conan create user/channel
better.
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 for the feedback! I have implemented the changes and introduced a pull request: #662
As discussed. I have already published a package of the master branch. I can also add the packaging config to the latest version, but let's take it step by step.