space-ros / docker

Docker images to facilitate Docker-based development.
21 stars 32 forks source link

Docker images do not include `ikos` #99

Closed ivanperez-keera closed 8 months ago

ivanperez-keera commented 8 months ago

So, I'm launching the space_robots image, which is based on spaceros:latest, and they do not include ikos.

They include ament_ikos, but not ikos, rendering the former a bit useless.

I think IKOS should be included with Space ROS.

ivanperez-keera commented 8 months ago

When I tried to install ikos inside the space_robots image, I also had to install the python3 packages catkin_pkg and empy.

ivanperez-keera commented 8 months ago

There are three ways we can go about this:

Thoughts?

xfiderek commented 8 months ago

I would avoid homebrew install. If we will be able to pull exact version that we are interested in from apt then we should go for it, provided that its possible on IKOS side to maintain repo with deb.

ivanperez-keera commented 8 months ago

I'd be willing to set for compiling from source for this release, in the interest of at least having something in.

I have an action item to create the DEB package.

I could use help with both of these tasks.

ivanperez-keera commented 8 months ago

By the way, I don't believe we can install packages with snap inside the docker image, but ikos is available as a snap.

Bckempa commented 8 months ago

By the way, I don't believe we can install packages with snap inside the docker image, but ikos is available as a snap.

I believe at that level of nested containerization we start owing Christopher Nolan royalties 😉

More seriously everything I've seen for running snapd inside a Docker container has been seriously hacky and route the actual hosting out to the docker host's snap process.

ivanperez-keera commented 8 months ago

We are still working towards providing a DEB, but that will take some time.

But I definitely think it's important that this version of Space ROS include IKOS.

We can for now install it "manually" and replace the deb in the future.

For extra points, we could figure out which dependencies are build only vs run-only, so that we purge those not needed after installation before the docker layer including ikos is "defined".

Bckempa commented 8 months ago

Manually for now makes sense. This also ties into the development vs deployment container idea we briefly discussed at the last meeting as a topic to explore after this release as well as a potential to better leverage caching by splitting the FS layers after the build so it gets reused when IKOS hasn't changed.

Bckempa commented 8 months ago

For some historical perspective, the ikos build was removed when the main spaceros container converted to use an Earthfile: https://github.com/space-ros/docker/pull/15/files#diff-2d4e413abb5e215100f775d885e15560cfec984f31f474d13ebf5360d022a7bbL146-L156

The IKOS_DIR argument in the Earthfile is the vestigial remnant of the process.

ivanperez-keera commented 8 months ago

IKOS has been submitted to Debian non-free: https://ftp-master.debian.org/new/ikos_3.2-1.html. It's still not in, but it's moving up the list to be included. We've also requested the change of license at NASA so that it can be included in Debian main.

However, we cannot assume that 1) it'll be included before we freeze (Thursday), and 2) the DEB will be immediately installable in Space ROS's docker image (the versions of libraries it is linked against may be different; that's often the case across distros and distro versions versions with, at least, libc).

I'd say, for this time around, we just undo the change that Brian pointed to, compile it inside the Space ROS image, and put that in the 2024.01.0 release.

Bckempa commented 8 months ago

That adds a ton to the build overhead, both in building IKOS and in running the analysis, when we don't have the full dashboard-and-disposition system in place and plan to do so for the next release. I'd lean away from adding something that big at the last minute and would rather see it as the first commit after this release.

ivanperez-keera commented 8 months ago

Here I have to disagree, for three reasons:

First, it's not last-minute. Upgrading the version of ikos available in Space ROS was proposed for this release almost 3 months ago, just tracked as part of a different issue (https://github.com/space-ros/space-ros/issues/29#issuecomment-1793504079). As a matter of fact, the release of IKOS in Dec 2023 tried to address the installation concerns raised as part of that issue in the Space ROS project, and was timed precisely so that it could be included in Space ROS, so not at all a sudden matter.

Second, it's reverting a change that happened before, and we've been talking for a while about how to best do this. I'll also note that the reason why IKOS was removed was due to the LLVM version available in Ubuntu 22.04, something that was fixed in the release of IKOS in Dec 2022, not for any reasons related to the compilation time. It won't add, say, 20 minutes to the compilation time either.

Third, it doesn't add a ton to the build overhead, and we don't need to run the analysis either, only make IKOS available inside the image. Most people will just pull from osrf/space-ros:latest, so it's not something that affects them in any way. If we can compile other packages, we can build IKOS.

EDIT: Further details.

Bckempa commented 8 months ago

I'm unconvinced but not going to put up a fight on this. If you intend this to be in the release please remember to add it to the project and milestone.

ivanperez-keera commented 8 months ago

I have the DEBs for ikos. I tried it on a fresh ubuntu and they work.

I'm not sure about whether it would be best to make them release artifacts in the ikos release, or to put them elsewhere.

But once that's sorted, installing them in the image should be trivial.

ivanperez-keera commented 8 months ago

If I'm reading the Github Actions right, the build times for the spaceros image are pretty much the same with and without IKOS. It doesn't appear to have a noticeable impact in the build time.