Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relocate broken #28

Closed
HQJaTu opened this issue Dec 28, 2023 · 7 comments
Closed

Relocate broken #28

HQJaTu opened this issue Dec 28, 2023 · 7 comments

Comments

@HQJaTu
Copy link

HQJaTu commented Dec 28, 2023

Venv commit: python/cpython@ebc8103

base.py has

class ActivateFile(BinFile):
    read_pattern = re.compile(r"""^VIRTUAL_ENV=["'](.*)["']$""")

As new activation script is:

# on Windows, a path can contain colons and backslashes and has to be converted:
if [ "$OSTYPE" = "cygwin" ] || [ "$OSTYPE" = "msys" ] ; then
    # transform D:\path\to\venv to /d/path/to/venv on MSYS
    # and to /cygdrive/d/path/to/venv on Cygwin
    export VIRTUAL_ENV=$(cygpath "__VENV_DIR__")
else
    # use the path as-is
    export VIRTUAL_ENV="__VENV_DIR__"
fi

Above regexp won't match anything making the entire thing collapse. Also, there are number of new file types, like .fish.

Suggested patch:

--- venv/base.py-orig   2023-12-28 14:54:09.350289838 +0200
+++ venv/base.py        2023-12-28 16:46:10.395259729 +0200
@@ -207,7 +207,10 @@

     """The virtual environment /bin/activate script."""

-    read_pattern = re.compile(r"""^VIRTUAL_ENV=["'](.*)["']$""")
+    read_pattern_pre_312 = re.compile(r"""^VIRTUAL_ENV=["'](.*)["']$""")
+    read_pattern_312 = re.compile(r"""^    export VIRTUAL_ENV=["'](.*)["']$""")
+    read_pattern_312_csh = re.compile(r"""^setenv VIRTUAL_ENV ["'](.*)["']$""")
+    read_pattern_312_fish = re.compile(r"""^set -gx VIRTUAL_ENV ["'](.*)["']$""")

     def _find_vpath(self):
         """
@@ -221,7 +224,16 @@

             for count, line in enumerate(file_handle):

-                match = self.read_pattern.match(line)
+                match = self.read_pattern_pre_312.match(line)
+                if not match:
+                    match = self.read_pattern_312.match(line)
+
+                if not match:
+                    match = self.read_pattern_312_csh.match(line)
+
+                if not match:
+                    match = self.read_pattern_312_fish.match(line)
+
                 if match:

                     return match.group(0), match.group(1), count
@kevinconway
Copy link
Owner

kevinconway commented Jan 4, 2024

Thanks for opening an issue and providing a fix at the same time! I appreciate that you took backwards compatibility into account. I should have time this weekend to test and merge your patch. I will also accept a pull request that contains this patch if you want to create one.

@kevinconway
Copy link
Owner

Also, the project should already handle the alternative shells like fish using https://github.com/kevinconway/venvctrl/blob/master/venvctrl/venv/base.py#L244-L262. If you're seeing issues with those then it may be another bug.

@HQJaTu
Copy link
Author

HQJaTu commented Jan 6, 2024

Unfortunately, there is no PR as I'm swamped with work with stuff that doesn't work.
I understand offering a PR would make your life easier. This was a rush job on my part. Apologies.

@kevinconway
Copy link
Owner

I looked more closely at the issue. You linked to a change for venv in the standard library but that's not the same as virtualenv. Currently, this library only works with virtualenv which solves the same problem as the linked issue a different way so I do not suspect that the same kind of change will be made to virtualenv. I can confirm from testing that venvctrl works with the latest release of virtualenv.

Can you give me any more details on how this issue is impacting you? For example, are you working in an environment that has set virtualenv as an alias for python -m venv or something similar?

@HQJaTu
Copy link
Author

HQJaTu commented Jan 7, 2024

I never use virtualenv. As recommended in many sources, I stick with Python's native venv shipped with every installation.

When talking about venvctrl, I never realized it not to support venv! Partly, this is because the project I'm really using is rpmvenv and I ever had any problems with dependency packages like venvctrl before this. Btw. note how rpmvenv's name isn't rpmvirtualenv!

Thus, supporting both is relatively easy and can be achieved. My suggested patch should be a good indication on this.

Going forward, there are pretty much two options:

  1. You agree to support venv. Everybody should be happy.
  2. You agree not to support venv. (When I'm less busy) I'll create a fork, make it support venv and I'm happy, a workflow exists for me to work with Python RPMs again. As a side-effect, other venv users are less happy.

I really don't see a 3rd option. Agree?

Also, I'd suggest putting emphasis on venv vs. virtualenv in docs and code. The topic is confusing enough for any developer.

@kevinconway
Copy link
Owner

OK, I see now. You are using the python_venv.command option to replace virtualenv with python -m venv, correct? I forgot that was an option. rpmvenv uses a configurable command to create an environment and defaults to virtualenv. I thought it used venvctrl to create an environment which can only create them using virtualenv, it is hard coded. I forgot that rpmvenv only uses venvctrl to relocate. This is why I was confused, I didn't think about someone using rpmvenv with venv instead of virtualenv.

I don't know if I will officially support venv long term because I don't know how much extra work it will be. I already have to fix the project every few years when an activate script changes format. But, I will merge a fix for now. Please review #29 when you have time. The change to fix it was a bit more complex than the patch you suggested. I also added a configuration so all of the tests would run for both virtualenv and venv.

As for the name of venv and the name of venvctrl. When I started this project 10 years ago it was still Python 2 and venv did not exist. venv was the common short hand for virtualenv. I used the common term in the name of the project. So, it wasn't confusing when I created it...

@kevinconway
Copy link
Owner

Fixed by #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants