More granular control over PIPENV_VENV_IN_PROJECT variable. #5026

matteius · 2022-03-30T22:54:24Z

Allow PIPENV_VENV_IN_PROJECT to be read in as None, and ensure if it is set to False that it does not use .venv directory.

The issue

Fixes #2763

The fix

The variable was being cast to a boolean so it would be False even if it was not set, this allows a user to explicitly set the variable to a non-truthy value and ensure that the .venv directory gets ignored.

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 #.

…is set to False that it does not use .venv directory.

smac89 · 2022-03-30T23:55:39Z

@matteius I'm not able to test atm, but from a casual glance, it looks like that should fix the issue.

Moreover, I think switching from booleans to constants may be a better approach to avoid confusion (but this may introduce breakages):

I propose the following values:

enable
disable
auto

The PIPENV_VENV_IN_PROJECT environment variable can take any of these values, and when it is set to auto (which should be the default), then pipenv will behave the way it currently does.

Actually, this may not break anything, because enable and disable may just be more aliases for truthy and falsey.

Anyways, yea that looks like a good fix for the issue

smac89 · 2022-03-30T23:58:00Z

pipenv/environments.py

+        self.PIPENV_VENV_IN_PROJECT = os.environ.get("PIPENV_VENV_IN_PROJECT")
+        if self.PIPENV_VENV_IN_PROJECT is not None:
+            self.PIPENV_VENV_IN_PROJECT = self.PIPENV_VENV_IN_PROJECT.lower() in (
+                "true",


Note that some of these are already defined above with _true_values

…from other tests.

matteius · 2022-03-31T04:01:47Z

news/4992.process.rst

@@ -1 +0,0 @@
-Internal to pipenv, the utils.py was split into a utils module with unused code removed.


I duplicated this news fragment in main.

matteius · 2022-03-31T04:02:43Z

tests/integration/test_pipenv.py

@@ -92,14 +92,9 @@ def test_proper_names_unamanged_virtualenv(PipenvInstance):
 def test_directory_with_leading_dash(raw_venv, PipenvInstance):
    with temp_environ():
        with PipenvInstance(chdir=True, venv_in_project=False, name="-project-with-dash") as p:
-            if "PIPENV_VENV_IN_PROJECT" in os.environ:


These tests aren't about this variable and shouldn't impact the test run since it use with temp_environ().

matteius · 2022-03-31T04:03:01Z

tests/integration/test_install_basic.py

@@ -405,8 +405,6 @@ def test_install_venv_project_directory(PipenvInstance):
            prefix="pipenv-", suffix="temp_workon_home"
        ) as workon_home:
            os.environ["WORKON_HOME"] = workon_home
-            if "PIPENV_VENV_IN_PROJECT" in os.environ:


These tests aren't about this variable and shouldn't impact the test run since it use with temp_environ().

matteius · 2022-03-31T04:03:22Z

tests/integration/test_pipenv.py

            c = p.pipenv('run pip freeze')
            assert c.returncode == 0
            c = p.pipenv('--venv')
            assert c.returncode == 0
            venv_path = c.stdout.strip()
            assert os.path.isdir(venv_path)
-            # Manually clean up environment, since PipenvInstance assumes that
-            # the virutalenv is in the project directory.
-            p.pipenv('--rm')


None of the other tests do this kind of cleanup.

matteius · 2022-03-31T22:20:57Z

I made some updates and made it safer with respect to the allowed true and false values. If we want to expand that list at some point to include enable(d)/disable(d) we can but I mostly just wanted to put out a patch for the issue at hand.

pipenv/environments.py

oz123 · 2022-04-06T18:33:28Z

pipenv/environments.py

+            if self.PIPENV_VENV_IN_PROJECT.lower() in _true_values:
+                self.PIPENV_VENV_IN_PROJECT = True
+            elif self.PIPENV_VENV_IN_PROJECT.lower() in _false_values:
+                self.PIPENV_VENV_IN_PROJECT = False


Why does one need both False and None here? I see you documented it, but why do we need this feature?
If this is only for backward compatibility, I suggest considering supporting it but also to issue a warning that only false
should be used in the future.
If someone needs .venv they could specify it specifically. Otherwise, IMHO .venv is like magic. It's hidden at first when you ls a directory, and this behavior is kind of opposite to Python's own: Explicit it better than implicit.
This is a great principle for any software project too...

None indicates that you haven't set the variable, so in this case the behavior is use the .venv if it is present. However if you want to definitely ignore a present .venv you would se it to be False. This is helpful for those doing development locally and building docker containers, that sometimes the .venv directory exists locally and gets copied into their container but is different system installed versions than the OS the container is running on. They want to be able to explicitly state in the .env not to ever use a .venv directory in the project, even if it exists because it got copied into the container. For additional context: #2763 (comment)

if you want to definitely ignore a present .venv you would se it to be False.

I thought this should be the default, always ignore. Even if the variable isn't set.

A solution to @smac89's problem could done with .dockerignore. They probably have something like this:
in project source:

$ ls -a setup.py Pipfile Pipfile.lock src/<many-python-files-here> .venv Dockerfile

Now, they (naively) do something in Dockerfile

ADD . /usr/src/app WORKDIR /usr/src/app RUN pipenv install

And viola, this thing is broken, because it uses an existing venv. The solution is to create .dockerignore file, and then add .venv in it. This will prevent the ADD directive from copying .venv into the docker container.

