search

osrf / rocker

A tool to run docker containers with overlays and convenient options for things like GUIs etc.
Apache License 2.0
559 stars 73 forks source link

Volume argument parsing change proposal #166

Closed marco-perin closed 2 years ago

marco-perin commented 2 years ago

This is a proposal, as the the README.md actually specifies that the last path should be terminated with --, but I missed that. This made me lose quite a bit of time understanding what was going on since I was trying to bind volumes as follows:

rocker --nvidia --x11 --name iaslab --volume "$HOME/my/folder:/container/folder" user/image
Extension volume doesn't support default arguments. Please extend it.
usage: rocker [-h] [--noexecute] [--nocache] [--nocleanup] [--pull] [-v] [--dev-helpers]
              [--devices [DEVICES [DEVICES ...]]] [--env NAME[=VALUE] [NAME[=VALUE] ...]]
              [--env-file ENV_FILE] [--git] [--git-config-path GIT_CONFIG_PATH] [--home]
              [--name NAME] [--network {host,none,bridge}] [--nvidia] [--privileged]
              [--pulse] [--ssh] [--user] [--user-override-name USER_OVERRIDE_NAME]
              [--user-preserve-home] [--volume HOST-DIR[:CONTAINER-DIR[:OPTIONS]]
              [HOST-DIR[:CONTAINER-DIR[:OPTIONS]] ...]] [--x11]
              [--mode {interactive,non-interactive,dry-run}] [--image-name IMAGE_NAME]
              [--extension-blacklist [EXTENSION_BLACKLIST [EXTENSION_BLACKLIST ...]]]
              image [command [command ...]]
rocker: error: the following arguments are required: image

The argument parser for the --volume flag, being configured with nargs='+', was causing the --volume directive to eat up the image positional argument if used as last optional one (also see this StackOverflow issue). Adopting this change makes the configuration of volumes so that each bind is specified with its own --volume flag.

I think that this approach is slightly better (and also more inline with the way the Docker CLI handles it). If changing the code is not possible or wanted, I propose to document the need for -- in the rocker --help message.

Co-authored with @gdelazzari

tfoote commented 2 years ago

Requiring repeating the --volume argument makes the command much more verbose. And the use of -- is a standard of the argparse tool. This is something that's standard upstream. If you're using any nargs=+ or nargs=?' arguments this can be an ambiguity. If you use the--volumeargument before any other argument that starts with a--` this will not be necessary.

I believe that most of the argument parsing is automated. But if you can find a way to detect that there's a nargs argument at the end without any positional arguments then it likely is something that would be helpful to the user and potentially even submitted upstream for standard processing. In general I don't want to start doing custom processing of command line arguments.

marco-perin commented 2 years ago

Got it, closing this and will open another PR to improve the help section