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

fix bugs in igntion-tools recipe #11309

Closed
wants to merge 27 commits into from

Conversation

ggulgulia
Copy link
Contributor

@ggulgulia ggulgulia commented Jun 21, 2022

Specify library name and version: ignition-tools/1.4.0*

Description

  • The current recipe had several issues

    • incorrect Findignition-tools1.cmake (:x:) generation instead of Findignition-tools.cmake (:heavy_check_mark: ) generation
    • incorrect library component name ignition-tools1 (:x:) instead of ignition-tools (:heavy_check_mark: )
    • missing core library component name
    • missing various cmake variables for
  • This PR fixes the issues


  • 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.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@ggulgulia
Copy link
Contributor Author

@uilianries in the error, it seems that ruby/3.1.0 and gmp/6.1.2 conan packages are downloaded but then build cannot find binaries. What am I missing here ?

use `CONFIG` : find_package(pkg-name CONFIG)
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

override default conan behavior specifying `package_folder/include` dir
@ggulgulia
Copy link
Contributor Author

@prince-chrismc can you please review once again ?

@conan-center-bot

This comment has been minimized.

@github-actions
Copy link
Contributor

Hooks produced the following warnings for commit ae68bd3
ignition-tools/1.4.0
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library './lib/libignition-tools-backward.so' links to system library 'm' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\lib\ignition-tools-backward.dll' links to system library 'dbghelp' but it is not in cpp_info.system_libs.

uilianries
uilianries previously approved these changes Jul 25, 2022
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.

Since the goal of the PR is to fix the generators.

I would be nice to have https://github.com/gazebosim/gz-tools/blob/20bcaf96bed27850b256fe3ac95c0aa1640143fe/CMakeLists.txt#L190

self.cpp_info.names["cmake_find_package"] = lib_name
self.cpp_info.names["cmake_find_package_multi"] = lib_name
self.cpp_info.set_property("cmake_file_name", "ignition-tools")
self.cpp_info.components["backward"].names["cmake_find_package"] = "backward"
Copy link
Contributor

@prince-chrismc prince-chrismc Jul 25, 2022

Choose a reason for hiding this comment

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

I am not sure whats the best option here but the targets are not correctly exporting from the upstream project... Does this match the projects intentions? I am not sure I can answer that...

https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html#exporting-targets calls out install(EXPORT...) and install(TARGETS ...) neither exist 😕

https://github.com/gazebosim/gz-tools/blob/ignition-tools_1.4.0/src/CMakeLists.txt#L29 main gets closer but it's still not doing something consumers would rely on AFAIK.

recipes/ignition-tools/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

@github-actions
Copy link
Contributor

Hooks produced the following warnings for commit 3b54cde
ignition-tools/1.4.0
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\lib\ignition-tools-backward.dll' links to system library 'dbghelp' but it is not in cpp_info.system_libs.

@conan-center-bot
Copy link
Collaborator

All green in build 31 (1ff89f45afad944b449d5447eb95adec40f364b1):

  • ignition-tools/1.4.0@:
    All packages built successfully! (All logs)

    🔸 Informative: This recipe is not ready for Conan v2

    We have started the migration process to Conan v2 and exporting recipes successfully will be required in the future.
    This is just an informative note to gain awareness about the process, no need to take any action. The plan is to enforce smaller steps that are easier to fix and, eventually, this conan export step will work.
    See the recipe migration guide to know more about the changes required.

    ERROR: Error loading conanfile at '/home/conan/w/prod_cci_PR-11309/recipes/ignition-tools/all/conanfile.py': Unable to load conanfile in /home/conan/w/prod_cci_PR-11309/recipes/ignition-tools/all/conanfile.py
      File "<frozen importlib._bootstrap_external>", line 728, in exec_module
      File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
      File "/home/conan/w/prod_cci_PR-11309/recipes/ignition-tools/all/conanfile.py", line 2, in <module>
        from conans import CMake, ConanFile, tools
    ImportError: cannot import name 'CMake' from 'conans' (/opt/pyenv/versions/3.7.13/lib/python3.7/site-packages/conans/__init__.py)
    

@github-actions
Copy link
Contributor

Hooks produced the following warnings for commit 1ff89f4
ignition-tools/1.4.0
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\lib\ignition-tools-backward.dll' links to system library 'dbghelp' but it is not in cpp_info.system_libs.

@ggulgulia
Copy link
Contributor Author

Since the goal of the PR is to fix the generators.

I would be nice to have https://github.com/gazebosim/gz-tools/blob/20bcaf96bed27850b256fe3ac95c0aa1640143fe/CMakeLists.txt#L190

@prince-chrismc does this mean I should install the package config file ignition-tools.pc ? The content of this file from the library installed version has only 4 lines:

# This packages provides no headers or libraries, so those fields are omitted
Name: Ignition tools
Description: A set of tools classes for robot applications
Version: 1.4.0

So I'm not sure if installing this is really useful. What do you think ?

@ggulgulia
Copy link
Contributor Author

any update on my queries ?

@jgsogo
Copy link
Contributor

jgsogo commented Aug 5, 2022

The whole purpose of Conan is to generate those .pc files (or FindXXX.cmake, XXX-config.cmake) using its own generators, to take into account all the dependencies and so on. Generated .pc file by Conan should contain more accurate information (https://github.com/conan-io/conan-center-index/blob/master/docs/faqs.md#why-are-cmake-findconfig-files-and-pkg-config-files-not-packaged).

url: "https://github.com/ignitionrobotics/ign-tools/archive/refs/tags/ignition-tools_1.4.0.tar.gz"
sha256: "fa3f7984ebb8f412133ea93368395adce426ae36c715a9f94f9509af7dac3b03"
url: "https://github.com/gazebosim/gz-tools/archive/refs/tags/ignition-tools_1.4.0.tar.gz"
sha256: "8ceebefcd1977dc2e26beede347a9c06d851b89ec0fd7d5c86126f43a49ac178"
Copy link
Contributor

Choose a reason for hiding this comment

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

@jcar87 We have another check sum change for you to label 👏

@prince-chrismc
Copy link
Contributor

@prince-chrismc does this mean I should install the package config file ignition-tools.pc ? The content of this file from the library installed version has only 4 lines:

You can set the properties

self.cpp_info.components["wayland-server"].set_property("pkg_config_name", "wayland-server")

self.cpp_info.components["wayland-server"].set_property("component_version", self.version)

@stale
Copy link

stale bot commented Sep 30, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 30, 2022
@stale
Copy link

stale bot commented Nov 22, 2022

This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions.

@stale stale bot closed this Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants