Add Nushell activation support #2693

cvanelteren · 2023-07-20T16:04:48Z

Hi all,

I made a stat on the nu shell integration ( #2687 ). I will post the commits now for code review and write some unittests later. I was browsing in the unittest directory and saw they were not complete, is this intentional? Looking forward to your thoughts/notes.
Sincerely,
C

jonashaag · 2023-07-20T16:14:20Z

Your implementation of activate and deactivate doesn't work, does it?

cvanelteren · 2023-07-20T20:35:34Z

libmamba/data/mamba.nu

@jonashaag it does (see below). I will write some unit tests. I pushed this when I had to leave, and didn't do extensive testing.

cvanelteren · 2023-07-21T07:33:59Z

@jonashaag is there a documented environment for development listed somewhere (perhaps private). Yesterday, I was able to build the application but today I'm scratching my head at unresolved dependencies that libmamba cannot find even though they are installed. In particular, it refers to

reproc (which is flagged out of date on aur but I built locally)
solv (which installed on my machine and accessible)
There are also some shared library issues that requires putting the -fPIC flag on the compilation. Thanks in advance!

jonashaag · 2023-07-21T07:51:50Z

There is documentation on setting up a build environment. If something doesn't work please report a bug.

cvanelteren · 2023-07-22T21:18:22Z

I am still piecing together all the parts but made some nice progress so far. I will further test this next week, and will come back here when all tests pass.

cvanelteren · 2023-07-23T17:00:17Z

@jonashaag tests with pytest pass now except the ones that fail to find the included added data ({mamba_root_prefix}/etc/profile.d/mamba.nu). To make this file available I added mamba.nu to mamba/mamba/shell_templates and edited /mamba/setup.py to include the nu script as

data_files = [
    ("etc/profile.d", ["mamba/shell_templates/mamba.sh",
                       "mamba/shell_templates/mamba.nu"]),
    ("etc/fish/conf.d", ["mamba/shell_templates/mamba.fish"]),
]

However my mamba path is not seeing this change. Is this outlined process correct?

Thanks in advance!

jonashaag · 2023-07-23T20:23:42Z

We are actually planning to get rid of those files entirely and always only use micromamba shell hook. So feel free to not write those files in the first place, and remove any code that writes those files for other shells (PowerShell and cmd.exe might be an exception).

jonashaag · 2023-07-23T20:29:49Z

libmamba/data/mamba.nu

+  let argv = arg[1]
+
+  if ($cmd in [ activate deactivate ]){
+        $env.MAMBA_EXE $cmd $argv


I don't understand how this works. For the other shells we use source here because we want to change the current shell environment

Ah I believe you are right. I took this from the fish shell code. I am not too familiar with fish but I believe there it is handled a bit differently. I do some more testing this weekend.

I had a closer look at your earlier remark. The file above makes the micromamba function available to the path. There are some changes to nushell that now deprecates the let-env command. I have fixed this with the latest commit.

cvanelteren · 2023-07-29T13:20:52Z

I am having a bit trouble understanding how the shell gets initiated. The PR currently makes micromamba available to to shells. For example

micromamba env list

~/projects/mamba> micromamba env list                                               1 07/29/2023 03:12:46 PM
  Name         Active  Path                                    
─────────────────────────────────────────────────────────────────
  base                 /home/casper/micromamba                 
  mamba-dev            /home/casper/micromamba/envs/mamba-dev  
  proplot-dev          /home/casper/micromamba/envs/proplot-dev
  test                 /home/casper/micromamba/envs/test       
                       /home/casper/tmpprefixGFI833U8YN

however when I attempt to create a new env or activate it, it generates errors:

mamba activate mamba-dev


'micromamba' is running as a subprocess and can't modify the parent shell.
Thus you must initialize your shell before using activate and deactivate.

To initialize the current nu shell, run:
    $ eval "$(micromamba shell hook --shell nu)"
and then activate or deactivate with:
    $ micromamba activate
To automatically initialize all future (nu) shells, run:
    $ micromamba shell init --shell nu --root-prefix=~/micromamba
If your shell was already initialized, reinitialize your shell with:
    $ micromamba shell reinit --shell nu
Otherwise, this may be an issue. In the meantime you can run commands. See:
    $ micromamba run --help

Supported shells are {bash, zsh, csh, xonsh, cmd.exe, powershell, fish, nu}.

The shell init command does finish however

micromamba shell init nu

~/projects/mamba> micromamba shell init nu                                            07/29/2023 03:19:07 PM
Modifying RC file "/home/casper/.config/nushell/config.nu"
Generating config for root prefix "/home/casper/projects/mamba/nu"
Setting mamba executable to: "/usr/bin/micromamba"
Adding (or replacing) the following in your "/home/casper/.config/nushell/config.nu" file

# >>> mamba initialize >>>
# !! Contents within this block are managed by 'mamba init' !!
$env.MAMBA_EXE = "/usr/bin/micromamba"
$env.MAMBA_ROOT_PREFIX = "/home/casper/projects/mamba/nu"
run-external $env.MAMBA_EXE 'shell' 'hook' '--shell' 'nu' '--root-prefix' $env.MAMBA_ROOT_PREFIX --redirect-stdout 
$env.PATH = ($env.PATH | append ( [$env.MAMBA_ROOT_PREFIX bin] | path join)) 
# <<< mamba initialize <<<

~/.config/nushell/config.nu


# >>> mamba initialize >>>
# !! Contents within this block are managed by 'mamba init' !!
$env.MAMBA_EXE = "/usr/bin/micromamba"
$env.MAMBA_ROOT_PREFIX = "/home/casper/projects/mamba/nu"
run-external $env.MAMBA_EXE 'shell' 'hook' '--shell' 'nu' '--root-prefix' $env.MAMBA_ROOT_PREFIX --redirect-stdout 
$env.PATH = ($env.PATH | append ( [$env.MAMBA_ROOT_PREFIX bin] | path join)) 
# <<< mamba initialize <<<

cvanelteren · 2023-07-31T21:17:15Z

I fumbled a bit around with nushell but I got it working now. It took a bit longer than expected to fully comprehend what the activation function was doing. I was initially thinking the paths were handled internally. I was wrong!

libmamba/data/micromamba.nu

cvanelteren · 2023-08-02T19:35:40Z

For posterity's sake, if you want the current environment to be visible in nu shell you need to edit or wrap the left prompt function in ~/.config/env.nu as (for example)


($env.PWD | str substring 0..($home | str length) | str replace --string $home $"\(($env.MICROMAMBA_CURRENT_ENV)\)~"),

jonashaag

Thanks a lot for your work on this!

CMakeLists.txt

libmamba/data/micromamba.nu

libmamba/tests/src/core/test_activation.cpp

mamba/mamba/mamba_shell_init.py

mamba/mamba/shell_templates/micromamba.nu

micromamba/tests/test_shell.py

cvanelteren · 2023-08-13T08:19:43Z

This was a baptism by fire (new to nushell) but I think this is ready to merge @jonashaag.

jonashaag · 2023-08-17T07:52:21Z

@cvanelteren it looks like you're still making improvements to this PR, which is great! Can you please ping me for review once you think this is ready?

cvanelteren · 2023-08-17T08:11:22Z

@jonashaag It is done. I was thinking about adding mamba to a separate PR, but adding it was less work. All tests pass here locally except for some windows related ones in micromamba.

jonashaag · 2023-08-22T20:00:12Z

Hm, lots of build failures

cvanelteren · 2023-08-24T09:27:36Z

Hm, lots of build failures

Not sure how the namespace was handled before, but I merged the main branch to fix the errors. Should be fixed now. I am getting some pytest errors relating to bash; 7 fail 2214 pass.

cvanelteren · 2023-08-30T12:07:31Z

@jonashaag should be good now ;-)!

libmamba/data/micromamba.nu

jonashaag · 2023-08-30T13:30:27Z

libmamba/src/core/activation.cpp

+        return "";
+    }
+
+    // TODO: check this


