Tested locally:

executing command [apt-get install -y python3-pytest]
executing command [apt-get install -y python3-flake8]
executing command [apt-get install -y python3-matplotlib]
executing command [apt-get install -y python3-numpy]
executing command [apt-get install -y qtbase5-dev]
executing command [apt-get install -y cmake]
executing command [apt-get install -y ros-crystal-tinydir-vendor]
executing command [apt-get install -y python3-lark-parser]
executing command [apt-get install -y python3-lxml]
executing command [apt-get install -y libconsole-bridge-dev]
executing command [apt-get install -y libeigen3-dev]
executing command [apt-get install -y pkg-config]
executing command [apt-get install -y libcppunit-dev]
executing command [apt-get install -y libpoco-dev]
executing command [apt-get install -y libpcre3-dev]
executing command [apt-get install -y zlib1g-dev]
executing command [apt-get install -y cppcheck]
executing command [apt-get install -y qt5-qmake]
executing command [apt-get install -y libtinyxml-dev]
executing command [apt-get install -y pyqt5-dev]
executing command [apt-get install -y python3-pyqt5]
executing command [apt-get install -y python3-pyqt5.qtsvg]
executing command [apt-get install -y python3-sip-dev]
executing command [apt-get install -y liblog4cxx-dev]
executing command [apt-get install -y python3-empy]
executing command [apt-get install -y python3-pydot]
executing command [apt-get install -y python3-pygraphviz]
executing command [apt-get install -y python3-psutil]
executing command [apt-get install -y libqt5core5a]
executing command [apt-get install -y libqt5gui5]
executing command [apt-get install -y libqt5opengl5]
executing command [apt-get install -y libqt5widgets5]
executing command [apt-get install -y pydocstyle]
executing command [apt-get install -y libopencv-dev]
executing command [apt-get install -y libfreetype6-dev]
executing command [apt-get install -y libx11-dev]
executing command [apt-get install -y libxaw7-dev]
executing command [apt-get install -y libxrandr-dev]
executing command [apt-get install -y libgl1-mesa-dev]
executing command [apt-get install -y libglu1-mesa-dev]
executing command [apt-get install -y libfreetype6]
executing command [apt-get install -y libtinyxml2-dev]
executing command [apt-get install -y python3-pep8]
executing command [apt-get install -y pyflakes3]
executing command [apt-get install -y libxml2-utils]
executing command [apt-get install -y libyaml-dev]
executing command [apt-get install -y libcurl4-openssl-dev]
executing command [apt-get install -y curl]
executing command [apt-get install -y clang-format]
executing command [apt-get install -y tango-icon-theme]
executing command [apt-get install -y libassimp-dev]
executing command [apt-get install -y uncrustify]
#All required rosdeps installed successfully

Looking a bit more into it, it still installs ros-crystal-tinydir-vendor that is not necessary either.
Adding it to the list in prereqs.yaml will prevent any ros-crystal-* package from being unnecessarily installed. This will mean that the setup.sh file will now be the one from the archive rather than the one installed by ros-crystal-ros-workspace.

Drawback: the archives provide a compiled ros1_bridge so its' setup file will try to source the underlay melodic workspace. It won't prevent the image from building but will print an error message:

not found: "/opt/ros/melodic/local_setup.sh"

Edit:
But I think it's fine as users will not see that message and the resulting image will provides exactly what's in the archive and not files from released packages
Actually sourcing the crystal nightly setup file always prints:

not found: "/opt/ros/melodic/local_setup.bash"
[connext_cmake_module] Warning: The location at which Connext was found when the workspace was built [[/opt/rti.com/rti_connext_dds-5.3.1]] does not point to a valid directory, and the NDDSHOME environment variable has not been set. Support for Connext will not be available.

Reverting 17cc064 for now

Following-up of #245 (comment), we have a couple options:

• use the setup file from the archive, and modify the entry point to source only that workspace and not the chained workspaces (change

  docker_images/ros2/nightly/nightly/ros_entrypoint.sh

  Line 5 in 8404682

  source "/opt/ros/$ROS_DISTRO/setup.bash"

  to source /opt/ros/$ROS_DISTRO/local_setup.bash
  • pro: no sed logic, no error message printed when sourcing, no reliance on released crystal debs
  • con: users entering the container would need to source the local_setup.bash
• use the setup file from the archive but source the non local setup.sh
  • pro: users can source the same file as usual
  • con: need to sed the following files: local_setup.sh/bash(/zsh?) and the setup.sh/bash(/zsh?)
• use the setup file from the released ros_workspace package:
  • pro: no handcrafted sed, users can source the file they want
  • con: need extra layer to install ros-$ROS_DISTRO-ros-workspace after the archive is extracted, relies on setup files from prior released distro

I have a preference for the last option, but am looking for feedback.
Any thoughts? /cc @ruffsl @nuclearsandwich @sloretz

The last option may be tricky during migration, when folks still wish to test nightly, but no released package have been made for that distro yet to install. (aside: when do we bump the ROS_DISTRO env? After packages for the next destor are released?)

I'm not sure I recall all the consequences of using local_setup.bash over setup.bash, but I'd prefer to keep the night image as close to replicating the official ros2 release images if possible. Presently, its simple for folks to swap FROM between using osrf/ros2:nightly to ros:crystal with no other changes to the Dockefile or downstream scripts.

#202 (comment)

If need be, we could push the sed logic down into a script to patch the archive is extracted. Another reason to keep the ros_entrypoint.sh as is, is that there are some downstream use cases the include sed logic to insert their own workspace sourcing. TBH this is a bit fragile, and an alternate approach to address that issue may be worth pursuing.

https://github.com/ros-planning/navigation2/blob/b8311a3c08ebb59894273d939c711dfee6827dad/Dockerfile#L80-L82

to source only that workspace and not the chained workspaces

Out of curiosity, could we could fix this upstream in how the nightly archive is distributed?
/cc @nuclearsandwich

The last option may be tricky during migration, when folks still wish to test nightly, but no released package have been made for that distro yet to install. (aside: when do we bump the ROS_DISTRO env? After packages for the next destor are released?)

As we cannot use the new rosdistro during rosdep invocation before the distro has been added to ros/rosdistro, we have to wait at least that much to bump the env var.
ros_workspace will be the fist package released, so should be available shortly after the distro is added to rosdistro.
My feeling is that the nightly should switch to the new distro as soon as it exists (before the official release), based on this timeline I would aim for April 8 for the switch in the nightly image

I'm not sure I recall all the consequences of using local_setup.bash over setup.bash

local_setup.bash sources only the specified workspace, setup.bash sources the workspace and the underlay workspaces. From http://design.ros2.org/articles/ament.html:

The different shell scripts in the root of the install space are generated by the ament tool. The local_setup.* files only iterate over all packages in the install space (by reading the list of packages in the ament index) and source their package specific setup files. The setup.* files also consider workspaces outside of this install space (by reading the list of parent_prefixp_path in the ament index) and source them before the local_setup.* files.

In standard installations it doesnt make a difference (as all the ros packages are installed under /opt/ros/$ROS_DISTRO. In the case of manual build, it is very common to have multiple workspaces chained and thus sourcing setup.bash should be preferred.

but I'd prefer to keep the night image as close to replicating the official ros2 release images if possible.

The current nightly is pretty far away from the ros:crystal image with respect to its content (it contains packages that are not even included in desktop). It would be more similar to the ros2:source image (that is apparently sourcing local_setup.bash in its entrypoint but I would argue it should be changed to setup.bash in that entry point as well).

Presently, its simple for folks to swap FROM between using osrf/ros2:nightly to ros:crystal with no other changes to the Dockefile or downstream scripts.

Yeah I agree that stay the goal

If need be, we could push the sed logic down into a script to patch the archive is extracted. Another reason to keep the ros_entrypoint.sh as is, is that there are some downstream use cases the include sed logic to insert their own workspace sourcing. TBH this is a bit fragile, and an alternate approach to address that issue may be worth pursuing.

Editing entrypoint in place somehow feels wrong. Though alternativs are for consumers to either provide their own entrypoint, or provide their own scripts at docker run invocation.

Out of curiosity, could we could fix this upstream in how the nightly archive is distributed?

Can you clarify what you're referring to here? The paths in the setup files? or the fact that it tries to source the melodic workspace?

If the former, I agree that it would be good to have a more friendly hardcoded path, then it's hard to pick what it should point to. A way to work around it here would be to override the COLCON_CURRENT_PREFIX environment variable.
Regarding the latter, as the archive provides the ros1_bridge, I believe it makes sense for the setup file to try to source both workspaces (melodic and nightly) needed to be able to use the bridge.

The current nightly is pretty far away from the ros:crystal image with respect to its content (it contains packages that are not even included in desktop).

I'm not so much concerned with added downstream ros2 content, but more about matching at least ros2 ros-core tag environment setup and defaults.

It would be more similar to the ros2:source image (that is apparently sourcing local_setup.bash in its entrypoint but I would argue it should be changed to setup.bash in that entry point as well).

Agreed, we should change source to use setup.bash.

A way to work around it here would be to override the COLCON_CURRENT_PREFIX environment variable.

Could you give an example of that in this case? At first, that doesn't sound to bad as a workaround.

Reverting 17cc064 for now

From your strikeout comments, I'm still not sure why you reverted it.

I'm not so much concerned with added downstream ros2 content, but more about matching at least ros2 ros-core tag environment setup and defaults.

Agreed that's where using the released setup files without modifying them would be preferred IMO

Agreed, we should change source to use setup.bash.

👍 change is now upstream https://github.com/osrf/docker_templates/blob/bf2fe4f280def333aa9a6e34b0c9bb954a27a597/docker_templates/templates/docker_images_ros2/source/ros_entrypoint.sh#L5

From your strikeout comments, I'm still not sure why you reverted it.

I reverted it so that we keep installing at least one package depending on ros_workspace and thus bringing in the setup files from the debs.
The strike out comments was to point out that even with the current sed logic, the setup files from the archive would try to source the melodic workspace resulting in a warning message for all users.
Reverting it allows to keep the image building without changing the templates and add it later on once the template is approved and merged.

Could you give an example of that in this case? At first, that doesn't sound to bad as a workaround.

I believe this is how it's meant to be used to avoid users to have to modify the files. This makes me think there's nothing to change in the setup files from the archive, but just point COLCON_CURRENT_PREFIX to where we extracted the archive.

The original file looks like:

# since a plain shell script can't determine its own path when being sourced
# either use the provided COLCON_CURRENT_PREFIX
# or fall back to the build time prefix (if it exists)
_colcon_prefix_chain_sh_COLCON_CURRENT_PREFIX=/home/jenkins-agent/workspace/packaging_linux/ws/install
if [ ! -z "$COLCON_CURRENT_PREFIX" ]; then
  _colcon_prefix_chain_sh_COLCON_CURRENT_PREFIX="$COLCON_CURRENT_PREFIX"
elif [ ! -d "$_colcon_prefix_chain_sh_COLCON_CURRENT_PREFIX" ]; then
  echo "The build time path \"$_colcon_prefix_chain_sh_COLCON_CURRENT_PREFIX\" doesn't exist. Either source a script for a different shell or set the environment variable \"COLCON_CURRENT_PREFIX\" explicitly." 1>&2
  unset _colcon_prefix_chain_sh_COLCON_CURRENT_PREFIX
  return 1
fi

If we set the COLCON_CURRENT_PREFIX we don't fall back to the build time prefix, so we don't need to change it.

But it will not solve the fact that it'll try to source the underlay melodic workspace

# source chained prefixes
# setting COLCON_CURRENT_PREFIX avoids relying on the build time prefix of the sourced script
COLCON_CURRENT_PREFIX="/opt/ros/melodic"
_colcon_prefix_chain_sh_source_script "$COLCON_CURRENT_PREFIX/local_setup.sh"

For that reason I believe it is better to used the released setup scripts rather than the ones provided in the archive.

Thanks for the informative breakdown!

I reverted it so that we keep installing at least one package depending on ros_workspace and thus bringing in the setup files from the debs.

Should we then explicitly install this ros_workspace package, rather than installing some other dependent package? If that dependent package has changes reflected in the nightly archive, then over-installing with a release package may obscure such changes from the user. Or is this non-issue for tinydir_vendor in particular?

If we set the COLCON_CURRENT_PREFIX we don't fall back to the build time prefix, so we don't need to change it.

👍

For that reason I believe it is better to used the released setup scripts rather than the ones provided in the archive.

👍

Should we then explicitly install this ros_workspace package, rather than installing some other dependent package?

👍 that is the better way of handling it. As this requires to modify the template in general, I didnt make the change in this PR but in osrf/docker_templates#55 (comment) and related #247

Continuing with #247