Add support for the (prefix) shell #9

certik · 2025-01-22T02:35:54Z

It's not working yet, but almost:

(spawn2) conda-spawn/conda_spawn(shell)$ $CONDA_PREFIX/bin/conda spawn  -n t1 --shell shell
~/repos/conda-spawn/conda_spawn$  source "/var/folders/vg/lymvsp1153sgwwfdh3pm45
hw0000gn/T/conda-spawn-c14_qd7s.sh" && PS1="(t1) ${PS1:-}" && stty echo
.: command not found
~/repos/conda-spawn/conda_spawn$

It launches shell, but doesn't load the environment correctly yet. I think shell doesn't support . yet (prefix-dev/shell#203), only source.

The activation script contains:

. "/Users/ondrej/miniforge3/envs/spawn2/etc/conda/deactivate.d/libxml2_deactivate.sh"
unset CONDA_EXE
unset _CE_M
unset _CE_CONDA
unset CONDA_PYTHON_EXE
export PATH='/Users/ondrej/miniforge3/envs/spawn2/envs/t1/bin:/Users/ondrej/miniforge3/condabin:/Users/ondrej/.pixi/bin:/Users/ondrej/bin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/Users/ondrej/.cargo/bin:/Users/ondrej/.cache/lm-studio/bin'
export CONDA_PREFIX='/Users/ondrej/miniforge3/envs/spawn2/envs/t1'
export CONDA_SHLVL='2'
export CONDA_DEFAULT_ENV='t1'
export CONDA_PROMPT_MODIFIER='(t1) '
export CONDA_PREFIX_1='/Users/ondrej/miniforge3/envs/spawn2'

I think this is coming from conda itself, so it fails.

certik · 2025-01-22T04:47:13Z

Worked around the "." bug. Now we get:

$  source "/var/folders/vg/lymvsp1153sgwwfdh3pm45
hw0000gn/T/conda-spawn-eh3swl3r.sh" && PS1="(t1) ${PS1:-}" && stty echo
Syntax error:   × Failed to parse input
  ╰─▶ Failure to parse at Pos((3, 43))
   ╭────
 1 │ if test -n "${xml_catalog_files_libxml2:-}"; then
   ·                                           ┬
   ·                                           ╰── expected QUOTED_ESCAPE_CHAR, QUOTED_CHAR, VARIABLE_EXPANSION, SUB_COMMAND, or EXIT_STATUS
   ╰────
  help: expected QUOTED_ESCAPE_CHAR, QUOTED_CHAR, VARIABLE_EXPANSION,
        SUB_COMMAND, or EXIT_STATUS

That's another bug in the shell: prefix-dev/shell#204.

certik · 2025-01-22T05:11:06Z

I modified the file /Users/ondrej/miniforge3/envs/spawn2/etc/conda/deactivate.d/libxml2_deactivate.sh to parse as a temporary workaround for prefix-dev/shell#204 and I think we have it!

(spawn2) conda-spawn/conda_spawn(shell)$ $CONDA_PREFIX/bin/conda spawn  -n t1 --shell shell                                      
~/repos/conda-spawn/conda_spawn$  source "/var/folders/vg/lymvsp1153sgwwfdh3pm45
hw0000gn/T/conda-spawn-ji3_9fl7.sh" && PS1="(t1) ${PS1:-}" && stty echo
~/repos/conda-spawn/conda_spawn$ which python
/Users/ondrej/miniforge3/envs/spawn2/envs/t1/bin/python
~/repos/conda-spawn/conda_spawn$ 
CTRL-D
(spawn2) conda-spawn/conda_spawn(shell)$

I don't know how other shells hide the source "/var/folders/vg/lymvsp1153sgwwfdh3pm45 hw0000gn/T/conda-spawn-ji3_9fl7.sh" && PS1="(t1) ${PS1:-}" && stty echo line, but for now I don't mind, as long as it works.

conda_spawn/shell.py

conda_spawn/activate.py

certik · 2025-01-22T05:23:52Z

Here is how to use this. Check out this PR locally, then:

conda activate base
pip install .

Then launch a new terminal with the shell, put conda into your ~/bin (for example), no other setup. Then:

conda spawn --shell shell -n lf

And it just works.

certik · 2025-01-22T20:50:17Z

TODO: figure out how to automatically launch shell if running inside shell.

certik · 2025-01-22T21:20:42Z

@jaimergp this is ready for review. I tested it with the shell on both macOS and Windows, it works great on both systems. There are some bugs in the shell that we need to fix, but on the conda side it seems to work great.

An example on Windows:

The "OK" string comes from my temporary local modifications of some of the Conda scripts, since the shell can't execute all of them yet. You can ignore it.

Example on macOS in ghostty and the shell:

certik · 2025-01-22T21:33:53Z

conda_spawn/shell.py

@@ -275,6 +308,8 @@ def default_shell_class():
 def detect_shell_class():
    try:
        name, _ = shellingham.detect_shell()
+        if sys.platform == "win32" and name == "cmd" and "SHELL" in os.environ:
+            name = os.environ["SHELL"]


This is needed on Windows, since it incorrectly recognizes the shell as cmd.exe and then proceeds with cmd instead of shell. In this case we simply check the SHELL variable, and use it if it is set, which fixes the problem.

It would be nice to add support for Prefix's shell in shellingham.

Ok, working on it. Shellingham recognizes shell on macOS but not Windows. I'll fix it.

Ok, so this is tricky. With sarugaku/shellingham#92 shellingham now correctly recognizes shell on Windows. However, on Windows conda is launched from conda.bat scripts and so technically conda is still launched from cmd.exe even though the actual shell is different.

I don't know how to fix that, other than what we did above. Or, we can try to figure out how to launch conda directly, which should be preferable on Windows, it actually works:

~/repos/math_notes(main)$ ~/AppData/Local/miniforge3/Scripts/conda.exe spawn myst (myst) ~/repos/math_notes(main)$ CTRL-D ~/repos/math_notes(main)$ conda spawn myst (myst) C:\Users\ondrejcertik\repos\math_notes>

I think I found a solution:

~$ cat bin/conda2.bat @shell -c "C:\Users\ondrejcertik\AppData\Local\miniforge3\Scripts\conda.exe %*" ~$ conda2 spawn myst (myst) ~$

Ok, the old solution was:

if sys.platform == "win32" and name == "cmd" and "SHELL" in os.environ: name = os.environ["SHELL"]

But it's not needed anymore.

conda_spawn/shell.py

jaimergp · 2025-01-29T01:45:38Z

conda_spawn/shell.py

+            self._files_to_remove.append(f.name)
+
+    def spawn(self, command: Iterable[str] | None = None) -> int:
+        proc = self.spawn_popen(command)


Can't we use the tty in Unix systems?

We probably can. I wanted to keep the code uniform to work on all platforms. Do you want to have different code for Windows and Unix?

Ttys are needed for PS1 support I'm afraid.

I was using the example from PowerShell. Do ttys work on Windows?

Regarding the PS1 support, that is now fixed in 8789c31, everything works:

~$ conda spawn lf (lf) ~$ CTRL-D ~$

I can confirm that PS1 works on Windows also:

In some shells, the PS1 variable is not ever exported, only local to the current session. So a process like conda's doesn't "see it" when it wants to expand it with something like PS1 = os.environ.get("PS1, "") + " (base)".

But if Prefix's shell always exports it I guess we are ok.

Currently we are not exporting $PS1. Can you give an example of a conda command (besides conda activate which is now replaced with conda spawn) that requires to modify $PS1?

jaimergp

Thanks for the contribution. I also talked with Wolf about this but right now I don't feel like conda spawn should be accepting new shell features. I'm still figuring out the ideal UX with the existing conda support (and some of them are missing, like csh or fish).

That said, it's not like this won't ever be merged. Just not right now. Things that would need to happen besides conda spawn being a bit more mature / recognized in the ecosystem:

Tests
Ability to specify a command
Prompt modification support
Shellingham support (ideally)

Hope you can understand!

certik · 2025-01-29T03:11:21Z

Thanks. I opened up issues for the things that you mentioned:

Tests: already done (cargo test works)
Ability to specify a command: Add an ability to specify a command prefix-dev/shell#211
Prompt modification support: Make the prompt configurable (via $PS1 or similar) prefix-dev/shell#200
Shellingham support (ideally): Add shell support to Shellingham prefix-dev/shell#212, PR submitted: Add support for (prefix) shell sarugaku/shellingham#92

Update: the main issues are now fixed, and I'll fix Shellingham soon. @jaimergp let me know what else you need on the shell side.

jaimergp · 2025-01-29T10:43:34Z

Awesome! And very quick too.

The first three items I mentioned were directed towards this PR: we need to add tests for the new shell (not sure at what level, this might be trickier than with default system shells), and also implement support for running commands from the CLI and modifying the PS1 prompt here.

certik · 2025-01-30T00:41:33Z

@jaimergp ok, I think everything works for me now. Commands work:

~/repos/conda-spawn(shell)$ conda spawn myst -- myst --help
Usage: myst [options] [command]

Options:
  -v, --version               Print the current version of MyST
  -d, --debug                 Log out any errors to the console
  -h, --help                  display help for command

Commands:
  init [options]              Initialize a MyST project in the current
                              directory
  build [options] [files...]  Build PDF, LaTeX, Word and website exports from
                              MyST files
  start [options]             Start the current project as a website
  clean [options] [files...]  Remove exports, temp files and installed
                              templates
  templates                   List and download templates

Environment works:

~/repos/conda-spawn(shell)$ conda spawn myst
(myst) ~/repos/conda-spawn(shell)$ which myst
C:\Users\ondrejcertik\AppData\Local\miniforge3\envs\myst\Scripts\myst.exe

The code seems clean, all the issues you raised are addressed.

Except testing of shell in this PR. This can be done by conda installing it at the CI, and then testing it that way. We have to make a new shell release for that, as I fixed a few things there since the last release. But other than that, let me know what else is needed here to get this merged.

jaimergp · 2025-01-31T21:00:44Z

conda_spawn/shell.py

+                delete=False,
+                mode="w",
+            ) as f:
+                f.write(self.script())


Don't you need a newline here too?

I assumed self.script() would end with a new line. I should add it here just in case.

jaimergp · 2025-01-31T21:03:12Z

conda_spawn/shell.py

@@ -164,6 +164,41 @@ def executable(self):
        return "zsh"


+class ShellShell(PosixShell):


I think we should rename this to avoid confusion:

Suggested change

class ShellShell(PosixShell):

class PrefixShellShell(PosixShell):

jaimergp · 2025-01-31T21:05:23Z

conda_spawn/activate.py

@@ -1077,6 +1077,12 @@ def _hook_postamble(self) -> str:
        return "Remove-Variable CondaModuleArgs"


+class ShellActivator(PosixActivator):
+    pathsep_join = ";".join if on_win else ":".join


This is surprising 🤔 Isn't shell supposed to be cross-platform? Why do they need different separators 🤔 Just curious.

This is actually at the core of the issue of being cross platform. On Windows the $PATH environment variable contains ;, on Linux and macOS it contains : as a separator. Consequently, there are two ways to deal with this: we either emulate a more posix like environment (posix requires :) from inside the script, but we must revert back to $PATH with ; for any programs that we call from the script, otherwise they won't work, since on Windows programs expect ; as a separator (that is for example what Xonsh or git-bash is doing); or you keep things native, you don't emulate anything. And that is what the shell is doing.

This is related to an issue of absolute paths: posix doesn't allow paths like C:\something, each absolute path must start with /. Again, one has two options, emulating paths and translating them for programs (git-bash) or using native paths (shell).

In order to write cross-platform scripts, one must either user relative paths, or use some platform-independent mechanism to append to $PATH.

jaimergp · 2025-01-31T21:07:24Z

I don't know how other shells hide the source "/var/folders/vg/lymvsp1153sgwwfdh3pm45 hw0000gn/T/conda-spawn-ji3_9fl7.sh" && PS1="(t1) ${PS1:-}" && stty echo line, but for now I don't mind, as long as it works.

That's because they are using the tty mode, not the popen one. I think it's worth it, but we can also do that in a second PR.

jaimergp · 2025-01-31T21:09:32Z

Except testing of shell in this PR. This can be done by conda installing it at the CI, and then testing it that way. We have to make a new shell release for that, as I fixed a few things there since the last release. But other than that, let me know what else is needed here to get this merged.

You can add it to the test requirements in pixi.toml and then copy some of the basic tests to at least have something.

certik · 2025-01-31T22:55:23Z

That's because they are using the tty mode, not the popen one. I think it's worth it, but we can also do that in a second PR.

Do you have an example how to use the tty mode on Windows? The modules that you use on linux do not seem to be available on Windows.

jaimergp · 2025-02-03T08:23:14Z

Do you have an example how to use the tty mode on Windows?

So far I only figured it out in Unix, so that'd be enough for now.

certik · 2025-02-03T16:44:23Z

So far I only figured it out in Unix, so that'd be enough for now.

Ok. Do you suggest to use tty for Linux/macOS and the current approach on Windows? (I need this to run on Windows also.)

certik commented Jan 22, 2025

View reviewed changes

conda_spawn/shell.py Outdated Show resolved Hide resolved

certik commented Jan 22, 2025

View reviewed changes

conda_spawn/shell.py Outdated Show resolved Hide resolved

certik commented Jan 22, 2025

View reviewed changes

conda_spawn/activate.py Outdated Show resolved Hide resolved

certik mentioned this pull request Jan 22, 2025

Figure out how to enable Conda environments inside the shell prefix-dev/shell#36

Open

certik force-pushed the shell branch 2 times, most recently from 7b6b9b2 to a555fc9 Compare January 22, 2025 20:38

certik marked this pull request as ready for review January 22, 2025 21:18

certik commented Jan 22, 2025

View reviewed changes

jaimergp reviewed Jan 29, 2025

View reviewed changes

conda_spawn/shell.py Show resolved Hide resolved

jaimergp reviewed Jan 29, 2025

View reviewed changes

jaimergp requested changes Jan 29, 2025

View reviewed changes

certik added 10 commits January 29, 2025 07:25

Add support for the (prefix) shell

2a730c4

Fix args and source

aaef7db

Use source instead of .

3a6d384

Fix formatting

f768060

Use pipes directly, no pexpect on Windows

2b7a264

Makes it work

ee83339

Split into ShellActivator

ec9f986

Use it

8b96476

Fix formatting

f058762

Use the SHELL variable on Windows if it exists

5352a07

certik force-pushed the shell branch from a59e1a0 to 5352a07 Compare January 29, 2025 14:27

Execute command if provided

3ea6774

certik added 2 commits January 29, 2025 07:42

Change the PS1 prompt

8789c31

Remove the workaround on Windows

0cd06d9

jaimergp reviewed Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the (prefix) shell #9

Add support for the (prefix) shell #9

certik commented Jan 22, 2025 •

edited

Loading

certik commented Jan 22, 2025

certik commented Jan 22, 2025

certik commented Jan 22, 2025

certik commented Jan 22, 2025

certik commented Jan 22, 2025 •

edited

Loading

certik Jan 22, 2025

jaimergp Jan 29, 2025

certik Jan 29, 2025

certik Jan 30, 2025

certik Jan 30, 2025

certik Jan 30, 2025

jaimergp Jan 29, 2025

certik Jan 29, 2025

jaimergp Jan 29, 2025

certik Jan 29, 2025

certik Jan 29, 2025

certik Jan 29, 2025

jaimergp Jan 31, 2025

certik Jan 31, 2025

jaimergp left a comment

certik commented Jan 29, 2025 •

edited

Loading

jaimergp commented Jan 29, 2025

certik commented Jan 30, 2025

jaimergp Jan 31, 2025

certik Jan 31, 2025

jaimergp Jan 31, 2025

jaimergp Jan 31, 2025

certik Jan 31, 2025 •

edited

Loading

jaimergp commented Jan 31, 2025

jaimergp commented Jan 31, 2025

certik commented Jan 31, 2025

jaimergp commented Feb 3, 2025

certik commented Feb 3, 2025

		@@ -164,6 +164,41 @@ def executable(self):
		return "zsh"


		class ShellShell(PosixShell):

	class ShellShell(PosixShell):
	class PrefixShellShell(PosixShell):

Add support for the (prefix) shell #9

Are you sure you want to change the base?

Add support for the (prefix) shell #9

Conversation

certik commented Jan 22, 2025 • edited Loading

certik commented Jan 22, 2025

certik commented Jan 22, 2025

certik commented Jan 22, 2025

certik commented Jan 22, 2025

certik commented Jan 22, 2025 • edited Loading

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

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

jaimergp left a comment

Choose a reason for hiding this comment

certik commented Jan 29, 2025 • edited Loading

jaimergp commented Jan 29, 2025

certik commented Jan 30, 2025

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

certik Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

jaimergp commented Jan 31, 2025

jaimergp commented Jan 31, 2025

certik commented Jan 31, 2025

jaimergp commented Feb 3, 2025

certik commented Feb 3, 2025

certik commented Jan 22, 2025 •

edited

Loading

certik commented Jan 22, 2025 •

edited

Loading

certik commented Jan 29, 2025 •

edited

Loading

certik Jan 31, 2025 •

edited

Loading