Earthfile in spaceros folder does not work as expected

[xfiderek]

I think i discovered some issues with https://github.com/space-ros/docker/blob/main/spaceros/Earthfile

Issue 1

Update 17.12.2023 - This issue will be fixed by: #89

COPY statement refers old branch of main space-ros repo (link)

sources:
  FROM +setup
  COPY github.com/space-ros/space-ros:earthly-wrapper+repos-file/ros2.repos ros2.repos

Current behavior: As a result, changes to files in space-ros repo are not reflected in docker build. This is because earthly-wrapper is some old branch with latest commit over a year ago.
Expected behavior (my guess): the latest earthfile from main branch is refered.

Check Earthly docs for more details on COPY syntax.

Issue 2

Update 17.12.2023 - This issue will be fixed by: #89
Do we even want to refer Earthfile from space-ros main repo in this build?

Current behavior: ros2.repos file from space-ros repo and ros2.repos file produced in this build may differ. This is because in this build, we refer repos-file task from space-ros earthly file, which means that every command from repos-file task is executed.
Expected behavior (my guess): We should clone ros2.repos file from space-ros repo, instead of re-running all commands.

Issue 3

There are some issues with rosdep

https://github.com/space-ros/docker/blob/c60ab974919ceb91e6ead0c2e28c9b9178c06be8/spaceros/Earthfile#L115C1-L115C345

RUN rosdep install --from-paths src --ignore-src --rosdistro rolling -y --skip-keys "console_bridge fastcdr fastrtps rti-connext-dds-5.3.1 urdfdom_headers rmw_connextdds ros_testing rmw_connextdds rmw_fastrtps_cpp rmw_fastrtps_dynamic_cpp composition demo_nodes_py lifecycle rosidl_typesupport_fastrtps_cpp rosidl_typesupport_fastrtps_c ikos"

1. rosdistro is hardcoded to rolling
2. skip-keys are hardcoded. should not we pass here excluded-pkgs.txt instead?
3. system packages installation is not reproducible - as we are installing system dependencies via rosdep it may happen that version of some dependencies will change across builds. Is it something that we want to accept?¹. System dependencies should be rather stable so that should not be a big deal, but I wanted to raise it anyway to make sure that its 100% expected.

Footnotes

1. As an alternative, we could specify the exact tag of ubuntu:jammy docker image and generate a list of system dependencies with exact package versions the same way we do it for ros2.repos (I guess it should be doable with rosdep) ↩

[Bckempa]

Issue 2 looks like an oversight from when the Earthfile was split between the docker and Space ROS repo proper. We don't want to dump too much energy into the Earthly file before space-ros/space-ros#107 is resolved, but patching to conform to the expected behavior you listed should be on our hit-list for this release.

[Bckempa]

Fixing #91 should address issue 1 as well

[xfiderek]

Ok, then my suggestion is to clarify way forward regarding space-ros/space-ros#107 and then patch the files accordingly. We don't need to fully close 107, but having an action plan would help us to avoid unnecessary refactors.

IMO if we want to avoid redundant refactors, the sequence should look like this:
Decision on space-ros/space-ros#107 -> #91 -> #88

Does it make sense?

Also, by when we want to have a decision reg. 107? Maybe we should add it to the meeting agenda space-ros/space-ros#117.

After we discuss the course of action, I can resolve both this issue and #91.

[xfiderek]

Added Issue no.3

[xfiderek]

As we discussed in the call, I will start fixing Issue 1 and 2 prior to making the decision space-ros/space-ros#114

[xfiderek]

Update: Issue 1 and Issue 2 will be addressed by #89
@Bckempa please take a look at newly added Issue 3 above

[Bckempa]

Issue 3

There are some issues with rosdep

https://github.com/space-ros/docker/blob/c60ab974919ceb91e6ead0c2e28c9b9178c06be8/spaceros/Earthfile#L115C1-L115C345

RUN rosdep install --from-paths src --ignore-src --rosdistro rolling -y --skip-keys "console_bridge fastcdr fastrtps rti-connext-dds-5.3.1 urdfdom_headers rmw_connextdds ros_testing rmw_connextdds rmw_fastrtps_cpp rmw_fastrtps_dynamic_cpp composition demo_nodes_py lifecycle rosidl_typesupport_fastrtps_cpp rosidl_typesupport_fastrtps_c ikos"

1. rosdistro is hardcoded to rolling
2. skip-keys are hardcoded. should not we pass here excluded-pkgs.txt instead?
3. system packages installation is not reproducible - as we are installing system dependencies via rosdep it may happen that version of some dependencies will change across builds. Is it something that we want to accept?1. System dependencies should be rather stable so that should not be a big deal, but I wanted to raise it anyway to make sure that its 100% expected.

Footnotes

1. As an alternative, we could specify the exact tag of ubuntu:jammy docker image and generate a list of system dependencies with exact package versions the same way we do it for ros2.repos (I guess it should be doable with rosdep) ↩

1. Yah that's an oversight from the initial Space ROS conversion to a pinned release, let's PR that.
2. I agree, we should have a standard pattern for doing these things and I think the external .txt files that get copied into the builds are good from a version-control perspective.
3. I agree that fully pinned builds would be ideal, but I'm ok calling this out-of-scope for now since ROS packages are responsible for declaring their rosdep version constraints and we're targeting a specific LTS diristbuition repository. I considered freezing an apt-cache as part of the pinned releases and using that but that opens up a bung of edge cases and some pretty explicit ties to one packaging infrastructure I don't know if we want.

TL;DR: If we open branch that fixes the distribution in the rosdep call and moves to an excluded-pkgs.txt would you consider this issue addressed for the Jan release and we'll open a discussion on strategies for pinning OS deps for the next cycle?

[xfiderek]

Hi @Bckempa.
To fix the issue we need to close the PRs:
(ament/ament_ikos#3)
space-ros/space-ros#131
#122

Note that the last one has dependencies on previous PRs.