Skip to content

Commit

Permalink
Remove extra quote in bash invocation (#15)
Browse files Browse the repository at this point in the history
Currently, we use the following bash invocation to run the linter:

ament_<linter> $(colcon list --packages-select 'a b c')

The use of "colcon list" is to convert the list of package names into
packages paths that the linter expects.
I.e. we can run ament_copyright /path/to/pkg/a /path/to/pkg/b, but not
ament_copyright a b.

This fails because colcon list is looking for a single package called 'a b c'.
As there is no package called 'a b c'. What we want is instead selecting
package, "a", "b", and "c". This can be done by removing the single quotes
around the package list:

ament_<linter> $(colcon list --packages-select a b c)

Unfortunately, "colcon list" returns 0 (success), even if it fails to
match anything. Therefore, there is no easy way to detect this issue
with the current implementation. An alternative solution could be to
parse stderr, which is cumbersome, and brittle. For now, we chose to
only fix the issue, without introducing a test or a safety mechanism to
prevent regressions from happening.

Another problem is that ament_<linter> "/path/to/a /path/to/b" fails due
to the same issue, those extra quotes have also been removed.

Finally, this fixes the test. Until now, the test was not executed
properly, but it was never noticed because colcon fails silently
if no packages are matched. This requires the introduction of a new
parameter workspace-director allowing the linters to rely on a colcon
workspace located into a custom directory.

Signed-off-by: Thomas Moulard <tmoulard@amazon.com>
  • Loading branch information
Thomas Moulard authored Dec 4, 2019
1 parent ee441c7 commit cd8ee0c
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 8 deletions.
6 changes: 4 additions & 2 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@ jobs:
runs-on: ubuntu-18.04
steps:
- uses: actions/checkout@v1

- run: npm ci
- run: npm run build

- run: |
git clone --depth=1 --branch=0.8.1 \
https://github.com/ament/ament_lint.git
- uses: ros-tooling/setup-ros2@0.0.3
- uses: ./ # Uses an action in the root directory
with:
package-name: ament_copyright ament_lint
workspace-directory: ament_lint
3 changes: 3 additions & 0 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ inputs:
Passing multiple package names is allowed.
Package names can be separated by any whitespace character.
required: true
workspace-directory:
description: Linters will look for packages in this colcon workspace
required: false
runs:
using: 'node12'
main: 'dist/index.js'
6 changes: 3 additions & 3 deletions dist/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -973,9 +973,9 @@ function run() {
console.log(`##[add-matcher]${path.join(matchersPath, "ament_flake8.json")}`);
const linterTool = core.getInput("linter");
const packageName = core.getInput("package-name", { required: true });
const packageNameList = packageName.split(RegExp('\\s'));
const packageNameList = packageName.split(RegExp("\\s"));
const ros2Distribution = core.getInput("distribution");
const ros2WorkspaceDir = process.env.GITHUB_WORKSPACE;
const ros2WorkspaceDir = core.getInput("workspace-directory") || process.env.GITHUB_WORKSPACE;
yield exec.exec("rosdep", ["update"]);
yield runAptGetInstall([`ros-${ros2Distribution}-ament-${linterTool}`]);
const options = {
Expand All @@ -987,7 +987,7 @@ function run() {
yield exec.exec("bash", [
"-c",
`source /opt/ros/${ros2Distribution}/setup.sh && ` +
`ament_${linterTool} "$(colcon list --packages-select '${packageNameList.join(' ')}' -p)"`
`ament_${linterTool} $(colcon list --packages-select ${packageNameList.join(" ")} -p)`
], options);
}
catch (error) {
Expand Down
7 changes: 4 additions & 3 deletions src/action-ros2-lint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ async function run() {
const packageName = core.getInput("package-name", { required: true });
const packageNameList = packageName.split(RegExp("\\s"));
const ros2Distribution = core.getInput("distribution");
const ros2WorkspaceDir = process.env.GITHUB_WORKSPACE;
const ros2WorkspaceDir =
core.getInput("workspace-directory") || process.env.GITHUB_WORKSPACE;

await exec.exec("rosdep", ["update"]);

Expand All @@ -49,9 +50,9 @@ async function run() {
[
"-c",
`source /opt/ros/${ros2Distribution}/setup.sh && ` +
`ament_${linterTool} "$(colcon list --packages-select '${packageNameList.join(
`ament_${linterTool} $(colcon list --packages-select ${packageNameList.join(
" "
)}' -p)"`
)} -p)`
],
options
);
Expand Down

0 comments on commit cd8ee0c

Please sign in to comment.