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

Development workflows #139

Closed
stonier opened this issue Oct 20, 2020 · 24 comments
Closed

Development workflows #139

stonier opened this issue Oct 20, 2020 · 24 comments

Comments

@stonier
Copy link
Collaborator

stonier commented Oct 20, 2020

Support simpler and more flexible development workflows.

  • Work on an arbitrary subset of our repos - e.g. delphyne/delphyne_gui only → simpler & flexibly handle different use cases
  • Dependency management via one tool - simpler & shouldn't need to reinvent wheels (use or drive requirements for rosdep)
  • Minimise non-system dependencies - too much endeavour to maintain
  • Minimise bundling of packages - e.g. tinyxml \in odm \in malidrive → enable dependency sharing & avoid release incompatibilities

These are essentially the precepts of the ROS federated ecosystem. It's a set of rules that works and enables scaling and flexibility. Just a bit trickier to handle with closed source packages sans a build farm, but it's doable. I believe it is a good goal.

FYI @scpeters just to kick-start the thread.

@agalbachicar
Copy link
Collaborator

Prereqs installed packages to construct the workspace

  • The workspace installs via prereqs:
    • bash-completion
    • build-essential
    • curl
    • gdb
    • git
    • mercurial
    • python3
    • python3-setuptools
    • python3-vcstool
    • python3-rosdep
    • python3-colcon-common-extensions
    • ros-dashing-ament-cmake
    • clan8 and its family of utilitlies.
    • tmux

Prereqs installed packages to use maliput

  • libpython3-dev

Prereqs installed packages to use delphyne

  • libignition-common3-dev
  • libignition-math6-dev
  • libignition-msgs2-dev
  • libignition-transport5-dev

Prereqs installed packages to use delphyne-gui

  • libignition-common3-dev
  • libignition-math6-dev
  • libignition-msgs2-dev
  • libignition-tools-dev
  • libignition-cmake2-dev
  • libignition-cmake1-dev
  • libignition-rendering2-dev
  • libignition-gui0-dev

Prereqs installed packages to use drake-vendor

  • Pulls drake and installs its own prereqs list.

Extra packages in the workspace needed but not maintained

  • pybind11: drake's fork.
  • proj4: used in malidrive to parse xord origin
  • ament_cmake_doxygen: required to execute doxygen documentation

@agalbachicar
Copy link
Collaborator

Per meeting on 10/28/2020 we will try to get rid of proj4 by implementing the functionality in maliput (math).

@agalbachicar
Copy link
Collaborator

pybind11 provides support to all python bindings in maliput and in delphyne. We are using that version because of compatibility with drake.

Related issues:

@agalbachicar
Copy link
Collaborator

About tinyxml in ODRM, here are some @stonier 's notes from the CMakeLists.txt:

#
# OpenDrive comes with a very old version of tinyxml.
#
# - OpenDrive Version: 2.4.3
# - Xenial Version: 2.6.2
# - Used in: OdrReaderXML.cc
#
# *Status*: The internal version has been reproduced faithfully here.
# *Caveat*: That will have conflicts if someone's application tries to
# link against this opendrive and a system version of tinyxml.
#
# ALTERNATIVE I: Since it's only used in opendrive sources,
# build it internally, hide visibility. The advantage of this is that the
# opendrive sources themselves require no modification.
#
# ALTERNATIVE II: Use the system version, but you'll need a small script
# to make modifications to OdrReaderXML.cc (mostly prefixing the various
# enums with TINYXML_).
#

@agalbachicar
Copy link
Collaborator

We also have libgtest code in malidrive, delphyne and delphyne-gui. We should be better off removing it and using system dependency as in maliput.

@stonier
Copy link
Collaborator Author

stonier commented Oct 28, 2020

