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 ign-rendering to feedstock #13677

Merged
merged 11 commits into from
Jan 23, 2021

Conversation

JShep1
Copy link

@JShep1 JShep1 commented Jan 13, 2021

Signed-off-by: John Shepherd [email protected]

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.

I'm on the Ignition Gazebo team and attempting to get the remaining Ignition packages into conda-forge for increased Windows support. This is my first time contributing to conda-forge so I'm open to feedback for steps I may have missed or other improvements that I need to make. I followed the contributing instructions as closely as I could but understand I may have made mistakes along the way.

There are a few more Ignition packages that we would like to add to conda-forge (see here) so once I get feedback on this one, I'll open PRs for the remaining Ignition packages.

Signed-off-by: John Shepherd <[email protected]>
@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-rendering-recipe) and found it was in an excellent condition.

@traversaro
Copy link
Contributor

Hi @JShep1 , I think that at the moment the recipe is failing as the build.sh and bld.bat scripts are missing. Fortunately, those scripts look similar for most CMake-based projects, and so this applies also for ignition libraries. For an example of this build scripts, see:

@JShep1
Copy link
Author

JShep1 commented Jan 13, 2021

Hi @JShep1 , I think that at the moment the recipe is failing as the build.sh and bld.bat scripts are missing. Fortunately, those scripts look similar for most CMake-based projects, and so this applies also for ignition libraries. For an example of this build scripts, see:

Thanks for the advice! I'll get the ign-rendering equivalent scripts in soon.

Signed-off-by: John Shepherd <[email protected]>
@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-rendering) and found it was in an excellent condition.

Signed-off-by: John Shepherd <[email protected]>
@Tobias-Fischer
Copy link
Contributor

To get OpenGL, try adding this to the meta.yaml:

In the build section:
{{ cdt('mesa-libgl-devel') }}  # [unix]
{{ cdt('mesa-dri-drivers') }}  # [unix]
{{ cdt('libselinux') }}  # [linux]
{{ cdt('libxdamage') }}  # [linux]
{{ cdt('libxxf86vm') }}  # [linux]
{{ cdt('libxfixes') }}  # [linux]
{{ cdt('libxext') }}  # [linux]
{{ cdt('libxau') }}  # [linux]

In the host & run sections:
xorg-libx11  # [unix]
xorg-libxext  # [unix]

I don't think you'll need freeglut, but let's see (remark: freeglut is not available on osx).

John Shepherd added 2 commits January 13, 2021 20:09
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
@JShep1
Copy link
Author

JShep1 commented Jan 14, 2021

To get OpenGL, try adding this to the meta.yaml:

Thanks! Greatly appreciated

@Tobias-Fischer
Copy link
Contributor

No worries - seems like it did the job :)

@wolfv
Copy link
Member

wolfv commented Jan 14, 2021

hmm, the internet tells me that defining GLX_GLXEXT_LEGACY=1 might help the issue with the GLintptr?

Really happy to see more robotics here! Welcome!

@traversaro
Copy link
Contributor

On Windows, a specific test is failing:

39/56 Test #39: UNIT_RenderingIface_TEST ..............***Failed    0.42 sec

Probably it may be easier to debug and check if this is happening also for a build of the library done "manually". If you want to temporary exclude that test by adding it to the exclusion option -E (that is already used for INTEGRATION|PERFORMANCE|REGRESSION .

@traversaro
Copy link
Contributor

hmm, the internet tells me that defining GLX_GLXEXT_LEGACY=1 might help the issue with the GLintptr?

Note that this seems to induced by the inclusion of the GL/glx.h header in https://github.com/ignitionrobotics/ign-rendering/blob/f8ec77282d02a0b92de1ee5b24c5a3bd4948775e/ogre/src/OgreRenderEngine.cc#L22 , a reasonable first test may be to just check if the included header is actually necessary by removing it.

Note that (as it typically happens with ignition libs) a similar inclusion is also happening in Classic Gazebo ( https://github.com/osrf/gazebo/blob/10863ffa8ec28e8e0e3d3a3a588d21cdc78055ab/gazebo/rendering/RenderEngine.cc#L27 ), so you can also try to checkout the gazebo feedstock ( https://github.com/conda-forge/gazebo-feedstock/tree/master/recipe ), because that line seems to be working fine there

@traversaro
Copy link
Contributor

traversaro commented Jan 14, 2021

For the macOS failure, it seems that the tests require an SDK targeting 10.13 . I remember we had a similar problem for another ignition library, but I can't remember which one it was, perhaps @Tobias-Fischer remembers. However, given that in this case the problem only affects the tests, it could make sense to disable the test compilation.
It was probably something related to this file: https://github.com/conda-forge/gazebo-feedstock/blob/master/recipe/conda_build_config.yaml .

@Tobias-Fischer
Copy link
Contributor

Yes that's correct re OSX, See also the doc: https://conda-forge.org/docs/maintainer/knowledge_base.html#requiring-newer-macos-sdks

On gazebo we should add the run requirements as described in the docs .. we're naughty not doing that at the moment.

@Tobias-Fischer
Copy link
Contributor

Hi @JShep1, could you please also add sth like in this commit for OSX? conda-forge/gazebo-feedstock@06af3f5

Signed-off-by: John Shepherd <[email protected]>
@JShep1
Copy link
Author

JShep1 commented Jan 22, 2021

Hi @JShep1, could you please also add sth like in this commit for OSX? conda-forge/gazebo-feedstock@06af3f5

Aha, thanks, I originally had this line but second guessed myself 😅

@Tobias-Fischer
Copy link
Contributor

osx fails because we didn't re-render. Not sure how that works in staged recipes?!?

John Shepherd added 3 commits January 21, 2021 22:05
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
@traversaro
Copy link
Contributor

osx fails because we didn't re-render. Not sure how that works in staged recipes?!?

I am not sure, but a possible strategy is just skip osx for now, merge the PR and add osx back in the feedstock once it is created.

@Tobias-Fischer
Copy link
Contributor

Sounds good to me. Or even merge as is, as the problem should resolve by itself?

@traversaro
Copy link
Contributor

traversaro commented Jan 22, 2021

Sounds good to me. Or even merge as is, as the problem should resolve by itself?

I am not sure if it is possible to merge PRs on staged-recipes with failing build status, I guess @wolfv knows better.

Copy link
Member

@wolfv wolfv left a comment

Choose a reason for hiding this comment

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

Awesome!

I left some small comments.

recipes/libignition-rendering/build.sh Outdated Show resolved Hide resolved
recipes/libignition-rendering/meta.yaml Show resolved Hide resolved
@@ -0,0 +1,4 @@
macos_min_version: # [osx]
Copy link
Member

Choose a reason for hiding this comment

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

I am going to ask on the conda-forge gitter if we can merge with the failing build or if we need to skip OS X first.

Signed-off-by: John Shepherd <[email protected]>
@wolfv wolfv merged commit c921bac into conda-forge:master Jan 23, 2021
@traversaro
Copy link
Contributor

Something to consider for future ignition-* recipes is to understand if there is a way to generate the feedstock with a name different from the recipe, to avoid the usual confusion of a feedstock with a name with the number that generates recipes also subsequent major releases.

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.

5 participants