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

Update shell_init for nushell #3771

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Update shell_init for nushell #3771

wants to merge 6 commits into from

Conversation

Wuvist
Copy link

@Wuvist Wuvist commented Jan 27, 2025

In nushell, PATH variable is a list, not a string.

Thus, after load-env, must set $env.PATH to a list. Otherwise, all PATH are lost.

Tested on nushell 0.101.0 on mac.

In nushell, PATH variable is a list, not a string.
After load-env, must set $env.PATH to a list.
@Wuvist
Copy link
Author

Wuvist commented Jan 27, 2025

Perhaps @cvanelteren could help to look at my this PR?

It should fix issue on nushell 0.101.0 on mac. But still problematic on windows 11.

($env.MAMBA_EXE) shell activate --shell nu $name is giving wrong env output on win, I would try to fix but I'm not familiar with C++.

@cvanelteren
Copy link
Contributor

cvanelteren commented Jan 27, 2025

I approve this msg ;-). On linux I haven't had any issues without this fix however. Can confirm that it should be a list.

I dunno how windows uses the path, haven't used in in a while. Where does it put $env.MAMBA_EXE for you?

@Wuvist
Copy link
Author

Wuvist commented Jan 27, 2025

nu 0.89.0 on Ubuntu(via WSL)
image

nu 0.101.0 on mac
image

Both return $env.PATH as list by default.

When set it to string, nu 0.89.0 on Ubuntu path still works, but nu 0.101.0 on mac will fail.

I'm not sure it's nu version issue or OS issue. What I can confirm now is that setting it to list works on both. :P

I will feedback more about windows later.

@cvanelteren
Copy link
Contributor

Hmm strange I am using 0.101.0 and I haven't had any issues yet, but for sure it should be a list.

Is the other issue you mentioned related or not?

@Wuvist
Copy link
Author

Wuvist commented Jan 28, 2025

Maybe related to: https://github.com/mamba-org/mamba/blob/fc950f8a3b26200ced1ddc58ecb71cf08b6407a3/libmamba/src/core/shell_init.cpp#L472C13-L472C67

I think the issue on windows is more related to the path separator, it's not :, but ;

Thus, | split row ";" mess up everything.

However, except the first PATH variable, the other variable has CONDA_ prefix, so I could modify the script to sth like:

    #ask mamba how to setup the environment and set the environment
    (^($env.MAMBA_EXE) shell activate --shell nu $name
      | str replace --regex '\s+' '' --all
      | str replace ';CONDA_' ';;;;CONDA_' --all
      | split row ";;;;"
      | parse --regex '(.*)=(.+)'
      | transpose --header-row
      | into record
      | load-env
    )
    $env.Path = $env.PATH | split row (char esep)

And mamba activate XXX cmd works on my win11 env. mamba deactivate doesn't work though, but it's good enough for me for now.

The last line also need to use $env.Path instead of $env.PATH, guess it's related to nushell/nushell#7741

I would submit another PR for win if I figure out why mamba deactivate is still breaking.

@cvanelteren
Copy link
Contributor

Can you open the PR for changes? I think I fixed the issue for you.

@Wuvist
Copy link
Author

Wuvist commented Jan 28, 2025

Think I more or less confirmed what go wrong on win:

  • path separator, it's not :, but ;
  • It's $env.Path not $env.PATH
  • It's common that Windows path contains space, like C:\Program Files

I could "hack" the script to sth like:

def --env "mamba activate"  [name: string] {
    #add condabin when base env
    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
    (^($env.MAMBA_EXE) shell activate --shell nu $name
      | str replace ';CONDA_' ';;;;CONDA_' --all
      | split row ";;;;"
      | parse --regex '(.*) = (.+)'
      | transpose --header-row
      | into record
      | load-env
    )
    $env.Path = $env.PATH | split row (char esep)
    $env.PATH = $env.Path
    # update prompt
    # if ($env.CONDA_PROMPT_MODIFIER? != null) {
    #   $env.PROMPT_COMMAND = {|| $env.CONDA_PROMPT_MODIFIER + (do $env.PROMPT_COMMAND_BK)}
    # }
}
def --env "mamba deactivate"  [] {
    #remove active environment except base env
    if $env.CONDA_PROMPT_MODIFIER? != null {
      # unset set variables
      for x in (^$env.MAMBA_EXE shell deactivate --shell nu
              | str replace ';CONDA_' ';;;;CONDA_' --all
              | str replace ';hide-env' ';;;;hide-env' --all
              | split row ";;;;") {
          if ("hide-env" in $x) {
            hide-env ($x | parse "hide-env {var}").var.0
          } else if $x != "" {
            let keyValue = ($x
            | parse '{key} = {value}'
            )
            load-env {$keyValue.0.key: $keyValue.0.value}
            $env.Path = $env.PATH | split row (char esep)
            $env.PATH = $env.Path
          }
    }
    # reset prompt
    # $env.PROMPT_COMMAND = $env.PROMPT_COMMAND_BK
  }
}

