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 recipe for libignition-sensors4 #14313

Merged
merged 22 commits into from
Mar 21, 2021
Merged

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Mar 20, 2021

Related to #13551 . This adds a new missing library out of the ignition robotics libraries (see https://ignitionrobotics.org/).

The build on macOS is failing due to the need to 10.13 SDK, as done in #13677 we can merge the recipe and then re-render in the newly created feedstock.

I tried to put the recipe in the libignition-sensors directory even if it creates a libignition-sensors4 package as given that we use a single feedstock to generate all the (different) packages named libignition-sensors<n> we would like it to be called libignition-sensors-feedstock instead of libignition-sensors4-feedstock (as in the future it will host also libignition-sensors5, libignition-sensors5, ...) but to be honest I am not sure it will work. However, this is just a nice to have, nothing really important. Actually we modified the feedstock to specify explicitly its name, see #14313 (comment) .

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/example/libignition-sensors) and found some lint.

Here's what I've got...

For recipes/example/libignition-sensors:

  • Please move the recipe out of the example dir and into its own dir.
  • Please do not delete the example recipe found in recipes/example/meta.yaml.

@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/libignition-sensors) and found it was in an excellent condition.

@traversaro
Copy link
Contributor Author

@wolfv @Tobias-Fischer @j-rivero @JShep1 I listed yourself as additional maintainers, please confirm if this is ok for you, or if you prefer not to be a mantainer of this package.

@wolfv
Copy link
Member

wolfv commented Mar 20, 2021

I'm fine with that!

@traversaro
Copy link
Contributor Author

macOS fails with:

In file included from /Users/runner/miniforge3/conda-bld/libignition-sensors4_1616242492432/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehol/include/sdformat-10.3/sdf/../sdf/Element.hh:28:
/Users/runner/miniforge3/conda-bld/libignition-sensors4_1616242492432/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehol/include/sdformat-10.3/sdf/../sdf/Param.hh:350:37: error: 'get<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, bool, char, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, int, unsigned long long, unsigned int, double, float, sdf::v10::Time, ignition::math::v6::Angle, ignition::math::v6::Color, ignition::math::v6::Vector2<int>, ignition::math::v6::Vector2<double>, ignition::math::v6::Vector3<double>, ignition::math::v6::Quaternion<double>, ignition::math::v6::Pose3<double>>' is unavailable: introduced in macOS 10.13
        std::string strValue = std::get<std::string>(this->dataPtr->value);
                                    ^
/Users/runner/miniforge3/conda-bld/libignition-sensors4_1616242492432/_build_env/bin/../include/c++/v1/variant:1435:16: note: 'get<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, bool, char, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, int, unsigned long long, unsigned int, double, float, sdf::v10::Time, ignition::math::v6::Angle, ignition::math::v6::Color, ignition::math::v6::Vector2<int>, ignition::math::v6::Vector2<double>, ignition::math::v6::Vector3<double>, ignition::math::v6::Quaternion<double>, ignition::math::v6::Pose3<double>>' has been explicitly marked unavailable here
constexpr _Tp& get(variant<_Types...>& __v) {

We probably need to target 10.13 due to the use of recent C++ features.

@traversaro traversaro changed the title [WIP] Add recipe for libignition-sensors4 Add recipe for libignition-sensors4 Mar 20, 2021
@traversaro
Copy link
Contributor Author

@conda-forge/help-c-cpp The PR is now ready for review. See the fist comment for an overview, and for the explanation of why the macOS build is failing and it make sense to merge the PR regardless.

@traversaro
Copy link
Contributor Author

@wolfv @Tobias-Fischer @j-rivero @JShep1 I listed yourself as additional maintainers, please confirm if this is ok for you, or if you prefer not to be a mantainer of this package.

For the time being I removed your names, once the PR is merged we can add them back if you want, thanks!

@Tobias-Fischer
Copy link
Contributor

I'm happy, too. One remark, last time we merged an ignition feedstock we were wondering if there's a way to avoid having the version number in the feedstock name. Should we try here?

@j-rivero
Copy link
Contributor

For the time being I removed your names, once the PR is merged we can add them back if you want, thanks!

fine for me too. Thanks Silvio

@traversaro
Copy link
Contributor Author

I'm happy, too. One remark, last time we merged an ignition feedstock we were wondering if there's a way to avoid having the version number in the feedstock name. Should we try here?

@xhochy provided the solution, it is just sufficient to specify the extra.feedstock_name parameter (see #14313 (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/libignition-sensors4) and found it was in an excellent condition.

@traversaro
Copy link
Contributor Author

Thanks @Tobias-Fischer @j-rivero , I added you as maintainers.

@wolfv
Copy link
Member

wolfv commented Mar 21, 2021

I think the tests on windows should use if not exists ... exit 1

Happy to merge right after!

@traversaro
Copy link
Contributor Author

I think the tests on windows should use if not exists ... exit 1

Happy to merge right after!

Old habits die hard. : ) Done! I also tried to make the recipe quite parametric, so for future ignition libraries it should be sufficient to just copy the meta.yaml and change dependencies and initial metadata.

- test -f ${PREFIX}/lib/libignition-{{ component_name }}{{ major_version }}.dylib # [osx]
- test -f ${PREFIX}/lib/cmake/ignition-{{ component_name }}{{ major_version }}/ignition-{{ component_name }}{{ major_version }}-config.cmake # [not win]
- if not exist %PREFIX%\\Library\\include\\ignition\\{{ component_name }}{{ major_version }}\\ignition\\{{ component_name }}.hh exit 1 # [win]
- if not exist $PREFIX$\\Library\\lib\\ignition-{{ component_name }}{{ major_version }}.lib (exit 0) exit 1 # [win]
Copy link
Member

Choose a reason for hiding this comment

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

I think here is still a typo?
Instead of (exit 0) exit 1, it should just be (exit 1), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, fixed now, sorry for the noise.

@traversaro
Copy link
Contributor Author

After the changes the Windows existence checks are failing, not sure why.

@wolfv
Copy link
Member

wolfv commented Mar 21, 2021

yeah the build log is really inconclusive :/ . Let me restart the windows build

@traversaro
Copy link
Contributor Author

yeah the build log is really inconclusive :/ . Let me restart the windows build

There is the same failure in #14323 that was copy pasted from this, so i guess there is some typo in the check, looking at it again.

@traversaro
Copy link
Contributor Author

yeah the build log is really inconclusive :/ . Let me restart the windows build

There is the same failure in #14323 that was copy pasted from this, so i guess there is some typo in the check, looking at it again.

Found it, it was $PREFIX$ instead of the correct %PREFIX%.

@wolfv wolfv merged commit 8cf4b26 into conda-forge:master Mar 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants