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 yarp recipe #18234

Merged
merged 16 commits into from
Jun 13, 2022
Merged

Add yarp recipe #18234

merged 16 commits into from
Jun 13, 2022

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Mar 4, 2022

YARP (Yet Another Robot Platform, https://en.wikipedia.org/wiki/YARP) is a C++ library with Python bindings used in robotics. It contains functionality related to inter-process communication (so-called YARP ports) and abstraction of hardware devices (so-called YARP devices). It was initially developed in early 2000s as a collaboration between University of Genoa and MIT, and is extensively used in the software architecture of the iCub Humanoid robot (see https://www.frontiersin.org/articles/10.3389/frobt.2016.00024/full for more details).

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/yarp) and found some lint.

Here's what I've got...

For recipes/yarp:

  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [60]

@traversaro traversaro changed the title Add yarp recipe [WIP] Add yarp recipe Mar 4, 2022
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/yarp) and found it was in an excellent condition.

@traversaro traversaro changed the title [WIP] Add yarp recipe Add yarp recipe Mar 5, 2022
@traversaro
Copy link
Contributor Author

Linux/macOS builds are failing with:

["  ERROR (yarp,lib/yarp/yarp_portaudioRecorder.so): .. but ['portaudio'] not in reqs/run, (i.e. it is overlinking) (likely) or a missing dependency (less likely)", "  ERROR (yarp,lib/yarp/yarp_portaudioPlayer.so): .. but ['portaudio'] not in reqs/run, (i.e. it is overlinking) (likely) or a missing dependency (less likely)", "  ERROR (yarp,bin/yarpmanager-console): .. but ['libedit'] not in reqs/run, (i.e. it is overlinking) (likely) or a missing dependency (less likely)", "  ERROR (yarp,lib/libYARP_companion.so.3.6.0): .. but ['libedit'] not in reqs/run, (i.e. it is overlinking) (likely) or a missing dependency (less likely)", "  ERROR (yarp,lib/yarp/yarp_portaudio.so): .. but ['portaudio'] not in reqs/run, (i.e. it is overlinking) (likely) or a missing dependency (less likely)", "  ERROR (yarp,lib/libYARP_os.so.3.6.0): .. but ['libedit'] not in reqs/run, (i.e. it is overlinking) (likely) or a missing dependency (less likely)"]

even if both portaudio and libedit have correct run_exports annotations and the latest version of their package is used.

@traversaro
Copy link
Contributor Author

The PR is ready for review @conda-forge/help-c-cpp @conda-forge/help-python-c .

@traversaro
Copy link
Contributor Author

Build fails with:

conda_build.exceptions.OverLinkingError: overlinking check failed 
["  ERROR (yarp,lib/libYARP_sig.3.6.0.dylib): .. but ['soxr'] not in reqs/run, (i.e. it is overlinking) (likely) or a missing dependency (less likely)"]

@traversaro
Copy link
Contributor Author

Build fails with:

conda_build.exceptions.OverLinkingError: overlinking check failed 
["  ERROR (yarp,lib/libYARP_sig.3.6.0.dylib): .. but ['soxr'] not in reqs/run, (i.e. it is overlinking) (likely) or a missing dependency (less likely)"]

This will be fixed by conda-forge/soxr-feedstock#1 .

@carterbox
Copy link
Member

@conda-forge/help-c-cpp please review

Copy link
Member

@carterbox carterbox left a comment

Choose a reason for hiding this comment

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

Please split the Python API for this package into a separate output or convert the python run requirement into a run constraint. The python requirement as it is presently will cause downstream CXX packages to inherit a python run requirement even if they do not use python.

recipes/yarp/meta.yaml Outdated Show resolved Hide resolved
@traversaro
Copy link
Contributor Author

traversaro commented Apr 26, 2022

Please split the Python API for this package into a separate output or convert the python run requirement into a run constraint. The python requirement as it is presently will cause downstream CXX packages to inherit a python run requirement even if they do not use python.

Since when this was a conda-forge constraint? I mantain several mixed C++/Python packages that have a single output (mainly for reduce recipe complexity, and also because I never fully understood how run_exports works with multiple output recipe to be completly honest) and this was never a problem until now, and I do not see anything in official conda-forge CFEP or documentation (see for example https://github.com/conda-forge/pinocchio-feedstock/blob/5405f41b064b223e64d2594ec694cfdc37f61723/recipe/meta.yaml#L43). Alternatively, do you have a good example of a C++/Python package with C++ and Python parts on multiple outputs that handles correctly run_exports or of a C++/Python package that has a Python run_constraint? Thanks!

@traversaro
Copy link
Contributor Author

By the way, I am afraid that without a pin on python version also the current version with python in the run dependencies is wrong.

@traversaro
Copy link
Contributor Author

By the way, I am afraid that without a pin on python version also the current version with python in the run dependencies is wrong.

Given https://github.com/conda-forge/python-feedstock/blob/237b7631fb414e543d36b744ed2120661be3673e/recipe/meta.yaml#L135 I guess if the correct fix (unless we separate the recipe in multiple outputs) is to just drop python from the run dependencies?

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/yarp) and found some lint.

