allow multiple package names for --only-packages #114

dirk-thomas · 2016-11-04T23:09:57Z

Extract checking the passed arguments for sane values and combinations.
Enables multiple package names to be passed to --only-packages.
Consolidates all the criteria which packages should be processed in a single attribute (start_with, end_with, only_packages are merged into skip_packages).
Printing the topological order as well as the actual iteration over the packages then uses the consolidated result without the need to reimplement the logic.

mikaelarguedas · 2016-11-04T23:18:03Z

Works fine locally, that's great! will save us quite some time
Question: Is there a rationale for being able to skip-package the package provides to --end-with but not one of the packages provided to --only-packages. Not saying there is a use case there but it's surprising that the behaviour is different while both packages are explicitely listed in commandline as "to be built"

dirk-thomas · 2016-11-04T23:23:23Z

Listing a single pkg name in --only-packages as well as --skip-packages is contracting each other.

Using start-with / --end-with together with --skip-packages is no contradiction and can be reasonable (e.g. start with D, end with H but skip F).

mikaelarguedas · 2016-11-04T23:26:42Z

Yeah I'm just surprised that I can run --start-with A --end-with A --skip-package A but I see your point

dirk-thomas · 2016-11-04T23:29:29Z

When you consider a future option --deps it makes sense to ignore the starting package.

mikaelarguedas · 2016-11-04T23:32:21Z

Fair point, we should just explain it somewhere in a future "ament tutorial" so that people are not confused by it.

dirk-thomas · 2016-11-07T18:11:21Z

After talking about the various case more I updated the patch to only output a warning if all selected packages are also being skipped.

mikaelarguedas

Thanks for iterating so quickly on it

Should we propagate the same changes to https://github.com/ament/ament_tools/blob/master/ament_tools/verbs/uninstall/cli.py#L64 for consistency in parameters among verbs ?

mikaelarguedas · 2016-11-07T18:29:21Z

ament_tools/verbs/build/cli.py

