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

Adding imagl recipe #4225

Merged
merged 4 commits into from
Jan 19, 2021
Merged

Adding imagl recipe #4225

merged 4 commits into from
Jan 19, 2021

Conversation

Woazim
Copy link
Contributor

@Woazim Woazim commented Jan 12, 2021

Specify library name and version: imagl/0.1.0

  • 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 Jan 12, 2021

CLA assistant check
All committers have signed the CLA.

@conan-center-bot
Copy link
Collaborator

Some configurations of 'imagl/0.1.0' failed in build 1 (e9078ddecfbfe8fc718f79e5fd426d6b8282501b):

@Woazim Woazim closed this Jan 12, 2021
@Woazim Woazim reopened this Jan 12, 2021
@conan-center-bot
Copy link
Collaborator

All green in build 3 (8376ef595f32b7f3ffa341a57729bfdc74ecd1b5)! 😊

cmake.build()
cmake.install()

# Explicit way:
Copy link
Contributor

Choose a reason for hiding this comment

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

pls remove dead code

Comment on lines 11 to 12
options = {"shared": [True, False]}
default_options = {"shared": False}
Copy link
Contributor

Choose a reason for hiding this comment

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

You are missing the fPIC option

Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

Thank you for contributing this exciting project! 🏆

I strongly recommend you take a lot at other recipes that are added already, there's a few details missing this is a good example

You most likely need the "cmake_wrapper" like this https://github.com/conan-io/conan-center-index/blob/7497b701cc07901ee3f69736c2e1b1802aa8df79/recipes/sentry-native/all/CMakeLists.txt

Comment on lines 21 to 23
tools.replace_in_file(self.name + "-v" + self.version + "/CMakeLists.txt", "# conan insert",
'''include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake)
conan_basic_setup()''')
Copy link
Contributor

Choose a reason for hiding this comment

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

patching sources needs to be done in the build method


def build(self):
cmake = CMake(self)
cmake.verbose = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cmake.verbose = True

cmake.verbose = True
cmake.configure(source_folder=self.name + "-v" + self.version, defs={"STATIC_LIB": "OFF" if self.options.shared else "ON"})
cmake.build()
cmake.install()
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be done in the package method

if not(self.options.shared): static_suffix = "s"
self.cpp_info.libs = ["imaGL{debug}{static}".format(debug = debug_suffix, static = static_suffix)]

