Add support for the (prefix) shell - #9
Conversation
|
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_STATUSThat's another bug in the shell: prefix-dev/shell#204. |
|
I modified the file (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 |
|
Here is how to use this. Check out this PR locally, then: Then launch a new terminal with the And it just works. |
7b6b9b2 to
a555fc9
Compare
|
TODO: figure out how to automatically launch |
|
@jaimergp this is ready for review. I tested it with the 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:
|
| try: | ||
| name, _ = shellingham.detect_shell() | ||
| if sys.platform == "win32" and name == "cmd" and "SHELL" in os.environ: | ||
| name = os.environ["SHELL"] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It would be nice to add support for Prefix's shell in shellingham.
There was a problem hiding this comment.
Ok, working on it. Shellingham recognizes shell on macOS but not Windows. I'll fix it.
There was a problem hiding this comment.
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>There was a problem hiding this comment.
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) ~$
There was a problem hiding this comment.
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.
| self._files_to_remove.append(f.name) | ||
|
|
||
| def spawn(self, command: Iterable[str] | None = None) -> int: | ||
| proc = self.spawn_popen(command) |
There was a problem hiding this comment.
Can't we use the tty in Unix systems?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Ttys are needed for PS1 support I'm afraid.
There was a problem hiding this comment.
I was using the example from PowerShell. Do ttys work on Windows?
There was a problem hiding this comment.
Regarding the PS1 support, that is now fixed in 8789c31, everything works:
~$ conda spawn lf
(lf) ~$
CTRL-D
~$
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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!
|
Thanks. I opened up issues for the things that you mentioned:
Update: the main issues are now fixed, and I'll fix Shellingham soon. @jaimergp let me know what else you need on the |
|
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. |
|
@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 templatesEnvironment 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.exeThe code seems clean, all the issues you raised are addressed. Except testing of |
| delete=False, | ||
| mode="w", | ||
| ) as f: | ||
| f.write(self.script()) |
There was a problem hiding this comment.
Don't you need a newline here too?
There was a problem hiding this comment.
I assumed self.script() would end with a new line. I should add it here just in case.
| return "zsh" | ||
|
|
||
|
|
||
| class ShellShell(PosixShell): |
There was a problem hiding this comment.
I think we should rename this to avoid confusion:
| class ShellShell(PosixShell): | |
| class PrefixShellShell(PosixShell): |
|
|
||
|
|
||
| class ShellActivator(PosixActivator): | ||
| pathsep_join = ";".join if on_win else ":".join |
There was a problem hiding this comment.
This is surprising 🤔 Isn't shell supposed to be cross-platform? Why do they need different separators 🤔 Just curious.
There was a problem hiding this comment.
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.
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. |
You can add it to the |
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. |
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.) |
|
Hi there, thank you for your contribution! This pull request has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further activity occurs. If you would like this pull request to remain open please:
NOTE: If this pull request was closed prematurely, please leave a comment. Thanks! |



It's not working yet, but almost:
It launches
shell, but doesn't load the environment correctly yet. I thinkshelldoesn't support.yet (prefix-dev/shell#203), onlysource.The activation script contains:
I think this is coming from
condaitself, so it fails.