Great, I think you're moving in the right direction re minimising dependencies:

  • TinyXML: is this a no-op if we drop ODRM?
  • Libgtest: concur, system dependency
  • Proj: concur, eliminate by adding functionality to maliput-math(?)
  • Pybind11: I wonder how much we need the system specific bindings that need the unique ptr functionality? e.g. perhaps we can just expose our own c++ api (sans unique pointer requirements) to be python bound (either with a non-custom pybind11 or boost's python) -- Plugin architecture for maliput maliput#373
  • Ignition dependencies: I haven't dug in to this one yet, but wondering what is needed here to get it to properly work with package.xml/rosdep -- Migrate to newer version of ignition-gui delphyne_gui#332

Aside from minimising dependencies, other objectives I'd like to hit here (please comment if you think they make sense or not):

  • Eliminate use of prereqs-install. It's awkwardly competing with package.xml / rosdep for dependency management). One tool for one job.
  • Binary underlay (like /opt/ros/eloquent) that serves our own packaging. This enables flexible creation of workspaces.
  • Solve any residual pure cmake / python problems in the workspace for dependency management (e.g. PROJ is entirely broken today). It should hopefully be entirely resolved by other items above.

@agalbachicar
Copy link
Collaborator

Re proj4 --> maliput/maliput#356

@agalbachicar
Copy link
Collaborator

TinyXML: is this a no-op if we drop ODRM?

The new implementation of malidrive relies on TinyXML2 from system packages (installed via rosdep) for xodr map parsing.

Pybind11: I wonder how much we need the system specific bindings that need the unique ptr functionality? e.g. perhaps we can just expose our own c++ api (sans unique pointer requirements) to be python bound (either with a non-custom pybind11 or boost's python)

I believe we can proxy the access to unique_ptrs. I also know @andrewbest-tri and @liangfok wanted to include boost-python bindings. Would that make sense in favor of deprecating pybind11? (I still need to analyze the impact on delphyne and delphyne-gui)

Eliminate use of prereqs-install. It's awkwardly competing with package.xml / rosdep for dependency management). One tool for one job.

Makes sense to me.

Binary underlay (like /opt/ros/eloquent) that serves our own packaging. This enables flexible creation of workspaces.

We are using dashing now. Is it a requisite or can we push this migration to a separate effort? I haven't checked how much it would take to migrate though, note that combinatoric escalation is a problem for CI infrastructure maintenance and budget.

Solve any residual pure cmake / python problems in the workspace for dependency management (e.g. PROJ is entirely broken today). It should hopefully be entirely resolved by other items above.

Understood.

@scpeters
Copy link
Contributor

Re proj4 --> ToyotaResearchInstitute/maliput#356

I didn't mention this during the meeting, but it looks like libproj-dev is in Ubuntu, though the version in 18.04 is only 4.9.3, though the version in 20.04 is 6.3.1

I assume at some point we will move to 20.04; perhaps we could at least test with 6.3.1 to see if it is suitable? it could save us from maintaining extra code in maliput

@agalbachicar
Copy link
Collaborator

All custom gtest and gmock versions were removed from the workspace.

@agalbachicar
Copy link
Collaborator

Dumping a few decisions taken in weekly meetings:

Pybind11: I wonder how much we need the system specific bindings that need the unique ptr functionality? e.g. perhaps we can just expose our own c++ api (sans unique pointer requirements) to be python bound (either with a non-custom pybind11 or boost's python)

For maliput, we will split maliput bindings into a separate package and from there develop a plugin architecture. Backends will require a new load interface so they can be seamlessly loaded in python or any other language (not C++).

For delphyne, we still require pybind due to drake's interaction. We could move all the bindings to another package (potential increased and unnecessary complexity) and release from pybind11 dependency delphyne. It also brings another question onto the table, is it necessary to have delphyne on its own? Do we foresee or is it there already another package that uses delphyne but not delphyne-gui / delphyne-demos?

Ignition dependencies: I haven't dug in to this one yet, but wondering what is needed here to get it to properly work with package.xml/rosdep

Started a migration of these libraries to newer versions.

@stonier
Copy link
Collaborator Author

stonier commented Dec 22, 2020

It also brings another question onto the table, is it necessary to have delphyne on its own? Do we foresee or is it there already another package that uses delphyne but not delphyne-gui / delphyne-demos?

Just sane dependency management. Over the years I've practiced/advocated carving up msg / non-gui and gui packages from the get-go separately because user stories frequently want/need installation of them separately to avoid dependency headaches in workspaces or installations. I find it also helps in keeping the architecture from getting entangled (no matter how good initial intentions are) and that also is a reason to do it from the get-go. What are the advantages of the borg approach?

@stonier
Copy link
Collaborator Author

stonier commented Dec 22, 2020

Started a migration of these libraries to newer versions.

Great, thanks!

@stonier
Copy link
Collaborator Author

stonier commented Dec 22, 2020

For delphyne, we still require pybind due to drake's interaction. We could move all the bindings to another package (potential increased and unnecessary complexity) and release from pybind11 dependency delphyne.

We can probably unpack this one a bit further. Bindings for maliput and family are critical to be done right since it's broadening it's user base beyond N=2. Delphyne isn't planning to yet, so we can accommodate a little less rigour there, but it would be good to get it off a dependency on drake's python bindings. We don't really use them as intended (pythonic construction of drake systems) and could easily(?) just implement a few of our own with whatever library we choose to minimise our dependency tax.

@francocipollone
Copy link
Collaborator

francocipollone commented May 31, 2021

After a F2F chat with @agalbachicar: Next steps to finally deprecate prereqs machinery.

@scpeters
Copy link
Contributor

scpeters commented Jun 2, 2021

FYI we won't need the script for installing ignition packages on 20.04 since they are handled by rosdep

@francocipollone
Copy link
Collaborator

FYI we won't need the script for installing ignition packages on 20.04 since they are handled by rosdep

That's right. It will be even cleaner after we move to 20.04. So that script for installing ignition packages and also the instructions for installing the ignition packages in the installation and quick start guide will be happily removed.

UPDATE:

preqres machinery removed from ws 🥂

@agalbachicar
Copy link
Collaborator

What's blocking us now to close this ticket is to migrate to Ubuntu focal + ROS2 Foxy ( #196 ) and to solve pybind11 issues.

@agalbachicar
Copy link
Collaborator

TinyXML: is this a no-op if we drop ODRM?

Recently changed to system version downstream and it was already as a system dependency (via rosdep) for maliput_malidrive.

@agalbachicar
Copy link
Collaborator

@stonier , per #225 investigation it is not possible to move forward with pybind11 as a system dependency because of unique_ptrs in bionic ( needed by maliput repositories ). As soon as we move to focal in all repositories, that will be solved. I believe we have covered everything else. Do you agree with closing this ticket and keeping #225 open to track the pybind11 migration?

@francocipollone
Copy link
Collaborator

@stonier , per #225 investigation it is not possible to move forward with pybind11 as a system dependency because of unique_ptrs in bionic ( needed by maliput repositories ). As soon as we move to focal in all repositories, that will be solved. I believe we have covered everything else. Do you agree with closing this ticket and keeping #225 open to track the pybind11 migration?

@agalbachicar @stonier Is it now a good moment to re-ask: Should we still support bionic-dashing?

@francocipollone
Copy link
Collaborator

@stonier , per #225 investigation it is not possible to move forward with pybind11 as a system dependency because of unique_ptrs in bionic ( needed by maliput repositories ). As soon as we move to focal in all repositories, that will be solved. I believe we have covered everything else. Do you agree with closing this ticket and keeping #225 open to track the pybind11 migration?

@agalbachicar @stonier Is it now a good moment to re-ask: Should we still support bionic-dashing?

It was agreed during weekly meeting: #249

@francocipollone
Copy link
Collaborator

@stonier , per #225 investigation it is not possible to move forward with pybind11 as a system dependency because of unique_ptrs in bionic ( needed by maliput repositories ). As soon as we move to focal in all repositories, that will be solved. I believe we have covered everything else. Do you agree with closing this ticket and keeping #225 open to track the pybind11 migration?

I don't think they solved it for the version that is distributable for Focal (2.4.3).
See here.

They seemed to solve it later on at pybind/pybind11#2672

@francocipollone
Copy link
Collaborator

Closing as all the tasks were finished

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

No branches or pull requests

4 participants