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

Terminal can be launched with a specified command #38

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lread
Copy link
Contributor

@lread lread commented Dec 5, 2021

Added option :command to API and --command command line.

  • screenshot :play/--play option is now optional
  • Adapted sample play scripts to run /bin/bash as part
    of setup. On my system I run zsh with powerlevel10k and
    my fancy prompts were getting into README screenshots.
  • On macOS and Linux :command defaults to $SHELL else
    /bin/bash
  • Included support to default :command to powershell if on
    Windows. This gets us past launch, but there
    are still be many issues on Windows.
    Contributes to Consider Supporting Windows #29
  • See README and docstrings for details on usage

Closes #3

Added option :command to API and --command command line.

- screenshot :play/--play option is now optional
- Adapted sample play scripts to run /bin/bash as part
  of setup. On my system I run zsh with powerlevel10k and
  my fancy prompts were getting into README screenshots.
- On macOS and Linux :command defaults to $SHELL else
  /bin/bash
- Included support to default :command to powershell if on
  Windows. This gets us past launch, but there
  are still be many issues on Windows.
  Contributes to phronmophobic#29
- See README and docstrings for details on usage

Closes phronmophobic#3
@lread
Copy link
Contributor Author

lread commented Dec 5, 2021

@phronmophobic, previously the hardcoded launch command was ["/bin/bash" "-l"].
The new default loses the -l arg, do we need to retain it?

@phronmophobic
Copy link
Owner

@phronmophobic, previously the hardcoded launch command was ["/bin/bash" "-l"].
The new default loses the -l arg, do we need to retain it?

To be honest, I'm not sure. From the docs, it seems like -l makes sense, but I don't remember where that came from. Additionally, I'm not 100% sure we should be automatically copying environment variables either.

It would be interesting to check what other terminal emulators are doing. Additionally, I've only been running run-term from the command line or from a repl that was launched from the command line. I wonder if it makes a difference to launch run-term more directly, eg ProcessBuilder with java -cp <classpath> <uber.jar>.

@phronmophobic
Copy link
Owner

@phronmophobic
Copy link
Owner

More references:
alacritty/alacritty#645
kovidgoyal/kitty#247

As far as I can tell, it seems like passing --login is the way to go.

@lread
Copy link
Contributor Author

lread commented Dec 5, 2021

Thanks for the references! Only skimmed, will read more carefully tomorrow.
They seem to be macOS related?
Does Linux also need the --login arg?

I'm thinking that turfing automatically using $SHELL might make sense, just because we might not be able to guess what the correct shell args are.

@phronmophobic
Copy link
Owner

phronmophobic commented Dec 7, 2021

They seem to be macOS related?
Does Linux also need the --login arg?

I've read these tickets a few times now and I'm still not 100% sure. It seems like on macOS, you want to login mainly to load .bash_profile. It doesn't seem like this is necessary on linux and might actually subvert common expectations.

Based on the tickets, it looks like rather than passing -l arg, the command is prefixed with - on macOS:

I'm thinking that turfing automatically using $SHELL might make sense, just because we might not be able to guess what the correct shell args are.

I think the above fix also would work if we use $SHELL as fallback default. This is what iTerm2 does:

Even if the above change would fix using $SHELL, that doesn't mean we should. Since we do offer a command line interface, I think it's reasonable to use the $SHELL environment variable as a fallback default. Thoughts?

@lread
Copy link
Contributor Author

lread commented Dec 7, 2021

Thanks for all the follow-up investigations! I've been distracted with advent-of-code puzzles!

From the bash man page (same text for macOS and Linux), I see:

A login shell is one whose first character of argument zero is a -, or one started with the --login option.

So maybe the 2 techniques are equivalent?

But what if I am using a different shell? I'm not very shell-variant savvy but have used zsh.

