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

Cleanup pinnings and fix use of ffmpeg dependencies #38

Merged
merged 19 commits into from
Nov 29, 2020
Merged

Cleanup pinnings and fix use of ffmpeg dependencies #38

merged 19 commits into from
Nov 29, 2020

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Nov 28, 2020

In particular:

  • Change max_pin to x, as Gazebo minor version are ABI backward compatible
  • Remove run dependencies for packages that correctly have run_exports
  • Remove explicitly pinning of libsdformat and ignition-common3

This was necessary because the new Gazebo build after #37 caused the Gazebo build again to depend transitively on libuuid on macOS, with all the problems already discussed in conda-forge/libignition-common-feedstock#18 . Example of build failed for this: https://github.com/robotology/robotology-superbuild/pull/544/checks?check_run_id=1467517151 .

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

In particular: 
* Change max_pin to x, as Gazebo minor version  are ABI backward compatible 
* Remove run dependencies for packages that correctly have run_exports 
* Remove explicitly pinning of libsdformat and ignition-common3
@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 (recipe) and found it was in an excellent condition.

@traversaro traversaro changed the title Cleanup pinning Cleanup pinnings Nov 28, 2020
@traversaro traversaro closed this Nov 28, 2020
@traversaro traversaro reopened this Nov 28, 2020
@traversaro
Copy link
Contributor Author

@conda-forge-admin, please rerender

@github-actions
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.
I tried to rerender for you, but it looks like there was nothing to do.

@traversaro
Copy link
Contributor Author

traversaro commented Nov 28, 2020

The use of libignition-common3 3.7 meant that ffmpeg was brought in as a transitive dependency, and then the linking of Gazebo against ffmpeg was failing for the usual conda-forge/cmake-feedstock#129 . I updated the fix_build.patch to handle also that case.

I also fixed the test on Windows that checked for the existence of gazebo.dll, when now that file was renamed in libgazebo.dll in gazebosim/gazebo-classic#2864, and now a proper gazebo.exe exists. The fact that those test did not failed in #37 is quite strange, as I manually checked the updated file in https://anaconda.org/conda-forge/gazebo/11.3.0/download/win-64/gazebo-11.3.0-hb6cee51_0.tar.bz2 and indeed it contains bin/libgazebo.dll and bin/gazebo.exe .

@traversaro traversaro changed the title Cleanup pinnings Cleanup pinnings and fix use of ffmpeg dependencies Nov 28, 2020
@traversaro
Copy link
Contributor Author

The additional fix was sufficient on macOS, while it did not worked on Linux, I guess for those subtle differences in linking behaviour between Linux and macOS . Let's look for another strategy.

@traversaro
Copy link
Contributor Author

traversaro commented Nov 29, 2020

A more definitive path (switching to just use imported targets) is available in https://github.com/traversaro/gazebo/commit/5761f25860353ade33741ea585e9fefa3db88b59.patch .

@traversaro
Copy link
Contributor Author

A more definitive path (switching to just use imported targets) is available in https://github.com/traversaro/gazebo/commit/5761f25860353ade33741ea585e9fefa3db88b59.patch .

The improvement of this patch is that now also Windows fails. : )
Apparently there is something else that is injecting the need for the swscale.so.5 and the other ffmpeg libraries, see the error:

$BUILD_PREFIX/bin/../lib/gcc/x86_64-conda-linux-gnu/9.3.0/../../../../x86_64-conda-linux-gnu/bin/ld: warning: libswscale.so.5, needed by gazebo/common/libgazebo_common.so.11.3.0, not found (try using -rpath or -rpath-link)
$BUILD_PREFIX/bin/../lib/gcc/x86_64-conda-linux-gnu/9.3.0/../../../../x86_64-conda-linux-gnu/bin/ld: warning: libavdevice.so.58, needed by gazebo/common/libgazebo_common.so.11.3.0, not found (try using -rpath or -rpath-link)
$BUILD_PREFIX/bin/../lib/gcc/x86_64-conda-linux-gnu/9.3.0/../../../../x86_64-conda-linux-gnu/bin/ld: warning: libavformat.so.58, needed by gazebo/common/libgazebo_common.so.11.3.0, not found (try using -rpath or -rpath-link)
$BUILD_PREFIX/bin/../lib/gcc/x86_64-conda-linux-gnu/9.3.0/../../../../x86_64-conda-linux-gnu/bin/ld: warning: libavutil.so.56, needed by gazebo/common/libgazebo_common.so.11.3.0, not found (try using -rpath or -rpath-link)

Probably the easiest fix is just to workaround conda-forge/cmake-feedstock#129 by adding a add_link_options(-L"${CMAKE_INSTALL_PREFIX}/lib") that is a brittle solution but should work even if new dependencies are enabled.

@traversaro
Copy link
Contributor Author

I think I finally understood why we have all this linking problems. The CI sets a reasonable options for the linker in the LDFLAGS environment variable, that is typically read by CMake and put in the CMAKE_EXE_LINKER_FLAGS and CMAKE_SHARED_LINKER_FLAGS. The problem is that the Gazebo build system overwrites this values in https://github.com/osrf/gazebo/blob/gazebo11/cmake/DefaultCFlags.cmake#L41 .

@traversaro
Copy link
Contributor Author

Finally, indeed the fix in 6c5bd6b was sufficient to fix the build. I think that after this fix many of the patch https://github.com/conda-forge/gazebo-feedstock/blob/master/recipe/fix_build.patch can be simplified substantially, but probably it make more sense to do it in a future release.

@Tobias-Fischer
Copy link
Contributor

Great work @traversaro! A similar issue was haunting me when trying to get slam-toolbox to compile: https://github.com/RoboStack/ros-noetic/blob/master/patch/ros-noetic-slam-toolbox.patch

Took me a little while to figure out what to do there, too.

Agreed that the patch can probably be simplified now - I'll create an issue to keep track of this, and assign you if you don't mind.

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

Successfully merging this pull request may close these issues.

3 participants