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

Resolving system dependency conflicts (meta) #7412

Closed
15 of 17 tasks
stonier opened this issue Nov 6, 2017 · 15 comments
Closed
15 of 17 tasks

Resolving system dependency conflicts (meta) #7412

stonier opened this issue Nov 6, 2017 · 15 comments
Assignees

Comments

@stonier
Copy link
Contributor

stonier commented Nov 6, 2017

FCL & Dependencies

Strategy is to hide symbols in FCL & CCD. Octomap will use the same version as that in ROS (1.8.1) as opposed to system. Most users are likely on the same version, if this changes, then we can rethink/hide symbols there. Note that Drake only uses octomap indirectly via FCL.

  • ccd : send patch upstream and merged for symbol hiding (libccd/pull/39)
  • ccd : wait for patch to be accepted
  • fcl : send patch upstream and merged for symbol hiding (fcl/pull/233)
  • fcl bugfixing : reported a discovered bug upstream, but does not affect us directly (fcl/issues/244)
  • ccd/fcl/octomap : drake update for the holy trinity (drake/pull/7417)

YAML

VTK

PCL Enabling

Protobuf

More detailed notes in this spreadsheet.

@nuclearsandwich, @j-rivero, @clalancette can you add comments as things get resolved, or new items are brought to the fore. I'll update this list as we go.

@stonier stonier self-assigned this Nov 6, 2017
@jwnimmer-tri
Copy link
Collaborator

FYI I reduced the permissions on the "spreadsheet" link above from "Anyone with the link can edit" to "Anyone with the link can view". Otherwise, the document would turn into a w4r3z site. We may have to add back individually-authenticated commentors or editors later to compensate.

@stonier
Copy link
Contributor Author

stonier commented Nov 7, 2017

Thanks

@j-rivero
Copy link
Contributor

j-rivero commented Nov 7, 2017

Thanks for the summary @stonier, the list matches all that I have in my list.

One question: for the cases of ccd and and fcl, I understand that we want to port patches (submit pull requests) to internal TRI forks. Could you please point me to the location of these forks?.

FYI I reduced the permissions on the "spreadsheet" link above from "Anyone with the link can edit" to "Anyone with the link can view". Otherwise, the document would turn into a w4r3z site. We may have to add back individually-authenticated commentors or editors later to compensate.

Thanks Jeremy, I added most of us to the list of people that can edit.

@stonier
Copy link
Contributor Author

stonier commented Nov 7, 2017

From the WORKSPACE file, it looks as if we are actually not currently using forks of fcl nor libccd, just hashes from the original projects. They both certainly are getting upstream patches.

libccd

We are in fact using a very old tag of libccd (2014). There has been very little code changes since then - just one commit I think. The rest have been in CMake upgrade and it looks like @jamiesnape has already started to do some symbol hiding. Diff between tip and v2.0 here. Since it already has some logic for exports in tip, we should probably try and upgrade Drake to use that and do whatever patching is necessary on top of that to hide the symbols in Drake.

  • @j-rivero : check that Drake builds from tip
  • @j-rivero : if that works, create a PR that points to tip of the RobotLocomotion/libccd fork I just created
  • @avalenzu : to follow up and make sure that the contact collisions still work with the PR
  • @j-rivero : if necessary, drop a patch on the fork for hiding symbols, update the PR, and upstream the patch

Sound like a plan?

FCL

It has a fork in RobotLocomotion/fcl now.

@j-rivero
Copy link
Contributor

j-rivero commented Nov 7, 2017

Sound like a plan?

Yep. Working on it.

@j-rivero
Copy link
Contributor

j-rivero commented Nov 7, 2017

@stonier the pull request should be ready at pull request #7417

The fcl cherry pick for your own fork is at RobotLocomotion/fcl#1

@stonier
Copy link
Contributor Author

stonier commented Nov 9, 2017

Some updates to the above.

Yaml

Yaml is blocking on system protobuf getting in because of a Mac issue on crosstool not having /usr/local/include which indirectly itself causes issues with the protobuf 3.1 currently being used. Refer to the causal chain in the first post.

Protobuf

Pending on a solution to adopting homebrew's python. @jamiesnape can you raise that issue and start discussion on how to solve this ASAP?

@jamiesnape
Copy link
Contributor

Pending on a solution to adopting homebrew's python. @jamiesnape can you raise that issue and start discussion on how to solve this ASAP?

#7440

@jamiesnape
Copy link
Contributor

jamiesnape commented Nov 9, 2017

We are in fact using a very old tag of libccd (2014). There has been very little code changes since then - just one commit I think. The rest have been in CMake upgrade and it looks like @jamiesnape has already started to do some symbol hiding. Diff between tip and v2.0 here. Since it already has some logic for exports in tip, we should probably try and upgrade Drake to use that and do whatever patching is necessary on top of that to hide the symbols in Drake.

You should be able to extend slightly (with say the boilerplate from the GCC wiki) all the logic I added to hide everything that you need to and keep their three build systems working rather than trying to do so with CMake-specific logic per danfis/libccd#39. Not using GenerateExportHeader was deliberate given the requirements of that project, though maybe that changed.

@nuclearsandwich
Copy link
Contributor

#7453 excludes the yaml_util header from the installation without reverting back to system yaml-cpp. We probably still want to re-introduce the system yaml-cpp changes once the build on mac stabilizes.

@j-rivero
Copy link
Contributor

Not using GenerateExportHeader was deliberate given the requirements of that project, though maybe that changed.

To pass all the CI tests (they are using cmake, autotools and Makefile) you are right, we can not rely only in the GenerateExportHeaders of cmake. I've changed the PR to use an static file for the exports and patch Makefile and autotools. All tests passes now.

@jamiesnape
Copy link
Contributor

Looks like libccd merged the patch.

@jwnimmer-tri
Copy link
Collaborator

I think this might be finished? At least, the checkbox list in the top post shows that everything has merged.

@nuclearsandwich
Copy link
Contributor

Every dependency with an identified conflict was resolved. So yes, unless/until new issues are reported I think this could be closed.

@jwnimmer-tri
Copy link
Collaborator

Thanks! This was very exciting to see happen, it should be a nice improvement in compatibility. I will close for now but we can re-open and/or file a new issue if we find we need other improvements.

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

No branches or pull requests

5 participants