@@ -114,13 +114,13 @@ def prepare_arguments(parser, args):
        help='End with a particular package',
    )
    parser.add_argument(
-        '--only-package',
-        '--only',


Do we want to keep the --only alias for convenience ?

Any shorter but unique variation of arguments is always available. That's also why renaming it from --only-package to --only-packages is backward compatible.

I agree, but it won't be displayed in the help message anymore if it is removed here.

That's true, but neither do many of the other arguments have that kind of alternative explicit shortcut. If more people want to keep it please feel free to readd it.

it will be an extra option that we have to maintain auto-complete for so I'd support leaving it off the list

Sounds good, let's leave it out then

dhood · 2016-11-07T18:33:10Z

ament_tools/verbs/build/cli.py

-        help='Only process a particular package, implies --start-with <pkg> and --end-with <pkg>'
+        '--only-packages',
+        nargs='*', default=[],
+        help='Only process a particular set of packages'


'List of packages to process' would be more consistent with the --skip-packages help (and makes it clear it's not a flag)

I updated the existing help string to use set too.

I meant that the whole phrase makes it sounds like a boolean flag to me. but the array "type" in the help output will make it clearer

dhood

The base path is listed as the last option in the help output. But if I use --only-packages I cannot put the base path as the last option.

dhood@osrf-esteve:~/ros2_ws [ros2_ws]$ ament build --only-packages rcl ./src
Package './src' specified with --only-package was not found.

.2. What is the expected behaviour of passing an empty list? I would have expected that no work is done, but instead the whole workspace is built.

.3. Tab autocomplete needs to be updated

dhood · 2016-11-07T18:43:35Z

ament_tools/verbs/build/cli.py

@@ -114,13 +114,13 @@ def prepare_arguments(parser, args):
        help='End with a particular package',
    )
    parser.add_argument(
-        '--only-package',
-        '--only',


it will be an extra option that we have to maintain auto-complete for so I'd support leaving it off the list

dhood · 2016-11-07T18:44:34Z

ament_tools/verbs/build/cli.py

-
-    if opts.only_package:
+    nonexistent_skip_packages = set(opts.skip_packages) - set(package_names)
+    if set(opts.skip_packages) - set(package_names):


we can use the nonexistent_skip_packages variable here instead of recalculating it

dhood · 2016-11-07T18:49:25Z

ament_tools/verbs/build/cli.py

+
+def consolidate_package_selection(opts, package_names):
+    # after this function opts.skip_packages will contain the information from:
+    # start_package, end_package, only_packages


start_with, end_with?

Sorry, documentation from a different tool which uses different argument names 😉 Updated.

dirk-thomas · 2016-11-07T19:22:16Z

Since --only-packages is a greedy argument you need to insert -- before passing other arguments behind it.
I changed the argument to nargs='+' to enforce passing at least one name.
Updated completion.

dhood · 2016-11-07T20:01:48Z

I am still finding the greedy behaviour a bit strange when combined with --skip-packages and specifying the source path.

This works fine:

dhood@osrf-esteve:~/ros2_ws [ros2_ws]$ ament build --only-packages rclcpp rcl --skip-packages rcl -- ./src

But this does not:

dhood@osrf-esteve:~/ros2_ws [ros2_ws]$ ament build --only-packages rclcpp rcl -- --skip-packages rcl -- ./src
usage: ament build [-h] [-C DIRECTORY] [--build-space BUILD_SPACE]
                   [--install-space INSTALL_SPACE] [--build-tests]
                   [--make-flags [MAKE_FLAGS [MAKE_FLAGS ...]]] [--skip-build]
                   [--skip-install] [-s] [--isolated]
                   [--start-with START_WITH] [--end-with END_WITH]
                   [--only-packages ONLY_PACKAGES [ONLY_PACKAGES ...]]
                   [--skip-packages [SKIP_PACKAGES [SKIP_PACKAGES ...]]]
                   [--parallel] [--force-ament-cmake-configure]
                   [--ament-cmake-args [AMENT_CMAKE_ARGS [AMENT_CMAKE_ARGS ...]]]
                   [--force-cmake-configure]
                   [--cmake-args [CMAKE_ARGS [CMAKE_ARGS ...]]]
                   [--ctest-args [CTEST_ARGS [CTEST_ARGS ...]]]
                   [basepath]
ament build: error: argument basepath: Path '--skip-packages' does not exist

dirk-thomas · 2016-11-07T20:04:20Z

The -- tells argparse that the next argument is a positional argument. But the next one is --skip-packages so you must not precede it with -- in that case.

dhood · 2016-11-07T22:39:50Z

I see your point about -- telling argparse that the optionals are done, and that what follows is the positional arguments. I agree that this is the expected behaviour in the context of argparse, and that I should not expect to use both --only-packages and --skip-packages with the delimiter.

This was unexpected for me because of the difference in how other optionals such as --cmake-args have to be passed.

To illustrate, here's a valid invocation for passing cmake args, only-packages, skip-packages and specifying the basepath:

~/ros2_ws$ ament build --cmake-args -DCMAKE_BUILD_TYPE=Debug -- --only-package rclcpp --skip-packages rcl rclcpp -- ./src

You'll see that, while more optionals come after --cmake-args, we have to pass the delimiter with it, since we are parsing the delimiter with extract_argument_group, not argparse.

This is why I figured you'd have to do the same for ending argument collection for --only-package in my previous comment.

I wouldn't have gotten into this case if [basepath] was listed first in the usage help output (as opposed to last). But I think people will be able to figure it out (I was just trying to break the parsing 😄 )

dirk-thomas added the in review Waiting for review (Kanban column) label Nov 4, 2016

dirk-thomas self-assigned this Nov 4, 2016

dirk-thomas force-pushed the multiple_only branch from 576265a to e511428 Compare November 7, 2016 18:10

mikaelarguedas reviewed Nov 7, 2016

View reviewed changes

dhood reviewed Nov 7, 2016

View reviewed changes

dirk-thomas force-pushed the multiple_only branch from e511428 to abe5939 Compare November 7, 2016 18:34

dhood suggested changes Nov 7, 2016

View reviewed changes

dirk-thomas force-pushed the multiple_only branch from abe5939 to 767f1e3 Compare November 7, 2016 19:22

dhood approved these changes Nov 7, 2016

View reviewed changes

allow multiple package names for --only-packages

d0ae9d6

dirk-thomas force-pushed the multiple_only branch from e7ec15d to d0ae9d6 Compare November 7, 2016 22:47

dirk-thomas merged commit 78d74ed into master Nov 7, 2016

dirk-thomas deleted the multiple_only branch November 7, 2016 22:49

dirk-thomas removed the in review Waiting for review (Kanban column) label Nov 7, 2016

dirk-thomas mentioned this pull request Nov 8, 2016

end --only-packages with -- ros2/ci#10

Merged

dhood mentioned this pull request Nov 9, 2016

uninstall verb fails #122

Closed

mikaelarguedas mentioned this pull request Aug 4, 2017

option to build only a package and its dependencies #155

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow multiple package names for --only-packages #114

allow multiple package names for --only-packages #114

dirk-thomas commented Nov 4, 2016

mikaelarguedas commented Nov 4, 2016

dirk-thomas commented Nov 4, 2016

mikaelarguedas commented Nov 4, 2016

dirk-thomas commented Nov 4, 2016

mikaelarguedas commented Nov 4, 2016

dirk-thomas commented Nov 7, 2016

mikaelarguedas left a comment

mikaelarguedas Nov 7, 2016

dirk-thomas Nov 7, 2016 •

edited

Loading

mikaelarguedas Nov 7, 2016

dirk-thomas Nov 7, 2016

dhood Nov 7, 2016

mikaelarguedas Nov 7, 2016

dhood Nov 7, 2016

dirk-thomas Nov 7, 2016

dhood Nov 7, 2016

dhood left a comment •

edited

Loading

dhood Nov 7, 2016

dhood Nov 7, 2016

dirk-thomas Nov 7, 2016

dhood Nov 7, 2016

dirk-thomas Nov 7, 2016

dirk-thomas commented Nov 7, 2016

dhood commented Nov 7, 2016

dirk-thomas commented Nov 7, 2016

dhood commented Nov 7, 2016

allow multiple package names for --only-packages #114

allow multiple package names for --only-packages #114

Conversation

dirk-thomas commented Nov 4, 2016

mikaelarguedas commented Nov 4, 2016

dirk-thomas commented Nov 4, 2016

mikaelarguedas commented Nov 4, 2016

dirk-thomas commented Nov 4, 2016

mikaelarguedas commented Nov 4, 2016

dirk-thomas commented Nov 7, 2016

mikaelarguedas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhood left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented Nov 7, 2016

dhood commented Nov 7, 2016

dirk-thomas commented Nov 7, 2016

dhood commented Nov 7, 2016

dirk-thomas Nov 7, 2016 •

edited

Loading

dhood left a comment •

edited

Loading