Pip: Empty and missing environment variables in requirements.txt are not substituted

[adampl]

Environment

• pip version: 20.1.1
• Python version: 3.7.7
• OS: Ubuntu 18.04

Description

Consider you want to install TensorFlow package that comes with 3 variants:

• tensorflow
• tensorflow-gpu
• tensorflow-cpu

Depending on the environment, you want to install either CPU or GPU variant, or the main package by default. So you can add a line in requirements.txt like tensorflow${TF_VARIANT} and it works great if TF_VARIANT is defined and not empty.

However, when the variable is undefined or empty, I get the following error:

ERROR: Invalid requirement: 'tensorflow${TF_VARIANT}' (from line 1 of requirements.txt)

Expected behavior

An empty or missing variable should be substituted with an empty string.

[uranusjr]

pip uses os.path.expandvars under the hood, and this is a documented behaviour of the function. I understand this may feel counterintuitive if you are used to expansion rules of certain shells, but these rules are tool-specific by nature, and whatever pip does can never be perfect. I feel the current behaviour is the most pragmatic choice, and the best we can do is to add this to the documentation (PR welcomed).

[adampl]

I can see only one reference to expandvars in the pip code, but that's not it. Could you point me to the right location in the code?

[deveshks]

I think pip._internal.req.req_file:expand_env_variables is what processes the environment variables in requirement files.

The documentation for it is provided at https://pip.pypa.io/en/latest/reference/pip_install/#using-environment-variables

[adampl]

Thanks. Apparently it doesn't use expandvars anymore. If we just replace this:
https://github.com/pypa/pip/blob/bc6a3ef978d0fe8715185b834fbe68f987fcc763/src/pip/_internal/req/req_file.py#L532-L534

With something like that:

value = os.getenv(var_name) or ""

The problem would go away. What do you think?

[deveshks]

Yep, I tried that and it works.

diff --git a/src/pip/_internal/req/req_file.py b/src/pip/_internal/req/req_file.py
index cde0b08d..463e08f1 100644
--- a/src/pip/_internal/req/req_file.py
+++ b/src/pip/_internal/req/req_file.py
@@ -529,10 +529,7 @@ def expand_env_variables(lines_enum):
     """
     for line_number, line in lines_enum:
         for env_var, var_name in ENV_VAR_RE.findall(line):
-            value = os.getenv(var_name)
-            if not value:
-                continue
-
+            value = os.getenv(var_name) or ""
             line = line.replace(env_var, value)

$ cat requirements.txt 
foo${ENV_VAR}

$ echo $ENV_VAR
$ pip install -r requirements.txt 
ERROR: Could not find a version that satisfies the requirement foo (from -r requirements.txt (line 1)) (from versions: none)
ERROR: No matching distribution found for foo (from -r requirements.txt (line 1))

$ export ENV_VAR=bar
$ echo $ENV_VAR
bar
$ pip install -r requirements.txt 
ERROR: Could not find a version that satisfies the requirement foobar (from -r requirements.txt (line 1)) (from versions: none)
ERROR: No matching distribution found for foobar (from -r requirements.txt (line 1))

[McSinyx]

Bike-shedding a little: we can actually use os.getenv(var_name, ''). IMHO this use case should be supported because the behavior expected here is the default as defined by POSIX, unless -u is set.

[gutsytechster]

If this is a welcoming change, I'd be happy to create a PR for it. :)

[adampl]

@uranusjr Could you please re-evaluate this issue in light of the findings above?

[uranusjr]

IMHO this use case should be supported because the behavior expected here is the default as defined by POSIX, unless -u is set.

OTOH Windows expects missing environment variables to not be replaced. A middle-of-the-road approach would be to replace empty environment variables, but not missing ones. Windows does not support setting empty environment variables (set FOO= simply unsets the FOO environment variable), so this won’t affect Windows usage, while providing a way to replace a variable with an empty value.

[adampl]

OTOH, I don't think anyone would expect the Windows behavior, because Pip doesn't even allow the Windows %VAR% syntax :)

As pointed out in the spec: https://pip.pypa.io/en/stable/reference/pip_install/#using-environment-variables

Note There is no support for other variable expansion syntaxes such as $VARIABLE and %VARIABLE%.

EDIT:

Windows does not support setting empty environment variables

So let's not make any difference in behavior between empty and missing variables.

[sbidoul]

A middle-of-the-road approach would be to replace empty environment variables, but not missing ones

I'm on board with this.

The change must be done here and we'd need a test for that.

[adampl]

@sbidoul But that would make it actually impossible to substitute a variable with an empty string on Windows, right? Also a ${VAR} string is not a valid requirement spec, so no one would benefit from it. Pip is not using Windows syntax, therefore no one should expect the Windows-like behavior. It's the POSIX syntax, and empty string is the default POSIX behavior.

Let's not create another PITA for users, but strive for the greatest productivity and compatibility. That's the way it works in Docker and all other tools I can think of now.

[uranusjr]

My feeling (as a user mainly developing on Windows) to this is entirely the other way around: If you require Windows users to substitute an environment variable with an empty string, your setup is already broken and should be fixed. It is entirely okay to expect the ability to substitute variables with empty strings if you only develop for POSIX users (e.g. in a corporate setting where user platforms are expected), but pip should not step out of the line to “fix” your inability to develop software with cross-platform compatibility.

[sbidoul]

In any case, replacing empty variables has to be done. It is arguably a bug.

What to do wrt missing variables is debatable. @adampl looking at other tools could indeed shed light on the matter.
It could also be interesting to look at the pip history to see if there was a specific reason for not replacing them.

[pfmoore]

IMO, not replacing missing variables makes it easier for the user to identify the problem if they make a typo. There is no common standard behaviour here (POSIX and Windows do very different things) so IMO arguments about "this is the expected behaviour" are not going to resolve the issue.

I'm in favour of not guessing that ${MY_LOCTAION} is meant to be an empty string rather than a typo for ${MY_LOCATION}, and letting the user see the problem when the invalid value appears in an error message.

[adampl]

If we asked Pip users, I bet 99% of them would prefer to be _allowed_ to replace a variable with an empty string. And if only non-empty variables are replaced, then not replacing missing vars will discriminate against Windows users, who cannot set a variable to be empty.

@pfmoore That's a valid point, though still I bet for most people it doesn't justify such strict limitation. Maybe issuing a warning would be a good compromise?