Fix issue #3901: environment variables parse in commands

[Hammond95]

The issue

This fixes #3901

The fix

My code will fix the specific use case reported in the issue, however this code still requires some improvements in my opinion. Before proceeding with these improvement, I think that we should define which use cases this feature should cover.

Some examples:

• do we want to solve variables into the command? ${MYVAR} or $MY_VAR
• do we want to allow the execution of multiple commands if separated by semicolon? echo "hello"; python --version
• do we want to allow multiple command execution with | or && or || operators?

Some possible improvements

• Also add a specific class to manage windows env variables
• manage reserved keywords
• dynamically instantiate the correct class based on the running OS

The checklist

• Associated issue
• A news fragment in the news/ directory to describe this fix with the extension .bugfix, .feature, .behavior, .doc. .vendor. or .trivial (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

[uranusjr]

I’m inclined to use structured input instead:

serve = {
    cmd = "uvicorn mysite:app --reload",
    env = { DEBUG = true },
}

This gets rid of all the parsing and platform-specific behaviours.

Multiple commands can be supported by supplying an array to cmd (or we can introduce a new cmds key instead). A flag can be added to specify how the chain continues after an error occurs (i.e. switch between ;, ||, and &&).

I don’t think it’s a good idea to re-invent Bash command parsing. Just let the user pass in something structured, it’s easier for everyone. (And by everyone I mean including the user. Most people in the world likely don’t use shell syntaxes as a native mental model, so requireing the input to be shell syntax means they also need to “serialise” their thought.)

[Hammond95]

@uranusjr This makes sense, I felt like I was re-inventing the wheel.
Should we also provide retro-compatibility for the commands defined as:
command = 'foo --bar=goo' ?

[uranusjr]

@Hammond95 Definitely. It should continue to exist and work identically to command = { cmd = 'foo --bar=goo' }

[uranusjr]

Oh, I just spotted a mistake in my structured example. Environment variable values must be strings (of course):

serve = {
    cmd = "uvicorn mysite:app --reload",
    env = { DEBUG = 'true' },
}

This brings up an interesting point. We’ll either need to do some introspection before passing the env values to subprocess (IIRC it would crash if any value is not string), or str(value) on the way in. But we’ll need to handle certain values carefully if we go with the str() route: users might not expect true to become True when they receive it in the subprocess.

[Hammond95]

@uranusjr Yeah, that's a good point, I will give a look at this.

Another question:
If we run pipenv run --help we get

Usage: pipenv run [OPTIONS] COMMAND [ARGS]...

  Spawns a command installed into the virtualenv.

Options:
  --python TEXT       Specify which version of Python virtualenv should use.
  --three / --two     Use Python 3/2 when creating virtualenv.
  --clear             Clears caches (pipenv, pip, and pip-tools).  [env var:
                      PIPENV_CLEAR]

  -v, --verbose       Verbose mode.
  --pypi-mirror TEXT  Specify a PyPI mirror.
  -h, --help          Show this message and exit.

Does it make sense to have the options --python, --three/--two, --clear, --pypi-mirror associated with this command?

[techalchemy]

Does it make sense to have the options --python, --three/--two, --clear, --pypi-mirror associated with this command?

I had a separate discussion recently with @ncoghlan about this question. The answer is 'probably not', but most likely we should address that in a separate PR since we have a lot of extra flags and arguments being passed around

[Hammond95]

Not sure if this requires a dedicated issue, but I have also found a possible problem in the file project.py:

In the method:

    def build_script(self, name, extra_args=None):
        try:
            script = Script.parse(self.parsed_pipfile["scripts"][name])
        except KeyError:
            script = Script(name)
        if extra_args:
            script.extend(extra_args)
        return script

in case you provide a non-existing key, pipenv will try to treat the string as a command, so the following will work as expected:

pipenv run python --version

this won't:

pipenv run "python --version"

[techalchemy]

in case you provide a non-existing key, pipenv will try to treat the string as a command, so the following will work as expected:

pipenv run python --version

Yes, This is the desired behavior

this won't:

pipenv run "python --version"

This seems like a bug and it also seems kind of silly that we wouldn't be calling Script.parse() here... based on the commit history it doesn't seem like there is any reason beyond the fact that we also accept extra_args. I went ahead and found this instance in core.py where we are using the code that way (and we also test that functionality):

    try:
        script = project.build_script(command, args)
        cmd_string = ' '.join([script.command] + script.args)
        if environments.is_verbose():
            click.echo(crayons.normal("$ {0}".format(cmd_string)), err=True)
    except ScriptEmptyError:
        click.echo("Can't run script {0!r}-it's empty?", err=True)
    run_args = [script]

I vaguely recall having a conversation with @uranusjr about this and whether there is any reason not to just parse the command all the time. @uranusjr -- can you think of any reason to tackle this a bit differently? As I recall there was a reason...

[uranusjr]

I’m confused. Do you mean pipenv run "python --version" should call python with --version as an argument, or call an executable named "python --version"? I would definitely expect the latter, and the build_script implementation mentioned above looks correct to me.

[Hammond95]

@uranusjr Well maybe you're right and the command provided in quotes shouldn't be run as python with --version argument.

But probably we should avoid inline command execution at all 🤔 and keep that only as a "caller" of the keys defined in the [scripts] section, let me know what you think.

Also in this case the command (pipenv run hello=world python --version) related to the issue, will fail...

[uranusjr]

But probably we should avoid inline command execution at all 🤔 and keep that only as a "caller" of the keys defined in the [scripts] section, let me know what you think.

Honestly, I agree (maybe except I’d keep python). But it’s way too late for this, unfortunately. There are simply too many people relying on this feature, removing it is not an option.