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.

Sign up for GitHub

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

Add support for the (prefix) shell #9

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Add support for the (prefix) shell #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2a730c4
Add support for the (prefix) shell
certik Jan 22, 2025
aaef7db
Fix args and source
certik Jan 22, 2025
3a6d384
Use `source` instead of `.`
certik Jan 22, 2025
f768060
Fix formatting
certik Jan 22, 2025
2b7a264
Use pipes directly, no pexpect on Windows
certik Jan 22, 2025
ee83339
Makes it work
certik Jan 22, 2025
ec9f986
Split into ShellActivator
certik Jan 22, 2025
8b96476
Use it
certik Jan 22, 2025
f058762
Fix formatting
certik Jan 22, 2025
5352a07
Use the SHELL variable on Windows if it exists
certik Jan 22, 2025
3ea6774
Execute command if provided
certik Jan 29, 2025
8789c31
Change the PS1 prompt
certik Jan 29, 2025
0cd06d9
Remove the workaround on Windows
certik Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions conda_spawn/activate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,6 +1077,12 @@ def _hook_postamble(self) -> str:
return "Remove-Variable CondaModuleArgs"


class ShellActivator(PosixActivator):
pathsep_join = ";".join if on_win else ":".join
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@certik certik Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

path_conversion = staticmethod(_path_identity)
run_script_tmpl = 'source "%s"'


activator_map: dict[str, type[_Activator]] = {
"posix": PosixActivator,
"ash": PosixActivator,
Expand All @@ -1089,4 +1095,5 @@ def _hook_postamble(self) -> str:
"cmd.exe": CmdExeActivator,
"fish": FishActivator,
"powershell": PowerShellActivator,
"shell": ShellActivator,
}
36 changes: 36 additions & 0 deletions conda_spawn/shell.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,41 @@ def executable(self):
return "zsh"


class ShellShell(PosixShell):
Copy link
Member

Choose a reason for hiding this comment

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

I think we should rename this to avoid confusion:

Suggested change
class ShellShell(PosixShell):
class PrefixShellShell(PosixShell):

Activator = activate.ShellActivator

def executable(self):
return "shell"

def args(self):
return ("--interact", "--norc")

def spawn_popen(
self, command: Iterable[str] | None = None, **kwargs
) -> subprocess.Popen:
try:
with NamedTemporaryFile(
prefix="conda-spawn-",
suffix=self.Activator.script_extension,
delete=False,
mode="w",
) as f:
f.write(self.script())
Copy link
Member

Choose a reason for hiding this comment

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

Don't you need a newline here too?

Copy link
Author

Choose a reason for hiding this comment

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

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

f.write(f"{self.prompt()}\n")
if command:
f.write(" ".join(command))
return subprocess.Popen(
[self.executable(), *self.args(), f.name], env=self.env(), **kwargs
)
finally:
self._files_to_remove.append(f.name)

def spawn(self, command: Iterable[str] | None = None) -> int:
proc = self.spawn_popen(command)
Copy link
Member

Choose a reason for hiding this comment

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

Can't we use the tty in Unix systems?

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Ttys are needed for PS1 support I'm afraid.

Copy link
Author

Choose a reason for hiding this comment

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

I was using the example from PowerShell. Do ttys work 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.

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

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

Copy link
Author

Choose a reason for hiding this comment

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

I can confirm that PS1 works on Windows also:

image

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

proc.communicate()
return proc.wait()


class CshShell(Shell):
pass

Expand Down Expand Up @@ -260,6 +295,7 @@ def args(self) -> tuple[str, ...]:
"posix": PosixShell,
"powershell": PowershellShell,
"pwsh": PowershellShell,
"shell": ShellShell,
"tcsh": CshShell,
"xonsh": XonshShell,
"zsh": ZshShell,
Expand Down
Loading