def configure(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be higher up in the order see this https://github.com/conan-io/conan-center-index/blob/ddd8c3c7499678583061a570b03cac615d67b157/docs/reviewing.md#order-of-methods-and-attributes

For managing the c++ version requirements... I'd suggest you copy this snippet
conan-io/conan#8002

Comment on lines +10 to +12
if(STATIC_LIB)
add_compile_definitions(IMAGL_STATIC)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

this most likely needs to be in the package info

Comment on lines 1 to 22
import os

from conans import ConanFile, CMake, tools


class ImaglTestConan(ConanFile):
settings = "os", "compiler", "build_type", "arch"
options = {"shared": [True, False]}
default_options = {"shared": False}
generators = "cmake"

def build(self):
cmake = CMake(self)
# Current dir is "test_package/build/<build_id>" and CMakeLists.txt is
# in "test_package"
cmake.configure(defs={"STATIC_LIB": "OFF" if self.options.shared else "ON"})
cmake.build()

def test(self):
if not tools.cross_building(self):
os.chdir("bin")
self.run(".%sexample" % os.sep)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow our usual test package format

Suggested change
import os
from conans import ConanFile, CMake, tools
class ImaglTestConan(ConanFile):
settings = "os", "compiler", "build_type", "arch"
options = {"shared": [True, False]}
default_options = {"shared": False}
generators = "cmake"
def build(self):
cmake = CMake(self)
# Current dir is "test_package/build/<build_id>" and CMakeLists.txt is
# in "test_package"
cmake.configure(defs={"STATIC_LIB": "OFF" if self.options.shared else "ON"})
cmake.build()
def test(self):
if not tools.cross_building(self):
os.chdir("bin")
self.run(".%sexample" % os.sep)
import os
from conans import ConanFile, CMake, tools
class TestPackageConan(ConanFile):
settings = "os", "compiler", "build_type", "arch"
generators = "cmake", "cmake_find_package_multi"
def build(self):
cmake = CMake(self)
cmake.configure()
cmake.build()
def test(self):
if not tools.cross_building(self.settings):
bin_path = os.path.join("bin", "test_package")
self.run(bin_path, run_environment=True)

Signed-off-by: Uilian Ries <uilianries@gmail.com>
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
Copy link
Collaborator

All green in build 4 (e12e215f5a0d27f061979859cd1f7ae62d150f3f)! 😊

  • imagl/0.1.0: Generated 12 packages (+ 1 invalid config from build()). All logs here

@Woazim
Copy link
Contributor Author

Woazim commented Jan 15, 2021

Thanks everyone, I went away for a few days and when I come back I see your great reviews! I merged the @uilianries pull request which includes all of your remarks. I still have a lot of things to learn to master conan. 😉

Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

It looks good, it might be worth losing the compiler restriction so have it available on CCI

def _compilers_minimum_version(self):
return {
"gcc": "9",
"Visual Studio": "16.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Visual Studio": "16.5",
"Visual Studio": "16",

https://c3i.jfrog.io/c3i/misc/logs/pr/4225/4/imagl/0.1.0/summary.json

{
        "reference": "imagl/0.1.0",
        "packageID": "INVALID",
        "settings": {
            "os": "Windows",
            "os_build": "Windows",
            "arch": "x86_64",
            "arch_build": "x86_64",
            "compiler": "Visual Studio",
            "compiler.version": "16",
            "build_type": "Debug",
            "compiler.runtime": "MDd"
        },
        "options": {
            "imagl:shared": "True"
        },
        "status": "DUPLICATED",
        "profile": null,
        "build": null,
        "test": null
    },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible for you to commit this simple change, since the pull request is now approved? Or do I have to create a new pull request / issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to make a new PR, this way CCI can generate all new packages

@SpaceIm
Copy link
Contributor

SpaceIm commented Jul 12, 2021

Hi, the CMakeLists of this library calls conan install under the hood: https://gitlab-lepuy.iut-clermont.uca.fr/opengl/imagl/-/blob/e420cccc1011b10e2e4e85351f7084d8feb5c641/CMakeLists.txt#L55-63
And this conan install uses another conanfile with version range: https://gitlab-lepuy.iut-clermont.uca.fr/opengl/imagl/-/blob/e420cccc1011b10e2e4e85351f7084d8feb5c641/conanfile.in
It's not allowed and must be removed in the recipe (patch).

@Woazim
Copy link
Contributor Author

Woazim commented Jul 12, 2021

Hi,

What is wrong: calling conan install or using version ranges?
And, by the way for my information, why it is wrong?

@SpaceIm
Copy link
Contributor

SpaceIm commented Jul 12, 2021

Both are wrong in CCI:

  1. conan install in CMakeLists:
    • this is an unmanaged (and embedded) conan install. conan install is already called before, with potentially different versions of dependencies (when I cross-build current recipe with 2 profiles, I ends up with 2 package id of each dependency because you call conan install with 1 profile, I don't even know which one is injected in the build).
    • this conan install in CMakeLists is called at build() time, and we don't allow downloads at build time. There are 3 allowed download steps, completly managed by CCI recipe: in source(), requirements() and build_requirements().
  2. version range: not allowed in CCI

And more generally, it's not recommended to have intrusive integration of a package manager in its CMakeLists. It makes it hard for others package managers to add the library.
I advice to either:

  • use transparent integration with cmake_find_package generator.
  • add a CMake option to use either intrusive conan integration, or regular find_package() calls.

@SpaceIm
Copy link
Contributor

SpaceIm commented Jul 12, 2021

I've opened #6279

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