Here's what I've got...

For recipes/yarp:

  • If python is a host requirement, it should be a run requirement.

@traversaro
Copy link
Contributor Author

By the way, I am afraid that without a pin on python version also the current version with python in the run dependencies is wrong.

Given https://github.com/conda-forge/python-feedstock/blob/237b7631fb414e543d36b744ed2120661be3673e/recipe/meta.yaml#L135 I guess if the correct fix (unless we separate the recipe in multiple outputs) is to just drop python from the run dependencies?

I did that in 12e09ac, but I just realized why I added it in the first place: due to the linter error in #18234 (comment) .

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/yarp) and found it was in an excellent condition.

@traversaro
Copy link
Contributor Author

By the way, I am afraid that without a pin on python version also the current version with python in the run dependencies is wrong.

Given https://github.com/conda-forge/python-feedstock/blob/237b7631fb414e543d36b744ed2120661be3673e/recipe/meta.yaml#L135 I guess if the correct fix (unless we separate the recipe in multiple outputs) is to just drop python from the run dependencies?

I did that in 12e09ac, but I just realized why I added it in the first place: due to the linter error in #18234 (comment) .

I restored python as run dependency in e44a1c1 to fix the linter error.

@traversaro
Copy link
Contributor Author

While working on #19049 (and inspired by https://github.com/conda-forge/bullet-feedstock) I understood that a nice way to have a recipe with split output between C++ library and Python bindings is to have Python bindings that can be build on their own that find via find_package the main library. As YARP build system permits that, I could adapt easily this recipe to have separate packages for the python bindings.

@traversaro
Copy link
Contributor Author

Linux/macOS build are successful.

Windows builds are failing with:

The reported errors are:
- Encountered problems while solving:
-   - package yarp-cxx-3.7.0-hf18cf19_0 requires qt >=5.12.9,<5.13.0a0, but none of the providers can be installed
- 
 - python_abi 3.9.* 2_cp39
 - wheel 0.37.1.* pyhd8ed1ab_0
 - setuptools 62.3.2.* py39hcbf5309_0
 - pip 22.1.2.* pyhd8ed1ab_0
 - yarp-cxx 3.7.0.* hf18cf19_0

with channels:
 - local
 - conda-forge
 - conda-forge

The reported errors are:
- Encountered problems while solving:
-   - package yarp-cxx-3.7.0-hf18cf19_0 requires qt >=5.12.9,<5.13.0a0, but none of the providers can be installed
- 

Not sure what the problem is. As the qt --> qt-main transiction is imminenent, I will try to switch to qt-main to see if the situation improves.

@traversaro
Copy link
Contributor Author

Not sure what the problem is. As the qt --> qt-main transiction is imminenent, I will try to switch to qt-main to see if the situation improves.

As libopencv still depends on qt and is not available for qt-main, this does not work.

@traversaro
Copy link
Contributor Author

The run_exports directive should be on the combined package yarp or only in the yarp-cxx package? For the time being I only put them in yarp-cxx to avoid the duplication of the max_pin information.

The run_export goes in yarp-cxx because that has the compiled-language API. Because yarp (combined) depends on yarp-cxx, if someone puts yarp in the host section, yarp-cxx will also be added and the run_export will be applied.

Oh, great, I did not know that the run_exports directive is transitive. Initially I tought this was not the case as otherwise downstream packages that depended on yarp-python gotan ABI-based constraint on yarp-cxx, even if they are not actually compiled packages, but then I realized that if you have a python package that depends on yarp-python, you will add it to run dependencies, not host one.

@traversaro traversaro closed this Jun 13, 2022
@traversaro traversaro reopened this Jun 13, 2022
@traversaro
Copy link
Contributor Author

Thanks, I addressed or replied to all the observations. The PR is ready for another round of review/reply to comments. @conda-forge/help-c-cpp

recipes/yarp/meta.yaml Show resolved Hide resolved
recipes/yarp/meta.yaml Show resolved Hide resolved
recipes/yarp/meta.yaml Show resolved Hide resolved
recipes/yarp/meta.yaml Show resolved Hide resolved
@traversaro
Copy link
Contributor Author

@carterbox Thanks for the suggested modifications, I added them.

Copy link
Member

@carterbox carterbox left a comment

Choose a reason for hiding this comment

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

Assuming the builds succeed, I approve this feedstock. I'll check back in a few hours.

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

Successfully merging this pull request may close these issues.

3 participants