And both mamba activate xxx mamba deactivate work on my env.

@cvanelteren I am not sure why you use str replace --regex '\s+' '' --all to remove all space, if it's to ignore the space in =, would it be better to use | parse --regex '(.*) = (.+)' and | parse '{key} = {value}'?

In above script, I also commented out the part handling prompt, because I'm using starship, which has built in support for conda: https://starship.rs/config/#conda

(My hacky script actually will add addition ; to CONDA_PROMPT_MODIFIER)

Not sure if it's desired to modify user's prompt in mamba's nushell support.

@cvanelteren
Copy link
Contributor

This is not the way. I can add my changes if the PR is open for edits which it currently is not.

@Wuvist
Copy link
Author

Wuvist commented Jan 28, 2025

This is not the way. I can add my changes if the PR is open for edits which it currently is not.

I'm not sure how to open this PR for edit, the Allow edits by maintainers is already check.

image

I've also invited you to my fork, guess that could also let you commit to this PR?

@cvanelteren Thank you for the help!

@cvanelteren
Copy link
Contributor

Can you add write access to your fork, then I can push directly

@cvanelteren
Copy link
Contributor

hmm it didn't sync the changes. You should have them on the repo however

@cvanelteren
Copy link
Contributor

Oh nvm it did. Check this build and see if it solves your issue.

@@ -515,7 +516,7 @@ namespace mamba
if $env.CONDA_PROMPT_MODIFIER? != null {
# unset set variables
for x in (^$env.MAMBA_EXE shell deactivate --shell nu
| split row ";") {
| split row (if $nu.os-info.name == "windows" { ";" } else { ";" }) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be (if $nu.os-info.name == "windows" { ";" } else { ":" })?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't the separator ; on windows?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yah, but the else part, it should be :, I guess line 519 should be the same as line 500

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right you are correct ;-) Fixed!

@Wuvist
Copy link
Author

Wuvist commented Jan 28, 2025

Oh nvm it did. Check this build and see if it solves your issue.

See your changes, think it will solve the path separator issue. But str replace --regex '\s+' '' --all will still break Windows path contains space, like C:\Program Files.

No sure why all space need to be removed. In my env, I just use parse --regex '(.*) = (.+)' instead of existing parse --regex '(.*)=(.+)' to remove the space in front and behind =, and it appears to work.

@cvanelteren
Copy link
Contributor

Hmm it's been quite some time since I originally wrote this. Not sure why I remove the white spaces. You can merge your changes back in. The main thing I changed is that the problem was that the variables were added always with ; at the end. In addition, the parsing now check on the specific sep for the os.

@cvanelteren
Copy link
Contributor

I think the variable assignment requires some cleaning with the white spaces -- at least that is my hunch.

@cvanelteren
Copy link
Contributor

But feel free to experiment. I think it is pretty close ;-)

@Wuvist
Copy link
Author

Wuvist commented Jan 28, 2025

But feel free to experiment. I think it is pretty close ;-)

Thank for the help! I will do more testing and learn how to compile C++ project first :)

@Wuvist Wuvist marked this pull request as draft January 28, 2025 12:49
@cvanelteren
Copy link
Contributor

Aah yes, the great journey. Let me know if you need more support. Good luck!

@Wuvist
Copy link
Author

Wuvist commented Jan 28, 2025

Aah yes, the great journey. Let me know if you need more support. Good luck!

:) Seems support is indeed needed.

It's quite fun for me to learn about ninja / cmake / clang etc, and I managed to build mamba on Mac.

But, no luck for me to use MSVC to build mamba on win.

I've dig into .github\workflows\windows_impl.yml and try to understand the MSVC comment in root CMakeLists.txt file leave by @Klaim

But no luck.

cl.exe either throw me error like:

[101/240] Building CXX object libmamba\CMakeFiles\libmamba-dyn.dir\src\core\shell_init.cpp.obj
FAILED: libmamba/CMakeFiles/libmamba-dyn.dir/src/core/shell_init.cpp.obj
C:\PROGRA~1\MIB055~1\2022\COMMUN~1\VC\Tools\MSVC\1442~1.344\bin\Hostx64\x64\cl.exe  /nologo /TP -DFMT_SHARED -DGHC_WIN_DISABLE_WSTRING_STORAGE_TYPE -DLIBMAMBA_EXPORTS -DMAMBA_USE_INSTALL_PREFIX_AS_BASE -DREPROCXX_SHARED -DREPROC_SHARED -DSIMDJSON_THREADS_ENABLED=1 -DSIMDJSON_USING_WINDOWS_DYNAMIC_LIBRARY=1 -DSPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_TRACE -DSPDLOG_FMT_EXTERNAL -Dlibmamba_dyn_EXPORTS -IC:\Users\wuvis\code\mamba\libmamba\include -IC:\Users\wuvis\code\mamba\libmamba\src -IC:\Users\wuvis\.local\share\mamba\envs\mamba\Library\include\winreg -IC:\Users\wuvis\code\mamba\libmamba\ext\solv-cpp\include -external:IC:\Users\wuvis\.local\share\mamba\envs\mamba\Library\include -external:IC:\Users\wuvis\.local\share\mamba\envs\mamba\Library\share -external:W0 /DWIN32 /D_WINDOWS /GR /EHsc /D_CRT_SECURE_NO_WARNINGS /DNOMINMAX /EHsc /Zc:__cplusplus /utf-8 /experimental:external /external:I C:\Users\wuvis\.local\share\mamba\envs\mamba /O2 /Ob2 /DNDEBUG -std:c++17 -MD /experimental:external /external:W1 /W4 /w14242 /w14254 /w14263 /w14265 /w14287 /we4289 /w14296 /w14311 /w14545 /w14546 /w14547 /w14549 /w14555 /w14619 /w14640 /w14826 /w14905 /w14906 /w14928 /utf-8 /Zc:__cplusplus /showIncludes /Folibmamba\CMakeFiles\libmamba-dyn.dir\src\core\shell_init.cpp.obj /Fdlibmamba\CMakeFiles\libmamba-dyn.dir\ /FS -c C:\Users\wuvis\code\mamba\libmamba\src\core\shell_init.cpp
cl : Command line warning D9025 : overriding '/external:W0' with '/external:W1'
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.42.34433\include\xutility(1187): error C2794: 'reference': is not a member of any direct or indirect base class of 'std::iterator_traits<fmt::v11::appender>'
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.42.34433\include\xutility(1187): note: the template instantiation context (the oldest one first) is
C:\Users\wuvis\code\mamba\libmamba\src\core\shell_init.cpp(186): note: see reference to function template instantiation 'void fmt::v11::print<fmt::v11::detail::styled_arg<std::basic_string<char,std::char_traits<char>,std::allocator<char>>>>(std::ostream &,fmt::v11::basic_format_string<char,fmt::v11::detail::styled_arg<std::basic_string<char,std::char_traits<char>,std::allocator<char>>>>,fmt::v11::detail::styled_arg<std::basic_string<char,std::char_traits<char>,std::allocator<char>>> &&)' being compiled
C:\Users\wuvis\.local\share\mamba\envs\mamba\Library\include\fmt/ostream.h(181): note: see reference to function template instantiation 'fmt::v11::detail::format_arg_store<fmt::v11::format_context,1,0,15> fmt::v11::make_format_args<fmt::v11::format_context,fmt::v11::detail::styled_arg<std::basic_string<char,std::char_traits<char>,std::allocator<char>>>,1,0,15,0>(fmt::v11::detail::styled_arg<std::basic_string<char,std::char_traits<char>,std::allocator<char>>> &)' being compiled
C:\Users\wuvis\.local\share\mamba\envs\mamba\Library\include\fmt\base.h(2018): note: see reference to function template instantiation 'fmt::v11::detail::value<Context> fmt::v11::detail::make_arg<true,Context,fmt::v11::detail::styled_arg<std::basic_string<char,std::char_traits<char>,std::allocator<char>>>,0>(T &)' being compiled
        with
        [
            Context=fmt::v11::context,
            T=fmt::v11::detail::styled_arg<std::basic_string<char,std::char_traits<char>,std::allocator<char>>>
        ]
...

