Specifying "append" options from environment variables / configuration files

[McSinyx]

Environment

• pip version: 20.1 and current master
• Python version: I don't think it's relevant
• OS: Debian GNU/Linux, but I don't think it's relevant

Description

I encountered this as I was trying to add tests for a new experimental feature in GH-8532, following the convention of using environmental variable PIP_USE_FEATURE as done for the new resolver, and quickly realized that the optparse append kind of option doesn't immediately makes sense in case of env vars. I thought of having comma-separated lists, but I'm not exactly sure if that is compatible with all of the current use cases:

All options having action of `append`

$ ack -C3 "'append'"
commands/freeze.py-34-        self.cmd_opts.add_option(
commands/freeze.py-35-            '-r', '--requirement',
commands/freeze.py-36-            dest='requirements',
commands/freeze.py:37:            action='append',
commands/freeze.py-38-            default=[],
commands/freeze.py-39-            metavar='file',
commands/freeze.py-40-            help="Use the order in the given requirements file and its "
--
commands/freeze.py-43-        self.cmd_opts.add_option(
commands/freeze.py-44-            '-f', '--find-links',
commands/freeze.py-45-            dest='find_links',
commands/freeze.py:46:            action='append',
commands/freeze.py-47-            default=[],
commands/freeze.py-48-            metavar='URL',
commands/freeze.py-49-            help='URL for finding packages, which will be added to the '
--
commands/wheel.py-65-            '--build-option',
commands/wheel.py-66-            dest='build_options',
commands/wheel.py-67-            metavar='options',
commands/wheel.py:68:            action='append',
commands/wheel.py-69-            help="Extra arguments to be supplied to 'setup.py bdist_wheel'.",
commands/wheel.py-70-        )
commands/wheel.py-71-        self.cmd_opts.add_option(cmdoptions.no_build_isolation())
--
commands/wheel.py-83-        self.cmd_opts.add_option(
commands/wheel.py-84-            '--global-option',
commands/wheel.py-85-            dest='global_options',
commands/wheel.py:86:            action='append',
commands/wheel.py-87-            metavar='options',
commands/wheel.py-88-            help="Extra global options to be supplied to the setup.py "
commands/wheel.py-89-            "call before the 'bdist_wheel' command.")
--
commands/uninstall.py-39-        self.cmd_opts.add_option(
commands/uninstall.py-40-            '-r', '--requirement',
commands/uninstall.py-41-            dest='requirements',
commands/uninstall.py:42:            action='append',
commands/uninstall.py-43-            default=[],
commands/uninstall.py-44-            metavar='file',
commands/uninstall.py-45-            help='Uninstall all the packages listed in the given requirements '
--
cli/cmdoptions.py-2-shared options and groups
cli/cmdoptions.py-3-
cli/cmdoptions.py-4-The principle here is to define options once, but *not* instantiate them
cli/cmdoptions.py:5:globally. One reason being that options with action='append' can carry state
cli/cmdoptions.py-6-between parses. pip parses general options twice internally, and shouldn't
cli/cmdoptions.py-7-pass on state. To be consistent, all options will follow this design.
cli/cmdoptions.py-8-"""
--
cli/cmdoptions.py-285-        type='choice',
cli/cmdoptions.py-286-        choices=['s', 'i', 'w', 'b', 'a'],
cli/cmdoptions.py-287-        default=[],
cli/cmdoptions.py:288:        action='append',
cli/cmdoptions.py-289-        metavar='action',
cli/cmdoptions.py-290-        help="Default action when a path already exists: "
cli/cmdoptions.py-291-             "(s)witch, (i)gnore, (w)ipe, (b)ackup, (a)bort.",
--
cli/cmdoptions.py-331-        '--extra-index-url',
cli/cmdoptions.py-332-        dest='extra_index_urls',
cli/cmdoptions.py-333-        metavar='URL',
cli/cmdoptions.py:334:        action='append',
cli/cmdoptions.py-335-        default=[],
cli/cmdoptions.py-336-        help="Extra URLs of package indexes to use in addition to "
cli/cmdoptions.py-337-             "--index-url. Should follow the same rules as "
--
cli/cmdoptions.py-354-    return Option(
cli/cmdoptions.py-355-        '-f', '--find-links',
cli/cmdoptions.py-356-        dest='find_links',
cli/cmdoptions.py:357:        action='append',
cli/cmdoptions.py-358-        default=[],
cli/cmdoptions.py-359-        metavar='url',
cli/cmdoptions.py-360-        help="If a URL or path to an html file, then parse for links to "
--
cli/cmdoptions.py-383-    return Option(
cli/cmdoptions.py-384-        '-c', '--constraint',
cli/cmdoptions.py-385-        dest='constraints',
cli/cmdoptions.py:386:        action='append',
cli/cmdoptions.py-387-        default=[],
cli/cmdoptions.py-388-        metavar='file',
cli/cmdoptions.py-389-        help='Constrain versions using the given constraints file. '
--
cli/cmdoptions.py-396-    return Option(
cli/cmdoptions.py-397-        '-r', '--requirement',
cli/cmdoptions.py-398-        dest='requirements',
cli/cmdoptions.py:399:        action='append',
cli/cmdoptions.py-400-        default=[],
cli/cmdoptions.py-401-        metavar='file',
cli/cmdoptions.py-402-        help='Install from the given requirements file. '
--
cli/cmdoptions.py-409-    return Option(
cli/cmdoptions.py-410-        '-e', '--editable',
cli/cmdoptions.py-411-        dest='editables',
cli/cmdoptions.py:412:        action='append',
cli/cmdoptions.py-413-        default=[],
cli/cmdoptions.py-414-        metavar='path/url',
cli/cmdoptions.py-415-        help=('Install a project in editable mode (i.e. setuptools '
--
cli/cmdoptions.py-778-    Option,
cli/cmdoptions.py-779-    '--install-option',
cli/cmdoptions.py-780-    dest='install_options',
cli/cmdoptions.py:781:    action='append',
cli/cmdoptions.py-782-    metavar='options',
cli/cmdoptions.py-783-    help="Extra arguments to be supplied to the setup.py install "
cli/cmdoptions.py-784-         "command (use like --install-option=\"--install-scripts=/usr/local/"
--
cli/cmdoptions.py-791-    Option,
cli/cmdoptions.py-792-    '--global-option',
cli/cmdoptions.py-793-    dest='global_options',
cli/cmdoptions.py:794:    action='append',
cli/cmdoptions.py-795-    metavar='options',
cli/cmdoptions.py-796-    help="Extra global options to be supplied to the setup.py "
cli/cmdoptions.py-797-         "call before the install command.",
--
cli/cmdoptions.py-874-    '--path',
cli/cmdoptions.py-875-    dest='path',
cli/cmdoptions.py-876-    type='path',
cli/cmdoptions.py:877:    action='append',
cli/cmdoptions.py-878-    help='Restrict to the specified installation path for listing '
cli/cmdoptions.py-879-         'packages (can be used multiple times).'
cli/cmdoptions.py-880-)  # type: Callable[..., Option]
--
cli/cmdoptions.py-903-    '--unstable-feature',
cli/cmdoptions.py-904-    dest='unstable_features',
cli/cmdoptions.py-905-    metavar='feature',
cli/cmdoptions.py:906:    action='append',
cli/cmdoptions.py-907-    default=[],
cli/cmdoptions.py-908-    choices=['resolver'],
cli/cmdoptions.py-909-    help=SUPPRESS_HELP,  # TODO: drop this in pip 20.3
--
cli/cmdoptions.py-914-    '--use-feature',
cli/cmdoptions.py-915-    dest='features_enabled',
cli/cmdoptions.py-916-    metavar='feature',
cli/cmdoptions.py:917:    action='append',
cli/cmdoptions.py-918-    default=[],
cli/cmdoptions.py-919-    choices=['2020-resolver'],
cli/cmdoptions.py-920-    help='Enable new functionality, that may be backward incompatible.',
--
cli/cmdoptions.py-925-    '--use-deprecated',
cli/cmdoptions.py-926-    dest='deprecated_features_enabled',
cli/cmdoptions.py-927-    metavar='feature',
cli/cmdoptions.py:928:    action='append',
cli/cmdoptions.py-929-    default=[],
cli/cmdoptions.py-930-    choices=[],
cli/cmdoptions.py-931-    help=(
--
cli/parser.py-206-                    )
cli/parser.py-207-                    self.error(error_msg)
cli/parser.py-208-
cli/parser.py:209:            elif option.action == 'append':
cli/parser.py-210-                val = val.split()
cli/parser.py-211-                val = [self.check_default(option, key, v) for v in val]
cli/parser.py-212-            elif option.action == 'callback':

A more thoughtful and future-proof way might be similar to sed's pattern, i.e. use the first and/or last character as the separator, so one may have PIP_USE_FEATURE=,2020-resolver,lazy-wheel,.

^{A bit into implementation details, the current implementation of parsing options from env var and config file gives the values back as a dict, thus the unpack of values needs to be handle immediately before giving these to optparse to use them as a base.}

Expected behavior

A way to make environment variables works for append kind of options, especially ones with value limited to a set of choices.

[pfmoore]

Hmm, that sed-like approach looks incredibly ugly to me. I'd strongly argue for a comma-separated string, until someone shows me a real life example where we need a comma in an option. (At which point I'd probably still argue for comma-separated and try to convince people that the example "isn't that important" 🙂)

Also, I don't think we necessarily need to have the same solution for environment variables and config files. Multiple lines in a config file with the same key are fine, whereas you can't have two environment variables with the same name.

(BTW, I'm not sure I'd follow the environment variable approach that we used for the new resolver when writing new tests. It worked, but it felt pretty clumsy. And I certainly wouldn't consider it as a motivating use case for this feature.)

[McSinyx]

Hmm, that sed-like approach looks incredibly ugly to me.

Ehm, it is 😄 Maybe using trailing separator would looks better than both before and after: PIP_USE_FEATURE=2020-resolver,lazy-wheel,, or PIP_USE_FEATURE=2020-resolver;lazy-wheel;, etc. I've just read the ack's output and it seems currently it's OK to use comma-separated string.

Also, I don't think we necessarily need to have the same solution for environment variables and config files.

I don't think I conveyed the idea quite right (I was sleepy at the time). What I meant is that currently they're handled the same way—stored in a mapping which doesn't support multivalue:

pip/src/pip/_internal/configuration.py

Lines 141 to 143 in 46bd55f

	self._config = {
	variant: {} for variant in self._override_order
	} # type: Dict[Kind, Dict[str, Any]]

[McSinyx]

BTW, I'm not sure I'd follow the environment variable approach that we used for the new resolver when writing new tests. It worked, but it felt pretty clumsy. And I certainly wouldn't consider it as a motivating use case for this feature.

After b46fb6b, I couldn't agree more! Also, may I have your review at GH-8532, on the CI as well as the approach?

[McSinyx]

I've just happen to read the source code handling this part and doubled checked with the reference and it seems pip actually supports this for both config and env vars:

pip/src/pip/_internal/cli/parser.py

Lines 209 to 211 in 8bf5731

	elif option.action == 'append':
	val = val.split()
	val = [self.check_default(option, key, v) for v in val]

I guess it's not a very popular feature and thus no one recalled its existence by the time I filed this issue 😄 I'll go ahead and close this now.