Nushell activation scripts #2170

elferherrera · 2021-08-16T10:36:31Z

Hi all,

I'm including a brand new activation script for nushell. I think I've got the base structure but I was wondering if someone could help me set up some things.

The first issue is that because of the nature of nushell, a deactivation function cannot be created. In order to deactivate an environment we will need to source a deactivate script (included with this PR). How can I check that that file is created together with the activate.nu file?
Can someone help me set up the tests for activating the scripts for nushell?

for more information, see https://pre-commit.ci

codecov · 2021-08-16T10:41:15Z

Codecov Report

Merging #2170 (96795f9) into main (19b861a) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2170      +/-   ##
==========================================
- Coverage   93.55%   93.55%   -0.01%     
==========================================
  Files          87       88       +1     
  Lines        4361     4373      +12     
==========================================
+ Hits         4080     4091      +11     
- Misses        281      282       +1

Flag	Coverage Δ
tests	`93.55% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/virtualenv/activation/__init__.py	`100.00% <100.00%> (ø)`
src/virtualenv/activation/nushell/__init__.py	`100.00% <100.00%> (ø)`
src/virtualenv/seed/embed/base_embed.py	`96.22% <0.00%> (-1.89%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19b861a...96795f9. Read the comment docs.

gaborbernat

For this to get accepted you'll need to setup the test suite for it, as per other shells do, see https://github.com/pypa/virtualenv/blob/main/tests/unit/activation/test_batch.py

elferherrera · 2021-08-16T16:40:08Z

thanks @gaborbernat for pointing me to that script. I see that I would need to set up in the CI an instance of nushell as well to test it. Is that correct?

gaborbernat · 2021-08-16T16:42:07Z

Yes, you're correct. I'd also like to mention that condition of accepting this PR is:

nushell will be stable and not break the CI every month,
does not add the significant overhead to the CI.

If either of those is true now or in the future we'll remove it from the core, and encourage you to instead provide it as a plugin.

src/virtualenv/activation/nushell/deactivate.nu

…into nushell

for more information, see https://pre-commit.ci

elferherrera · 2021-08-19T07:13:34Z

@gaborbernat I was wondering if you could help me understand something. When asserting this line I get this value
c:\users\123123\documents\repos\virtualenv\venv\scripts\python.exe
but it expects this
c:\users\123123\appdata\local\temp\pytest-of-123123\pytest-100\activation-tester-env0\e-$ èрт🚒♞中片-j\scripts\python.exe.

I've looked everywhere but dont understand where that is specified in the unit tests. In the logs I see that the files are copied to the correct location. Can you help me out?

gaborbernat · 2021-08-19T07:59:59Z

Those match up with these https://github.com/pypa/virtualenv/blob/main/tests/unit/activation/conftest.py#L123-L135. So what you see there is that post-activation we still get the original path and not the virtual environment one.

…into nushell

for more information, see https://pre-commit.ci

…into nushell

for more information, see https://pre-commit.ci

elferherrera · 2021-08-19T10:29:29Z

@gaborbernat thanks. That was useful. However, I'm having trouble understanding why the output from this line

source 'C:\Users\123123\AppData\Local\Temp\pytest-of-123123\pytest-131\activation-tester-env1\e-$ Ã¨Ñ€Ñ‚ðŸš’â™žä¸ç‰‡-j\Scripts\activate.nu'

has to be

c:\users\123123\appdata\local\temp\pytest-of-123123\pytest-131\activation-tester-env1\e-$ èрт🚒♞中片-j\scripts\python.exe

gaborbernat · 2021-08-19T10:38:57Z

To support paths with Unicode characters. What you see in the line above is a sign of garbled coding by the shell (meaning your activator might work for ASCII paths only), but will not for Unicode ones (notably when users have Unicode char in their name, and they work in their home folder).

elferherrera · 2021-08-19T10:49:37Z

@gaborbernat sorry, I copied that string from the terminal and it got jumbled. That is not my problem now. It seems that because Im testing my changes in a virtual environment the test is not overriding the VIRTUAL_ENV from the test, that's why one file is pointing to the venv in my shell and the other is pointing to the venv from the test.

Is it possible to monkeypatch the variable for the test?

gaborbernat · 2021-08-19T11:00:44Z

I don't think that's the case, because then why would this not happen for the other shells 😬

…

On Thu, 19 Aug 2021, 11:49 Fernando Herrera, ***@***.***> wrote: @gaborbernat <https://github.com/gaborbernat> sorry, I copied that string from the terminal and it got jumbled. That is not my problem now. It seems that because Im testing my changes in a virtual environment the test is not overriding the VIRTUAL_ENV from the test, that's why one file is pointing to the venv in my shell and the other is pointing to the venv from the test. Is it possible to monkeypatch the variable for the test? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2170 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFIQPUCUD5SQLGE3A2MO73T5TOUZANCNFSM5CHO26UQ> .

elferherrera · 2021-08-19T11:03:29Z

you are right. I'm running the powershell test and I dont get that error. This is really weird.
btw, if I run the activate command and create a venv with the main module everything works fine.
Also, if I change the test and use a simple folder I still get the same error.

elferherrera · 2021-08-19T11:18:15Z

Ok. I think I found the issue. The problem is that when nushell sources that file, it changes the env variables (PATH) only for its scope and because the test is looking outside nushell's scope, then it finds the previous env variable. I need to figure out how to maintain nushell's scope during the test

elferherrera · 2021-08-19T21:14:03Z

@gaborbernat again, more questions. I have been testing the script that is run in the test, which is

python.exe -c 'import sys; print(sys.executable)'
python.exe -c 'import os; import sys; v = os.environ.get("VIRTUAL_ENV"); print(v)'
source 'C:\Users\123123\Documents\repos\virtualenv\something\Scripts\activate.nu'
python.exe -c 'import sys; print(sys.executable)'
python.exe -c 'import os; import sys; v = os.environ.get("VIRTUAL_ENV"); print(v)'
pydoc -w os
deactivate
python.exe -c 'import sys; print(sys.executable)'
python.exe -c 'import os; import sys; v = os.environ.get("VIRTUAL_ENV"); print(v)'

and it runs perfectly in nushell and even inside a python shell using Popen
the result I get is

C:\Users\123123\Documents\repos\virtualenv\venv\Scripts\python.exe      <---- old venv
C:\Users\123123\Documents\repos\virtualenv\venv                         <---- old venv
C:\Users\123123\Documents\repos\virtualenv\something\Scripts\python.exe <---- new venv
something                                                               <---- new venv
wrote os.html                                                           <---- result from pydoc
C:\Users\123123\Documents\repos\virtualenv\venv\Scripts\python.exe      <---- old venv after deactivate
C:\Users\123123\Documents\repos\virtualenv\venv                         <---- old venv

which is what the test expect

however, when running the test, in this line the result we get is this:

c:\users\123123\documents\repos\virtualenv\venv\scripts\python.exe      <---- This should be none
None                                                                    <---- correct
c:\users\123123\documents\repos\virtualenv\venv\scripts\python.exe      <---- this should be the new env
e-$ èрт🚒♞中片-j                                                       <---- this is correct. Name of env
wrote os.html                                                           <---- result from pydoc
c:\users\123123\documents\repos\virtualenv\venv\scripts\python.exe      <---- old env
None                                                                    <---- correct

which obviously shows that the new env is not loaded.

So, this leads me to the next question, do you remember if there is an special configuration that I need to set for the ActivationTester? Do I need to patch some other env variables?

gaborbernat · 2021-08-19T21:18:44Z

If they are environment variables at play that must be specific to nu, and those I'm not aware of. 😬 Perhaps - someone who knows in detail how nu operates can help 🤔

elferherrera · 2021-09-03T15:18:31Z

We are so close. I really want to have this in nushell to start using it as my main shell

gaborbernat · 2021-09-04T19:57:21Z

We are so close. I really want to have this in nushell to start using it as my main shell

I'm on a holiday now so unavailable for looking into this myself. But if you can figure out the CI, we can push it ahead.

elferherrera · 2021-09-04T21:40:39Z

I'm on a holiday now so unavailable for looking into this myself. But if you can figure out the CI, we can push it ahead.

the problem is that the mac tests always fail because of that assertion. I dont know what to do about it

gaborbernat · 2021-09-04T21:57:42Z

It passes on main, see https://github.com/pypa/virtualenv/actions/runs/1200507280, so the reason must be with the code you're adding here 😊

PYTHONEXECUTABLE is not used by any of the other shell activation scripts, setting it directly will require this script to walk the path and determine the OS. We should not be setting it. This resolves test failures on macOS, since the previous version added the windows-specific `.exe` suffix.

lily-mara · 2021-09-16T06:04:39Z

I believe I have resolved the macOS test failures in elferherrera#1

Remove the PYTHONEXECUTABLE variable from nushell scripts

…into nushell

elferherrera · 2021-09-16T07:36:16Z

@gaborbernat thanks to @lily-mara we are passing all virtualenv tests. Do you mind approving the workflow?

gaborbernat · 2021-09-16T07:38:52Z

src/virtualenv/activation/nushell/__init__.py

+            "__VIRTUAL_NAME__": creator.env_name,
+            "__BIN_NAME__": ensure_text(str(creator.bin_dir.relative_to(creator.dest))),
+            "__PATH_SEP__": ensure_text(os.pathsep),
+            "__DEACTIVATE_PATH__": ensure_text(str(Path(dest_folder) / Path("deactivate.nu"))),


I don't think you need to wrap the deactivate.nu in path 👍

gaborbernat · 2021-09-16T07:40:37Z

tests/unit/activation/test_nushell.py

+def test_nushell(activation_tester_class, activation_tester):
+    class Nushell(activation_tester_class):
+        def __init__(self, session):
+            cmd = "c:\\program files\\nu\\bin\\nu.exe" if IS_WIN else "nu"


This might work in the CI but will not work locally as well 👍 Better to use shutil.which here and only fallback to this hard coded path if that returns None and we're on windows.

for more information, see https://pre-commit.ci

gaborbernat · 2021-09-16T08:43:51Z

@elferherrera seems Python 2 does not like it 😟

…into nushell

elferherrera · 2021-09-16T08:55:58Z

@gaborbernat let me have a look. I was checking that I was getting an error with the lints

gaborbernat · 2021-09-16T08:57:49Z

Python 2 doesn't support shutil.which so you'd need to add a small backwards compatible method 🤔

elferherrera · 2021-09-16T10:24:58Z

@gaborbernat I think this time everything passes

We should only use deprected API if we must. Signed-off-by: Bernát Gábor <[email protected]>

gaborbernat · 2021-09-16T13:16:00Z

Released via https://virtualenv.pypa.io/en/latest/changelog.html#v20-8-0-2021-09-16

elferherrera and others added 2 commits August 16, 2021 10:46

nushell activation scripts

e1c7d88

[pre-commit.ci] auto fixes from pre-commit.com hooks

880c067

for more information, see https://pre-commit.ci

gaborbernat requested changes Aug 16, 2021

View reviewed changes

gaborbernat marked this pull request as draft August 16, 2021 16:35

kubouch reviewed Aug 17, 2021

View reviewed changes

src/virtualenv/activation/nushell/deactivate.nu Outdated Show resolved Hide resolved

elferherrera and others added 3 commits August 17, 2021 16:00

nushell activate template

1dd281e

Merge branch 'nushell' of https://github.com/elferherrera/virtualenv …

efd6cf1

…into nushell

[pre-commit.ci] auto fixes from pre-commit.com hooks

aa53639

for more information, see https://pre-commit.ci

elferherrera and others added 7 commits August 19, 2021 10:36

alias in activate

76990f1

Merge branch 'nushell' of https://github.com/elferherrera/virtualenv …

11553e7

…into nushell

[pre-commit.ci] auto fixes from pre-commit.com hooks

8c02108

for more information, see https://pre-commit.ci

correction in text

e614214

Merge branch 'nushell' of https://github.com/elferherrera/virtualenv …

9794951

…into nushell

dealias of pydoc and deactivate

463e77b

[pre-commit.ci] auto fixes from pre-commit.com hooks

fa89fc5

for more information, see https://pre-commit.ci

kubouch mentioned this pull request Aug 20, 2021

Make Scope::remove_env_var global nushell/nushell#3941

Closed

using dest_folder for variables

9abbbfe

elferherrera added 5 commits September 16, 2021 07:36

Merge pull request #1 from lily-mara/nushell

743f0cb

Remove the PYTHONEXECUTABLE variable from nushell scripts

change command_prompt to command_script

e173b3b

Merge branch 'nushell' of https://github.com/elferherrera/virtualenv …

215e839

…into nushell

all tests enabled

4dade58

removed sys plugin

dbdff01

elferherrera closed this Sep 16, 2021

elferherrera reopened this Sep 16, 2021

gaborbernat requested changes Sep 16, 2021

View reviewed changes

elferherrera and others added 2 commits September 16, 2021 09:20

get path using shutil

8f654a4

[pre-commit.ci] auto fixes from pre-commit.com hooks

7d7a3db

for more information, see https://pre-commit.ci

gaborbernat approved these changes Sep 16, 2021

View reviewed changes

elferherrera added 2 commits September 16, 2021 09:51

import lint

80e7165

Merge branch 'nushell' of https://github.com/elferherrera/virtualenv …

ae48be0

…into nushell

using find executable

0d7dbed

elferherrera and others added 2 commits September 16, 2021 11:33

change to prompt_command

aed2af8

Ammend command discovery

96795f9

We should only use deprected API if we must. Signed-off-by: Bernát Gábor <[email protected]>

gaborbernat merged commit e0410ca into pypa:main Sep 16, 2021

shahinism mentioned this pull request Sep 17, 2021

Exception: No module named 'virtualenv.activation.nushell' python-poetry/poetry#4515

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nushell activation scripts #2170

Nushell activation scripts #2170

elferherrera commented Aug 16, 2021

codecov bot commented Aug 16, 2021 •

edited

Loading

gaborbernat left a comment

elferherrera commented Aug 16, 2021

gaborbernat commented Aug 16, 2021 •

edited

Loading

elferherrera commented Aug 19, 2021

gaborbernat commented Aug 19, 2021

elferherrera commented Aug 19, 2021

gaborbernat commented Aug 19, 2021 •

edited

Loading

elferherrera commented Aug 19, 2021

gaborbernat commented Aug 19, 2021 via email

elferherrera commented Aug 19, 2021 •

edited

Loading

elferherrera commented Aug 19, 2021

elferherrera commented Aug 19, 2021

gaborbernat commented Aug 19, 2021

elferherrera commented Sep 3, 2021

gaborbernat commented Sep 4, 2021

elferherrera commented Sep 4, 2021

gaborbernat commented Sep 4, 2021

lily-mara commented Sep 16, 2021

elferherrera commented Sep 16, 2021

gaborbernat Sep 16, 2021

gaborbernat Sep 16, 2021

gaborbernat commented Sep 16, 2021

elferherrera commented Sep 16, 2021

gaborbernat commented Sep 16, 2021

elferherrera commented Sep 16, 2021

gaborbernat commented Sep 16, 2021

Nushell activation scripts #2170

Nushell activation scripts #2170

Conversation

elferherrera commented Aug 16, 2021

codecov bot commented Aug 16, 2021 • edited Loading

Codecov Report

gaborbernat left a comment

Choose a reason for hiding this comment

elferherrera commented Aug 16, 2021

gaborbernat commented Aug 16, 2021 • edited Loading

elferherrera commented Aug 19, 2021

gaborbernat commented Aug 19, 2021

elferherrera commented Aug 19, 2021

gaborbernat commented Aug 19, 2021 • edited Loading

elferherrera commented Aug 19, 2021

gaborbernat commented Aug 19, 2021 via email

elferherrera commented Aug 19, 2021 • edited Loading

elferherrera commented Aug 19, 2021

elferherrera commented Aug 19, 2021

gaborbernat commented Aug 19, 2021

elferherrera commented Sep 3, 2021

gaborbernat commented Sep 4, 2021

elferherrera commented Sep 4, 2021

gaborbernat commented Sep 4, 2021

lily-mara commented Sep 16, 2021

elferherrera commented Sep 16, 2021

gaborbernat Sep 16, 2021

Choose a reason for hiding this comment

gaborbernat Sep 16, 2021

Choose a reason for hiding this comment

gaborbernat commented Sep 16, 2021

elferherrera commented Sep 16, 2021

gaborbernat commented Sep 16, 2021

elferherrera commented Sep 16, 2021

gaborbernat commented Sep 16, 2021

codecov bot commented Aug 16, 2021 •

edited

Loading

gaborbernat commented Aug 16, 2021 •

edited

Loading

gaborbernat commented Aug 19, 2021 •

edited

Loading

elferherrera commented Aug 19, 2021 •

edited

Loading