From ad6fbdd4450aebac8c993c1a3af38276f0fc7515 Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Mon, 25 Feb 2019 11:10:23 -0800 Subject: [PATCH 1/2] colcon plugin: support build-time chaining Until recently, colcon had a bug that caused it to not chain workspaces at build-time. That was recently fixed in a way that broke the colcon plugin. Rework the plugin to rely more on the setup files and less on environment variables to avoid this issue in the future. LP: #1816565 Signed-off-by: Kyle Fazzari --- snapcraft/plugins/colcon.py | 74 ++++++++++++------- tests/spread/plugins/colcon/run/task.yaml | 1 - .../snap/snapcraft.yaml | 1 - tests/unit/plugins/test_colcon.py | 71 +++++++++++------- 4 files changed, 94 insertions(+), 53 deletions(-) diff --git a/snapcraft/plugins/colcon.py b/snapcraft/plugins/colcon.py index dcb0743ff3..05bad4afec 100644 --- a/snapcraft/plugins/colcon.py +++ b/snapcraft/plugins/colcon.py @@ -294,17 +294,13 @@ def env(self, root): """Runtime environment for ROS binaries and services.""" env = [ - # The Snapcraft Core will ensure that we get a good LD_LIBRARY_PATH - # overall, but it defines it after this function runs. Some ROS - # tools will cause binaries to be run when we source the setup.sh, - # below, so we need to have a sensible LD_LIBRARY_PATH before then. - 'LD_LIBRARY_PATH="$LD_LIBRARY_PATH:{}"'.format( - formatting_utils.combine_paths( - common.get_library_paths(root, self.project.arch_triplet), - prepend="", - separator=":", - ) - ) + 'AMENT_PYTHON_EXECUTABLE="{}"'.format( + os.path.join(root, "usr", "bin", "python3") + ), + 'COLCON_PYTHON_EXECUTABLE="{}"'.format( + os.path.join(root, "usr", "bin", "python3") + ), + 'SNAP_COLCON_ROOT="{}"'.format(root), ] # Each of these lines is prepended with an `export` when the environment is @@ -408,20 +404,17 @@ def _source_setup_sh(self, root): """ # First, source the upstream ROS underlay if [ -f "{underlay_setup}" ]; then - AMENT_CURRENT_PREFIX="{underlay}" . "{underlay_setup}" + . "{underlay_setup}" fi # Then source the overlay if [ -f "{overlay_setup}" ]; then - COLCON_CURRENT_PREFIX="{overlay}" COLCON_PYTHON_EXECUTABLE="{python_path}" . "{overlay_setup}" + . "{overlay_setup}" fi """ ).format( - underlay=underlaydir, underlay_setup=os.path.join(underlaydir, "setup.sh"), - overlay=overlaydir, overlay_setup=os.path.join(overlaydir, "setup.sh"), - python_path=os.path.join(root, "usr", "bin", "python3"), ) # We need to source ROS's setup.sh at this point. However, it accepts @@ -476,7 +469,7 @@ def _prepare_build(self): mangling.rewrite_python_shebangs(self.installdir) # Rewrite the prefixes to point to the in-part rosdir instead of the system - self._fix_prefixes(self.installdir) + self._fix_prefixes() # Each Colcon package distributes .cmake files so they can be found via # find_package(). However, the Ubuntu packages pulled down as @@ -525,7 +518,7 @@ def _new_path(path): self._rewrite_cmake_paths(_new_path) # Rewrite prefixes for both the underlay and overlay. - self._fix_prefixes("$SNAP") + self._fix_prefixes() # If pip dependencies were installed, generate a sitecustomize that # allows access to them. @@ -534,16 +527,17 @@ def _new_path(path): "3", stage_dir=self.project.stage_dir, install_dir=self.installdir ) - def _fix_prefixes(self, new_prefix): + def _fix_prefixes(self): installdir_pattern = re.compile(r"^{}".format(self.installdir)) + new_prefix = "$SNAP_COLCON_ROOT" def _rewrite_prefix(match): # Group 1 is the variable definition, group 2 is the path, which we may need # to modify. path = match.group(3).strip(" \n\t'\"") - # Bail early if this isn't even a path - if os.path.sep not in path: + # Bail early if this isn't even a path, or if it's already been rewritten + if os.path.sep not in path or new_prefix in path: return match.group() # If the path doesn't start with the installdir, then it needs to point to @@ -556,8 +550,7 @@ def _rewrite_prefix(match): '\\1\\2"{}"\\4'.format(installdir_pattern.sub(new_prefix, path)) ) - # Set the AMENT_CURRENT_PREFIX throughout to the in-snap, run-time prefix - # instead of the build-time prefix. + # Set the AMENT_CURRENT_PREFIX throughout to the in-snap prefix snapcraft.file_utils.replace_in_file( self.installdir, re.compile(r""), @@ -565,12 +558,41 @@ def _rewrite_prefix(match): _rewrite_prefix, ) - # Set any *_COLCON_CURRENT_PREFIX throughout to the in-snap, run-time prefix - # instead of the build-time prefix. + # Set the COLCON_CURRENT_PREFIX (if it's in the installdir) to the in-snap + # prefix + snapcraft.file_utils.replace_in_file( + self.installdir, + re.compile(r""), + re.compile( + r"()(COLCON_CURRENT_PREFIX=)(['\"].*{}.*)()".format(self.installdir) + ), + _rewrite_prefix, + ) + + # Set the _colcon_prefix_sh_COLCON_CURRENT_PREFIX throughout to the in-snap + # prefix + snapcraft.file_utils.replace_in_file( + self.installdir, + re.compile(r""), + re.compile(r"()(_colcon_prefix_sh_COLCON_CURRENT_PREFIX=)(.*)()"), + _rewrite_prefix, + ) + + # Set the _colcon_package_sh_COLCON_CURRENT_PREFIX throughout to the in-snap + # prefix + snapcraft.file_utils.replace_in_file( + self.installdir, + re.compile(r""), + re.compile(r"()(_colcon_package_sh_COLCON_CURRENT_PREFIX=)(.*)()"), + _rewrite_prefix, + ) + + # Set the _colcon_prefix_chain_sh_COLCON_CURRENT_PREFIX throughout to the in-snap + # prefix snapcraft.file_utils.replace_in_file( self.installdir, re.compile(r""), - re.compile(r"()(_COLCON_CURRENT_PREFIX=)(.*)()"), + re.compile(r"()(_colcon_prefix_chain_sh_COLCON_CURRENT_PREFIX=)(.*)()"), _rewrite_prefix, ) diff --git a/tests/spread/plugins/colcon/run/task.yaml b/tests/spread/plugins/colcon/run/task.yaml index b2e3da8a8b..bd3dd022d3 100644 --- a/tests/spread/plugins/colcon/run/task.yaml +++ b/tests/spread/plugins/colcon/run/task.yaml @@ -1,7 +1,6 @@ summary: Build and run a basic colcon snap warn-timeout: 9m # Keep less than 10 minutes so Travis can't timeout priority: 100 # Run this test early so we're not waiting for it -manual: true # LP: #1816565 environment: SNAP_DIR: ../snaps/colcon-talker-listener diff --git a/tests/spread/plugins/colcon/snaps/colcon-talker-listener/snap/snapcraft.yaml b/tests/spread/plugins/colcon/snaps/colcon-talker-listener/snap/snapcraft.yaml index e91e380426..5d29034e5a 100644 --- a/tests/spread/plugins/colcon/snaps/colcon-talker-listener/snap/snapcraft.yaml +++ b/tests/spread/plugins/colcon/snaps/colcon-talker-listener/snap/snapcraft.yaml @@ -6,7 +6,6 @@ description: | grade: stable confinement: strict -base: core18 apps: colcon-talker-listener: diff --git a/tests/unit/plugins/test_colcon.py b/tests/unit/plugins/test_colcon.py index fe453eac33..9bcdabab9d 100644 --- a/tests/unit/plugins/test_colcon.py +++ b/tests/unit/plugins/test_colcon.py @@ -474,10 +474,18 @@ def test_run_environment(self, run_mock): underlay_setup = os.path.join(plugin.options.colcon_rosdistro, "setup.sh") overlay_setup = os.path.join("snap", "setup.sh") - # Verify that LD_LIBRARY_PATH was set before any setup.sh is sourced. Also - # verify that the underlay setup is sourced before the overlay. - ld_library_path_index = [ - i for i, line in enumerate(environment) if "LD_LIBRARY_PATH" in line + # Verify that the python executables and root are set before any setup.sh is + # sourced. Also verify that the underlay setup is sourced before the overlay. + ament_python_index = [ + i for i, line in enumerate(environment) if "AMENT_PYTHON_EXECUTABLE" in line + ][0] + colcon_python_index = [ + i + for i, line in enumerate(environment) + if "COLCON_PYTHON_EXECUTABLE" in line + ][0] + root_index = [ + i for i, line in enumerate(environment) if "SNAP_COLCON_ROOT" in line ][0] underlay_source_setup_index = [ i for i, line in enumerate(environment) if underlay_setup in line @@ -485,7 +493,9 @@ def test_run_environment(self, run_mock): overlay_source_setup_index = [ i for i, line in enumerate(environment) if overlay_setup in line ][0] - self.assertThat(ld_library_path_index, LessThan(underlay_source_setup_index)) + self.assertThat(ament_python_index, LessThan(underlay_source_setup_index)) + self.assertThat(colcon_python_index, LessThan(underlay_source_setup_index)) + self.assertThat(root_index, LessThan(underlay_source_setup_index)) self.assertThat( underlay_source_setup_index, LessThan(overlay_source_setup_index) ) @@ -505,12 +515,10 @@ def test_source_setup_sh(self): lines_of_interest = [ "set --", 'if [ -f "{}" ]; then'.format(underlay_setup), - 'AMENT_CURRENT_PREFIX="{}" . "{}"'.format(underlay, underlay_setup), + '. "{}"'.format(underlay_setup), "fi", 'if [ -f "{}" ]; then'.format(overlay_setup), - 'COLCON_CURRENT_PREFIX="{}" COLCON_PYTHON_EXECUTABLE="{}" . "{}"'.format( - overlay, python_path, overlay_setup - ), + '. "{}"'.format(overlay_setup), "fi", 'eval "set -- $BACKUP_ARGS"', ] @@ -593,16 +601,12 @@ def test_ament_prefix_is_rewritten(self): { "path": "setup.sh", "contents": ": ${AMENT_CURRENT_PREFIX:=/opt/ros/crystal}", - "expected": ': ${{AMENT_CURRENT_PREFIX:="{}/opt/ros/crystal"}}'.format( - self.plugin.installdir - ), + "expected": ': ${AMENT_CURRENT_PREFIX:="$SNAP_COLCON_ROOT/opt/ros/crystal"}', }, { "path": "test/local_setup.sh", - "contents": ": ${AMENT_CURRENT_PREFIX:=/foo/bar}", - "expected": ': ${{AMENT_CURRENT_PREFIX:="{}/foo/bar"}}'.format( - self.plugin.installdir - ), + "contents": ': ${AMENT_CURRENT_PREFIX:="/foo/bar"}', + "expected": ': ${AMENT_CURRENT_PREFIX:="$SNAP_COLCON_ROOT/foo/bar"}', }, { "path": "test/no-change.sh", @@ -696,14 +700,14 @@ def test_ament_prefix_is_rewritten(self): { "path": "setup.sh", "contents": ": ${AMENT_CURRENT_PREFIX:=/opt/ros/crystal}", - "expected": ': ${AMENT_CURRENT_PREFIX:="$SNAP/opt/ros/crystal"}', + "expected": ': ${AMENT_CURRENT_PREFIX:="$SNAP_COLCON_ROOT/opt/ros/crystal"}', }, { "path": "test/local_setup.sh", "contents": ': ${{AMENT_CURRENT_PREFIX:="{}/foo/bar"}}'.format( self.plugin.installdir ), - "expected": ': ${AMENT_CURRENT_PREFIX:="$SNAP/foo/bar"}', + "expected": ': ${AMENT_CURRENT_PREFIX:="$SNAP_COLCON_ROOT/foo/bar"}', }, { "path": "test/no-change.sh", @@ -731,21 +735,38 @@ def test_colcon_prefix_is_rewritten(self): files = [ { "path": "setup.sh", - "contents": "_colcon_prefix_chain_sh_COLCON_CURRENT_PREFIX=/foo/bar", - "expected": '_colcon_prefix_chain_sh_COLCON_CURRENT_PREFIX="$SNAP/foo/bar"', + "contents": "_colcon_prefix_sh_COLCON_CURRENT_PREFIX=/foo/bar", + "expected": '_colcon_prefix_sh_COLCON_CURRENT_PREFIX="$SNAP_COLCON_ROOT/foo/bar"', }, { "path": "test/local_setup.sh", - "contents": '_colcon_prefix_sh_COLCON_CURRENT_PREFIX="{}/baz"'.format( + "contents": '_colcon_package_sh_COLCON_CURRENT_PREFIX="{}/baz"'.format( self.plugin.installdir ), - "expected": '_colcon_prefix_sh_COLCON_CURRENT_PREFIX="$SNAP/baz"', + "expected": '_colcon_package_sh_COLCON_CURRENT_PREFIX="$SNAP_COLCON_ROOT/baz"', }, { - "path": "test/no-change.sh", + "path": "test/another_local_setup.sh", + "contents": 'COLCON_CURRENT_PREFIX="{}/qux"'.format( + self.plugin.installdir + ), + "expected": 'COLCON_CURRENT_PREFIX="$SNAP_COLCON_ROOT/qux"', + }, + { + "path": "test/no-change1.sh", "contents": "_colcon_prefix_chain_sh_COLCON_CURRENT_PREFIX=$NOT_A_PATH", "expected": "_colcon_prefix_chain_sh_COLCON_CURRENT_PREFIX=$NOT_A_PATH", }, + { + "path": "test/no-change2.sh", + "contents": "_colcon_package_bash_COLCON_CURRENT_PREFIX=/foo/bar", + "expected": "_colcon_package_bash_COLCON_CURRENT_PREFIX=/foo/bar", + }, + { + "path": "test/no-change3.sh", + "contents": "COLCON_CURRENT_PREFIX=/foo/bar", + "expected": "COLCON_CURRENT_PREFIX=/foo/bar", + }, ] for file_info in files: @@ -768,14 +789,14 @@ def test_colcon_python_executable_is_rewritten(self): { "path": "setup.sh", "contents": "_colcon_python_executable=/usr/bin/python3", - "expected": '_colcon_python_executable="$SNAP/usr/bin/python3"', + "expected": '_colcon_python_executable="$SNAP_COLCON_ROOT/usr/bin/python3"', }, { "path": "test/local_setup.sh", "contents": '_colcon_python_executable="{}/baz/python3"'.format( self.plugin.installdir ), - "expected": '_colcon_python_executable="$SNAP/baz/python3"', + "expected": '_colcon_python_executable="$SNAP_COLCON_ROOT/baz/python3"', }, { "path": "test/no-change.sh", From 2cfab7c844d6af9b9270662dbc18227940eaea30 Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Mon, 25 Feb 2019 14:53:26 -0800 Subject: [PATCH 2/2] Fix static Signed-off-by: Kyle Fazzari --- snapcraft/plugins/colcon.py | 2 +- tests/unit/plugins/test_colcon.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/snapcraft/plugins/colcon.py b/snapcraft/plugins/colcon.py index 05bad4afec..8e1f27b1d8 100644 --- a/snapcraft/plugins/colcon.py +++ b/snapcraft/plugins/colcon.py @@ -62,7 +62,7 @@ import snapcraft from snapcraft.plugins import _ros from snapcraft.plugins import _python -from snapcraft import common, file_utils, formatting_utils, repo +from snapcraft import file_utils, repo from snapcraft.internal import errors, mangling logger = logging.getLogger(__name__) diff --git a/tests/unit/plugins/test_colcon.py b/tests/unit/plugins/test_colcon.py index 9bcdabab9d..847291bdfb 100644 --- a/tests/unit/plugins/test_colcon.py +++ b/tests/unit/plugins/test_colcon.py @@ -509,7 +509,6 @@ def test_source_setup_sh(self): underlay_setup = os.path.join(underlay, "setup.sh") overlay = os.path.join("test-root", "opt", "ros", "snap") overlay_setup = os.path.join(overlay, "setup.sh") - python_path = os.path.join("test-root", "usr", "bin", "python3") # Make sure $@ is zeroed, then setup.sh sourced, then $@ is restored lines_of_interest = [