The man page for zshoptions includes:

        LOGIN (-l, ksh: -l)
              This is a login shell.  If this option is not explicitly set, the shell becomes a login shell if the first character of the argv[0] passed to the shell is a `-'.

As the veil of naivete lifts, I expect this is probably a convention?

So yeah, maybe we can still default to $SHELL and we'll document that we launch as a login shell using bash - convention. If this does not work for someone's $SHELL, they can override the default command.

But the question remains, only for macOS or also for Linux?
I haven't fully digested this yet, but it seems maybe (?) this is only needed for macOS.

@phronmophobic
Copy link
Owner

So maybe the 2 techniques are equivalent?

That's what I was thinking as well. The reason I think we should prefer using the "-" prefix over the --login arg is simply because that's what other terminal emulators are doing which makes it more likely that our default will match user expectations.

But the question remains, only for macOS or also for Linux?
I haven't fully digested this yet, but it seems maybe (?) this is only needed for macOS.

As far as I can tell, requiring a login shell is only required for macOS. That's also how the other terminal emulators seem to behave.

@lread
Copy link
Contributor Author

lread commented Dec 7, 2021

Okay, sounds good, thanks!
I shall follow up with a commit that reflects our current understanding.
(update: I did not really understand! See next comment for recap)

@lread
Copy link
Contributor Author

lread commented Dec 7, 2021

Okay, better recap the macOS login shell issue.

Due to the way Apple decided to initialize shells, on macOS only, new shells should be created as login terminals (our surveyed terminals all do this). This is not needed/wanted/necessary on Linux.

There are three options for shells macOS:

  1. do nothing. Do nothing - don't launch as login terminal
  2. args. Use -l or --login shell option to indicate login shell.
  3. prefix hack. Launch the shell in such a way that it is described with a - prefix. For example, a launch of bash would be described as -bash.

Here are the possible valid values for our new :command option

  1. user command. user-specified command (non-shell)
  2. user shell. user-specified shell
  3. default shell. either $SHELL value else /bin/bash

Note that membrane.term does not distinguish between user command and user shell, it just sees a :command.

Okay, now let's look at our options:

  1. [REJECTED] do nothing
    1. user command - this would be fine
    2. user shell - shell may not operate as expected
    3. default shell - shell may not operate as expected
  2. [CANDIDATE] args
    1. user command
      1. [NOGO] we would not want to tack on shell args to a user command
      2. the user would be in control of all args for command
    2. user shell
      1. [NOGO] we could tack on args to a user-specified shell, but would then would have to detect that the user-specified a shell vs some other command.
      2. or more likely, the user would tack them on
    3. default shell
      1. we could easily tack on shell args to defaults
      2. [POTENTIAL ISSUE] are we confident that these are the right args for all $SHELLs? maybe not, but if not the user could explicitly override the default.
  3. [CURRENT CHOICE] prefix hack - [ADDS WEIGHT] the dash prefix seems to be the preferred technique in terminals we've surveyed
    1. user command - we expect this should be harmless, but if we learn otherwise, we can later adapt with a boolean :dash-prefix-command? option.
    2. user shell - hack can be silently added to any command, it could be redundant if the user also specifies shell -l or --login shell args, but we think, not problematic.
    3. default shell - hack can be silently added to any command

Implementation complexities. Option 3 is more technically complex for membrane.term than option 2.
We use pty4j to launch our :command.
It's generic API does not allow for overriding the description of launched executable.
This probably makes sense because this is likely(?) only technically possible on macOS and Linux platforms.
Pty4j does, though, have a lower level API which we will explore for macOS.

@lread
Copy link
Contributor Author

lread commented Dec 11, 2021

Had a quick peek just now.

I so almost had a clever solution to dash prefixing on macOS with pty4j.

I figured I could create a Clojure proxy over com.pty4j.unix.UnixPtyProcess and override the exec method wherein I'd do my dash prefix bidding.

Alas, the exec method is not public, it is package-private.
I'll take another look at this sometime soon.

@lread
Copy link
Contributor Author

lread commented Dec 11, 2021

Even though we will likely do something here in the meantime, it would be better if pty4j supported this in its higher-level APIs. I've raised an issue over there to see if there is any interest.

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.

Support launching with a specific command
2 participants