jonashaag · 2023-08-30T13:30:31Z

libmamba/src/core/activation.cpp

+        return { "", "" };
+    }
+
+    // TODO: check this


libmamba/src/core/activation.cpp

libmamba/src/core/shell_init.cpp

cvanelteren

I replaced a few things with fmt::format but the ternary operator is a big uggly in my opinion. Is this a bit better @jonashaag?

jonashaag · 2023-08-31T10:07:31Z

libmamba/src/core/shell_init.cpp

+        content << "$env.MAMBA_ROOT_PREFIX = " << env_prefix << "\n";
+        content << "$env.PATH = ($env.PATH | prepend ([$env.MAMBA_ROOT_PREFIX bin] | path join) | uniq)\n";
+        content << R""""(
+def-env "micromamba activate"  [name: string] {


Why not do it the way it's done for the other shells? Namely, use shell hook

The other shell hooks still linked to the data subfolder, I thought you mentioned this was obsolete?

Hm, maybe I'm missing something here. For bash, this is part of the shell init generated code:

__mamba_setup="$("$MAMBA_EXE" shell hook --shell bash --root-prefix "$MAMBA_ROOT_PREFIX" 2> /dev/null)"

This calls into shell hook which outputs the equivalent of what you inlined here.

jonashaag · 2023-08-31T10:09:17Z

libmamba/src/core/activation.cpp

+             possible otherwise returns the command as a string*/
+            std::string new_command = command;
+            std::size_t idx = 0;
+            bool add = false;


jonashaag · 2023-08-31T10:09:22Z

libmamba/src/core/activation.cpp

+            /*Checks shell variable being set and converts shell lists to nushell lists if
+             possible otherwise returns the command as a string*/
+            std::string new_command = command;
+            std::size_t idx = 0;


Index of what?

I will clean up the variable names a bit.

jonashaag · 2023-08-31T10:11:10Z

libmamba/src/core/activation.cpp

+        std::stringstream out;
+        auto convert_to_nulist = [](std::string command)
+        {
+            /*Checks shell variable being set and converts shell lists to nushell lists if


I still have no clue what this function does.

The name indicates it converts something (what?) to a nu list.
The signature indicates it converts a command to a nu list.
The caller indicates it converts an environment variable value to a nu list.

What does it mean to "check shell variable being set"? Are you talking about $SHELL here? If so I don't find any reference to $SHELL in the function body. Are you talking about something else?

In nushell the path is indexed (see below). It is not a singular string like in bash. The function converts evar to a list if it detects the : delimiter. That's it.

There is, however, an edge case that this does not consider. Namely if for some reason the evar being set contains a path with a colon. I am open to suggestion to handle this differently. Edit: I believe in practice this is mutual exclusive as I have never seen paths with colons (except under windows).

libmamba/src/core/activation.cpp

jonashaag · 2023-08-31T10:15:55Z

libmamba/src/core/activation.cpp

+
+        for (const auto& [ekey, evar] : env_transform.export_vars)
+        {
+            // escape parenthesis if present otherwise checks if @evar can be converted to nushell


Rather than commenting on what is being done here (I can read that much myself!) it would be very helpful to have an explanation of why we are doing it.

Why are we quoting evar if and only if it contains a (?
Why are we converting it to something else otherwise?

In nushell parenthesis equates to evaluating the inside bits. This causes a problem when the environment problem is being set as it thinks ($conda_prompt) is an executable, hence the need to escape this variable inside a nushell. It behaves the same as other shell in other case

cvanelteren · 2023-09-06T19:22:56Z

I am pushing my (for now) final changes. Without dedicated source files, it is hard for nu to neatly run dynamically generated code (see here and here). The present setup, therefore, would be a "good enough" approach for it to work. If, however, the profile.d files are not removed, nushell's overlays could be used to neatly package the set environments. As @jonashaag commented, the goal is to remove these files, which makes it difficult to support nu without the ugly code block that is written to the config.nu file. otherwise it works fine.

EDIT: although the above is true, I figured out a way around it that is cleaner. Last thing is to integrate the prompt command properly. Currently it is set crudely by editing the existing command.

jonashaag · 2023-09-12T13:38:06Z

Can you please rebase or merge. There are unrelated changes to the CI workflows

cvanelteren · 2023-09-13T14:06:42Z

Cleaned it up @jonashaag !

jonashaag

Thank you for your perseverance, this finally looks good to merge!

<3

jonashaag · 2023-09-13T17:38:03Z

libmamba/src/core/shell_init.cpp

+        $env.PATH = ($env.PATH | prepend $"($env.MAMBA_ROOT_PREFIX)/condabin")
+    }
+    #ask mamba how to setup the environment and set the environment
+    (micromamba shell activate --shell nu $name


Should be using $MAMBA_EXE here?

cvanelteren · 2023-09-13T19:07:44Z

Happy to help ;-)! Thanks for the reviews and strict look over my shoulder. See you in a future commit

…

On Wed, 13 Sept 2023 at 19:40, Jonas Haag ***@***.***> wrote: ***@***.**** approved this pull request. Thank you for your perseverance, this finally looks good to merge! <3 ------------------------------ In libmamba/src/core/shell_init.cpp <#2693 (comment)>: > + s_mamba_exe = mamba_exe.string(); + } + + content << "\n# >>> mamba initialize >>>\n"; + content << "# !! Contents within this block are managed by 'mamba init' !!\n"; + content << "$env.MAMBA_EXE = " << mamba_exe << "\n"; + content << "$env.MAMBA_ROOT_PREFIX = " << env_prefix << "\n"; + content << "$env.PATH = ($env.PATH | prepend ([$env.MAMBA_ROOT_PREFIX bin] | path join) | uniq)\n"; + content << R"###(def-env "micromamba activate" [name: string] { + #add condabin when root + if $env.MAMBA_SHLVL? == null { + $env.MAMBA_SHLVL = 0 + $env.PATH = ($env.PATH | prepend $"($env.MAMBA_ROOT_PREFIX)/condabin") + } + #ask mamba how to setup the environment and set the environment + (micromamba shell activate --shell nu $name Should be using $MAMBA_EXE here? — Reply to this email directly, view it on GitHub <#2693 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEUVDVYYOUMXKZHGUIQJE73X2HVZVANCNFSM6AAAAAA2RU2UMI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- - Checkout my website <https://cvanelteren.github.io/> for contact info and current projects!

zacharyburnett · 2023-09-27T16:57:36Z

Looking forward to using this in my shell! Thanks for implementing it!

cvanelteren · 2023-09-27T18:35:57Z

No worries @zacharyburnett. For posterity please use #284, until this gets merged this PR has a deactivation bug.

cvanelteren commented Jul 20, 2023

View reviewed changes

jonashaag reviewed Jul 23, 2023

View reviewed changes

wolfv mentioned this pull request Jul 23, 2023

Implement support for nushell mamba-org/rattler#270

Closed

jonashaag reviewed Jul 31, 2023

View reviewed changes

libmamba/data/micromamba.nu Outdated Show resolved Hide resolved

cvanelteren force-pushed the nushell branch 3 times, most recently from 50ed169 to a76cc6e Compare July 31, 2023 21:39

cvanelteren force-pushed the nushell branch from 52f8668 to 1dbe2a6 Compare August 6, 2023 08:19

jonashaag reviewed Aug 10, 2023

View reviewed changes

cvanelteren force-pushed the nushell branch 2 times, most recently from fca3e69 to f152113 Compare August 29, 2023 05:07

cvanelteren marked this pull request as draft August 30, 2023 05:39

cvanelteren marked this pull request as ready for review August 30, 2023 05:48

jonashaag requested changes Aug 30, 2023

View reviewed changes

cvanelteren commented Aug 31, 2023

View reviewed changes

jonashaag reviewed Aug 31, 2023

View reviewed changes

cvanelteren marked this pull request as draft September 7, 2023 07:51

cvanelteren marked this pull request as ready for review September 10, 2023 13:28

cvanelteren requested a review from jonashaag September 13, 2023 07:17

Clean init

407d5ab

cvanelteren force-pushed the nushell branch from cebdaf8 to 407d5ab Compare September 13, 2023 08:29

jonashaag approved these changes Sep 13, 2023

View reviewed changes

Use env path set as external command

c1a0c5c

jonashaag changed the title ~~Init nu shell~~ Add Nushell activation support Sep 14, 2023

jonashaag merged commit f4cea9b into mamba-org:main Sep 14, 2023
23 checks passed

This was referenced Sep 15, 2023

Nushell closure fix #2840

Closed

Nushell hotfix #2841

Merged

Add Nushell activation support #2693

Add Nushell activation support #2693

Conversation

cvanelteren commented Jul 20, 2023

jonashaag commented Jul 20, 2023

Choose a reason for hiding this comment

cvanelteren commented Jul 21, 2023

jonashaag commented Jul 21, 2023

cvanelteren commented Jul 22, 2023

cvanelteren commented Jul 23, 2023

jonashaag commented Jul 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvanelteren commented Jul 29, 2023

cvanelteren commented Jul 31, 2023

cvanelteren commented Aug 2, 2023 • edited Loading

jonashaag left a comment

Choose a reason for hiding this comment

cvanelteren commented Aug 13, 2023

jonashaag commented Aug 17, 2023

cvanelteren commented Aug 17, 2023

jonashaag commented Aug 22, 2023

cvanelteren commented Aug 24, 2023

cvanelteren commented Aug 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvanelteren left a 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

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

cvanelteren Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

cvanelteren Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvanelteren Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

cvanelteren commented Sep 6, 2023 • edited Loading

jonashaag commented Sep 12, 2023

cvanelteren commented Sep 13, 2023

jonashaag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvanelteren commented Sep 13, 2023 via email

zacharyburnett commented Sep 27, 2023

cvanelteren commented Sep 27, 2023

jonashaag commented Jul 23, 2023 •

edited

Loading

cvanelteren commented Aug 2, 2023 •

edited

Loading

cvanelteren Aug 31, 2023 •

edited

Loading

cvanelteren Aug 31, 2023 •

edited

Loading

cvanelteren Aug 31, 2023 •

edited

Loading

cvanelteren commented Sep 6, 2023 •

edited

Loading