ros-tooling / setup-ros

Github Action to set up ROS 2 on hosts
Apache License 2.0
87 stars 41 forks source link

Prefer system packages over pip packages #535

Closed christophebedard closed 1 year ago

christophebedard commented 1 year ago

Description

The goal is to more closely match the packages installed:

Related Issues

Otherwise, when using setup-ros, we might get a newer version of a package from pip. This can lead to issues like https://github.com/ros2/ros2_tracing/issues/27, where tests fail with setup-ros but not with ci.ros2.org

Completion Criteria

What needs to be true before we can call this "Done"? Bullet lists are appropriate.

Implementation Notes / Suggestions

Compare with ci.ros2.org, e.g.: https://ci.ros2.org/job/ci_linux/17993/consoleFull#console-section-4

Testing Notes / Suggestions

Nothing new or different to test.

Flova commented 1 year ago

I currently face the same issue. I require python3-transforms3d via rosdep. This is compatible to python3-numpy, but the CI upgrades NumPy to a version where np.float is removed and replaced with float. This makes it incompatible with the python3-transforms3d version in the system packages. Locally everything works well, but the CI does not pass.

christophebedard commented 1 year ago

I'll take care of this once the bump to Node.js v16 is done (#521).

christophebedard commented 1 year ago

I've looked into this more and have a proposal.

If you follow the docs for a source build, you:

  1. Install a fairly limited number of packages needed for development
    1. The Rolling instructions here for 22.04 only use apt, but the Rolling instructions for 20.04 use pip for some dependencies, possibly to get versions that are newer and closer/equal to the ones from apt on 22.04. Foxy on 20.04 also uses pip, possibly just to get newer versions.
  2. Clone the ROS 2 repos
  3. Then install dependencies using rosdep, which prefers apt

The list of packages we currently install using pip is much longer than the list of packages installed during step 1: https://github.com/ros-tooling/setup-ros/blob/b074834c505f0d79000f03d991bed2d0bdb04cca/src/package_manager/pip.ts#L5-L62 This is probably because Windows and macOS use pip instead of apt (although they use choco and brew for some dependencies), so doing (mostly) the same steps for all platforms is easier. However, as we can see from the issues mentioned above, it doesn't work well.

On the other hand, ci.ros2.org's equivalent to step 1 (before the repos are even cloned) installs more packages, like python3-mypy, although this is most likely done for common dependencies to speed up builds with Docker layer caching.

I'd go with copying the official source build instructions for Ubuntu and leave Windows and macOS as-is. That means Ubuntu installs would have distro-specific apt and pip packages lists and wouldn't use the "universal" pip list. I wouldn't bother installing additional common dependencies like ci.ros2.org does, because there's no real benefit since we're not using Docker here.

The only risk is if this breaks existing configs/code, e.g., if someone changed their code to appease setup-ros's newer version of mypy, their CI might break if setup-ros switches to using an older mypy version. However, one could argue that it's already sort of broken. I would at least bump the minor version, of course. I implemented this for Jammy (https://github.com/ros-tooling/setup-ros/tree/christophebedard/ubuntu-prefer-system-packages) and it works as expected: https://github.com/ros2/ros2_tracing/pull/8.

@emersonknapp @ijnek @Flova any thoughts?

emersonknapp commented 1 year ago

I think the state as it is currently was based on an older version of the setup instructions. But it may have been a copy of the ci.ros2.org approach instead. I have no problem changing things to match the current source build instructions.

ijnek commented 1 year ago

@christophebedard Agree with your suggestions.

e.g., if someone changed their code to appease setup-ros's newer version of mypy, their CI might break if setup-ros switches to using an older mypy version.

Personally, I wouldn't push changes that don't work locally so I think if this happens, it would only affect very few users, if any (assuming users aren't installing their dependencies through pip too).

christophebedard commented 1 year ago

FYI @ijnek @Flova this was implemented in #593. It was released as v0.7/0.7.0.