conda-forge / dartsim-feedstock

A conda-smithy repository for dartsim.
BSD 3-Clause "New" or "Revised" License
1 stars 6 forks source link

Add GUI component #77

Closed jslee02 closed 4 months ago

jslee02 commented 4 months ago

Checklist

Changes:

conda-forge-webservices[bot] commented 4 months ago

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 (recipe) and found it was in an excellent condition.

jslee02 commented 4 months ago

openscenegraph is not available on linux_aarch64 and linux_ppc64le, but I don't know how to disable it for them.

traversaro commented 4 months ago

The valid preprocessor selectors are documented in https://docs.conda.io/projects/conda-build/en/latest/resources/define-metadata.html#preprocessing-selectors, I guess linux-64 should be linux64.

traversaro commented 4 months ago

Removed run: as it seems for python packages, according to https://conda-forge.org/docs/maintainer/adding_pkgs/#run

That is indeed true, but as dartsim is mainly a C++ library that uses Eigen in its public headers, removing eigen from the run dependencies will mean that find_package(DART) would fail out of the box |(either directly or during compilation, not sure), unless the users manually install eigen.

jslee02 commented 4 months ago

Removed run: as it seems for python packages, according to https://conda-forge.org/docs/maintainer/adding_pkgs/#run

That is indeed true, but as dartsim is mainly a C++ library that uses Eigen in its public headers, removing eigen from the run dependencies will mean that find_package(DART) would fail out of the box |(either directly or during compilation, not sure), unless the users manually install eigen.

Dose this mean all the transitive deps of a C++ library should be listed in run:? What about other deps like tinyxml2 that is also exposed in the public headers of DART

jslee02 commented 4 months ago

I've added back the runtime dependencies for now until I figure out how they work clearly, as it's already working, so it's better than breaking it.

traversaro commented 4 months ago

Removed run: as it seems for python packages, according to https://conda-forge.org/docs/maintainer/adding_pkgs/#run

That is indeed true, but as dartsim is mainly a C++ library that uses Eigen in its public headers, removing eigen from the run dependencies will mean that find_package(DART) would fail out of the box |(either directly or during compilation, not sure), unless the users manually install eigen.

Dose this mean all the transitive deps of a C++ library should be listed in run:? What about other deps like tinyxml2 that is also exposed in the public headers of DART

That is indeed a point that is not super clear, and for which I do not think there is a lot of consensus among conda-forge mantainers. However, if the library that you are exposing in the public headers is a compiled one (for example tinyxml2, as opposed to a header-only like eigen), the run dependency is already inserted automatically as tinyxml2 is required to run any program that uses dart's shared library, and so find_package(DART) will work fine as a byproduct.

jslee02 commented 4 months ago

Alright, it sounds like there's a lot to learn, and it will take time to establish good practices.

By the way, I needed to add boost as a transitive dependency when adding pagmo in https://github.com/conda-forge/dartsim-feedstock/pull/78, which I guess is one of the cases you were worried about.

jslee02 commented 4 months ago

Yay, I confirmed that libdart-gui-osg.so is installed:

https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=969507&view=logs&j=656edd35-690f-5c53-9ba3-09c10d0bea97&t=986b1512-c876-5f92-0d81-ba851554a0a3&l=1784

traversaro commented 4 months ago

By the way, I needed to add boost as a transitive dependency when adding pagmo in #78, which I guess is one of the cases you were worried about.

Yes, that is the case. I know that some packages also have a <pkg>-devel or <pkg>-dev output to handle that, but it is not so widespread.

github-actions[bot] commented 4 months ago

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

Thus the PR was passing and merged! Have a great day!