C:\Users\wuvis\.local\share\mamba\envs\mamba\Library\include\fmt/color.h(570): note: see reference to function template instantiation '_OutIt std::copy<const Char*,fmt::v11::appender>(_InIt,_InIt,_OutIt)' being compiled
        with
        [
            _OutIt=fmt::v11::appender,
            Char=char,
            _InIt=const char *
        ]
...

C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.42.34433\include\xutility(4607): note: see reference to variable template 'const bool _Iterator_is_volatile<fmt::v11::basic_appender<char> >' being compiled
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.42.34433\include\xutility(4546): note: see reference to alias template instantiation 'std::_Iter_ref_t<_Iter>' being compiled
        with
        [
            _Iter=fmt::v11::appender
        ]
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.42.34433\include\xutility(4546): error C2938: 'std::_Iter_ref_t' : Failed to specialize alias template
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.42.34433\include\xutility(4546): error C2062: type 'unknown-type' unexpected
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.42.34433\include\xutility(4607): error C3376: 'std::_Iterator_is_volatile': only static data member templates are allowed
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.42.34433\include\xutility(4723): error C2976: 'std::_Iter_copy_cat': too few template arguments
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.42.34433\include\xutility(4608): note: see declaration of 'std::_Iter_copy_cat'
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.42.34433\include\xutility(4732): error C2039: '_Bitcopy_assignable': is not a member of '`global namespace''
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.42.34433\include\xutility(4732): error C2059: syntax error: ')'
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.42.34433\include\xutility(4732): error C2143: syntax error: missing ';' before '{'

or quickly failed:

(mamba) PS C:\Users\wuvis\code\mamba> cmake --build build/
[1/252] Building CXX object libmamba\ext\solv-cpp\CMakeFiles\solv-cpp.dir\src\pool.cpp.obj
FAILED: libmamba/ext/solv-cpp/CMakeFiles/solv-cpp.dir/src/pool.cpp.obj
sccache C:\PROGRA~1\MIB055~1\2022\COMMUN~1\VC\Tools\MSVC\1442~1.344\bin\Hostx64\x64\cl.exe  /nologo /TP  -IC:\Users\wuvis\code\mamba\libmamba\ext\solv-cpp\include -external:IC:\Users\wuvis\.local\share\mamba\envs\mamba\Library\include -external:IC:\Users\wuvis\.local\share\mamba\envs\mamba\Library\share -external:W0 /DWIN32 /D_WINDOWS /GR /EHsc /D_CRT_SECURE_NO_WARNINGS /DNOMINMAX /EHsc /Zc:__cplusplus /utf-8 /MP /experimental:external /external:I C:\Users\wuvis\.local\share\mamba\envs\mamba /O2 /Ob2 /DNDEBUG -std:c++17 -MD /experimental:external /external:W1 /W4 /w14242 /w14254 /w14263 /w14265 /w14287 /we4289 /w14296 /w14311 /w14545 /w14546 /w14547 /w14549 /w14555 /w14619 /w14640 /w14826 /w14905 /w14906 /w14928 /showIncludes /Folibmamba\ext\solv-cpp\CMakeFiles\solv-cpp.dir\src\pool.cpp.obj /Fdlibmamba\ext\solv-cpp\CMakeFiles\solv-cpp.dir\ /FS -c C:\Users\wuvis\code\mamba\libmamba\ext\solv-cpp\src\pool.cpp
CreateProcess failed: The system cannot find the file specified.

Is the quick fail due to invalid cache of sccache? I've no idea how to clean sccache.

Still trying to figure out, and once I do. I guess I could send another PR to remove the warning msg: These instructions have some inaccuracies on Windows. on top of https://mamba.readthedocs.io/en/latest/developer_zone/dev_environment.html
:)

@cvanelteren
Copy link
Contributor

Did you follow the instructions on their website: https://mamba.readthedocs.io/en/latest/developer_zone/dev_environment.html

@Wuvist
Copy link
Author

Wuvist commented Jan 28, 2025

Did you follow the instructions on their website: https://mamba.readthedocs.io/en/latest/developer_zone/dev_environment.html

Yup, so I managed to build on Mac.

But their website had make it clear that "These instructions have some inaccuracies on Windows." :), guess MSVC is different from clang, and there is no instruction for building with MSVC.

@cvanelteren
Copy link
Contributor

cvanelteren commented Jan 28, 2025 via email

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

Successfully merging this pull request may close these issues.

2 participants