The real problem is however, the decision to default to the name .venv. This should have been named either pipenv venv without the leading dot.

As I mentioned in my comment, we use docker-compose, and as far as I can tell, docker-compose does not give one the option of filtering out certain files from a mounted volume. Therefore, once we mount the volume inside the build container, it includes everything in that volume.

.dockerignore only comes into play when building a container. We don't build containers during development because that would just be a massive waste of time.

NeroVanbiervliet · 2022-04-12T09:19:39Z

This change is not 100% backwards compatible and broke one of our docker builds:

# Dockerfile
RUN PIPENV_VENV_IN_PROJECT="enabled" pipenv sync

Previously, "enabled" was parsed as True, but with this change, you need to supply one of _true_values for it to work.

NeroVanbiervliet · 2022-04-12T09:24:52Z

From a quick search on github it seems that most people are using 1 or true as a way to set the variable. So they should be fine :)

markmiscavage · 2022-04-12T23:02:59Z

This change is not 100% backwards compatible and broke one of our docker builds:
# Dockerfile
RUN PIPENV_VENV_IN_PROJECT="enabled" pipenv sync
Previously, "enabled" was parsed as True, but with this change, you need to supply one of _true_values for it to work.

We were also using "enabled" and this broke all our project builds (40+ projects in Bitbucket) and took me a long while to debug as the error I was initially receiving was Error: the command gunicorn could not be found within PATH or Pipfile's [scripts]. on container start, rather than any issues during container build.

oz123 · 2022-04-12T23:30:21Z

While this is certainly annoying that so many builds are broken, I think this 'enabled' work was wrong to use. This probably would also work if you wrote 'disabled', since the previous code only checked if the variable was set, not if it was truthy.

>>> import os
>>> bool(os.environ.get("PIPENV_VENV_IN_PROJECT", "disabled"))
True
>>>

This was an accident waiting to happen. Sorry, for breaking your builds, maybe we could have done this in a more graceful way.

smac89 · 2022-04-13T20:53:26Z

This change is not 100% backwards compatible and broke one of our docker builds:
# Dockerfile
RUN PIPENV_VENV_IN_PROJECT="enabled" pipenv sync
Previously, "enabled" was parsed as True, but with this change, you need to supply one of _true_values for it to work.
We were also using "enabled" and this broke all our project builds (40+ projects in Bitbucket) and took me a long while to debug as the error I was initially receiving was Error: the command gunicorn could not be found within PATH or Pipfile's [scripts]. on container start, rather than any issues during container build.

I suggest you pin your version of pipenv to avoid these potential breaks in the future.

Allow PIPENV_VENV_IN_PROJECT to be read in as None, and ensure if it …

de13b0a

…is set to False that it does not use .venv directory.

matteius mentioned this pull request Mar 30, 2022

pipenv installs into .venv even though PIPENV_VENV_IN_PROJECT is not set #2763

Closed

smac89 reviewed Mar 30, 2022

View reviewed changes

matteius added 2 commits March 30, 2022 23:23

refactor based on PR feedback and add news fragment.

1ef8797

Review unit test coverage and add new tests. Remove unneccesary bits …

af6d02b

…from other tests.

matteius commented Mar 31, 2022

View reviewed changes

matteius requested a review from oz123 March 31, 2022 04:04

oz123 approved these changes Apr 6, 2022

View reviewed changes

oz123 merged commit 949ee95 into main Apr 6, 2022

oz123 deleted the issue-2763-is_venv_in_project branch April 6, 2022 19:10

matteius mentioned this pull request Apr 21, 2022

Dropped support for 3.6 not reflected in setup.py #5065

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More granular control over PIPENV_VENV_IN_PROJECT variable. #5026

More granular control over PIPENV_VENV_IN_PROJECT variable. #5026

matteius commented Mar 30, 2022 •

edited

Loading

smac89 commented Mar 30, 2022

smac89 Mar 30, 2022

matteius Mar 31, 2022

matteius Mar 31, 2022

matteius Mar 31, 2022

matteius Mar 31, 2022

matteius commented Mar 31, 2022

oz123 Apr 6, 2022

matteius Apr 6, 2022

oz123 Apr 6, 2022

smac89 Apr 13, 2022 •

edited

Loading

NeroVanbiervliet commented Apr 12, 2022

NeroVanbiervliet commented Apr 12, 2022

markmiscavage commented Apr 12, 2022

oz123 commented Apr 12, 2022

smac89 commented Apr 13, 2022

		@@ -1 +0,0 @@
		Internal to pipenv, the utils.py was split into a utils module with unused code removed.

More granular control over PIPENV_VENV_IN_PROJECT variable. #5026

More granular control over PIPENV_VENV_IN_PROJECT variable. #5026

Conversation

matteius commented Mar 30, 2022 • edited Loading

The issue

The fix

The checklist

smac89 commented Mar 30, 2022

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

matteius commented Mar 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smac89 Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

NeroVanbiervliet commented Apr 12, 2022

NeroVanbiervliet commented Apr 12, 2022

markmiscavage commented Apr 12, 2022

oz123 commented Apr 12, 2022

smac89 commented Apr 13, 2022

matteius commented Mar 30, 2022 •

edited

Loading

smac89 Apr 13, 2022 •

edited

Loading