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

silver init doesn't recognize login shells (e.g. "-bash", "-su") #67

Closed
zackw opened this issue Nov 14, 2020 · 0 comments · May be fixed by #68
Closed

silver init doesn't recognize login shells (e.g. "-bash", "-su") #67

zackw opened this issue Nov 14, 2020 · 0 comments · May be fixed by #68
Labels
bug Something isn't working enhancement New feature or request good first issue Good for newcomers medium Medium difficulty

Comments

@zackw
Copy link

zackw commented Nov 14, 2020

silver init looks up the name of its parent process (or, in older versions, at the value of the SILVER_SHELL environment variable, which you're expected to set to $0, which comes to the same thing) and uses that to decide what type of shell is running.

If the parent process is a login shell (e.g. invoked by /usr/bin/login when you log in on a text console, or by sshd when you log in remotely) then the parent process will have a dash prepended to its name (e.g. -bash instead of bash). silver does not recognize this convention and will print a panic message to the effect of "unknown shell: -bash; supported shells: ..."

silver init should trim a leading dash from the shell variable before matching on it.

[N.B. with some implementations of su it's possible for you to get "-su" as the name of the login shell. Rather than trying to deduce the actual identity of the shell from other clues I would suggest restoring the SILVER_SHELL environment variable and treating it as a way for the user to override silver's idea of the shell in use; when it's set, don't muck around with sysinfo at all.]

@thecaralice thecaralice added good first issue Good for newcomers enhancement New feature or request bug Something isn't working medium Medium difficulty labels Nov 14, 2020
zackw added a commit to zackw/silver that referenced this issue Nov 24, 2020
All silver subcommands look up the parent process’s name, and use
that, verbatim, to identify the shell in use.  This has several
problems:

 - On Windows, the parent process’s name may or may not end with
   “.exe”; there was special case logic to handle “powershell.exe”
   versus “powershell” but not “bash.exe” versus “bash”.  Relatedly,
   the parent process’s name ought to be interpreted case-
   insensitively on Windows; BASH.EXE is the same as bash.exe.

 - On Unix, there is a notion of a _login_ shell (invoked directly by
   login(1) for console logins, or sshd(1) for remote logins); these
   are identified by a leading dash in their process name, *not* by a
   special option.  There was no code to handle this at all (bug reujab#67).

 - sysinfo is slow, because it reads a whole bunch of files in
   /proc whether it’s going to need the data within or not.
   Some but not all of this overhead can be eliminated by using
   `System::new_with_specifics(RefreshKind::new())` instead of
   `System::new_all()`, but even then the wall-clock time for
   `silver init` drops from 0.021s to 0.002s (release build)
   if sysinfo is not invoked at all.

This patch addresses all these problems, as follows:

 - There is a new command line option `--shell` which may be used to
   explicitly set the shell in use.  Also, support for the
   `SILVER_SHELL` environment variable (used for this purpose in 1.1
   and previous) is restored.  sysinfo is only consulted if neither
   the command line option, nor the environment variable, is
   available.

 - No matter where we get the process name from, it is lowercased,
   a trailing “.exe” is removed, a leading dash is removed, and any
   leading directory names are also removed (bug reujab#16).

 - The initialization scripts set `SILVER_SHELL`, so sysinfo will be
   consulted at most once per session, not once per command.

I also reorganized the code in the Command::Init branch of main for
readability’s sake, and made the “unknown shell” error message not be
a panic.  (Panics are for _bugs_, not for errors with a cause outside
the program.)
zackw added a commit to zackw/silver that referenced this issue Nov 24, 2020
All silver subcommands look up the parent process’s name, and use
that, verbatim, to identify the shell in use.  This has several
problems:

 - On Windows, the parent process’s name may or may not end with
   “.exe”; there was special case logic to handle “powershell.exe”
   versus “powershell” but not “bash.exe” versus “bash”.  Relatedly,
   the parent process’s name ought to be interpreted case-
   insensitively on Windows; BASH.EXE is the same as bash.exe.

 - On Unix, there is a notion of a _login_ shell (invoked directly by
   login(1) for console logins, or sshd(1) for remote logins); these
   are identified by a leading dash in their process name, *not* by a
   special option.  There was no code to handle this at all (bug reujab#67).

 - sysinfo is slow, because it reads a whole bunch of files in
   /proc whether it’s going to need the data within or not.
   Some but not all of this overhead can be eliminated by using
   `System::new_with_specifics(RefreshKind::new())` instead of
   `System::new_all()`, but even then the wall-clock time for
   `silver init` drops from 0.021s to 0.002s (release build)
   if sysinfo is not invoked at all.

This patch addresses all these problems, as follows:

 - There is a new command line option `--shell` which may be used to
   explicitly set the shell in use.  Also, support for the
   `SILVER_SHELL` environment variable (used for this purpose in 1.1
   and previous) is restored.  sysinfo is only consulted if neither
   the command line option, nor the environment variable, is
   available.

 - No matter where we get the process name from, it is lowercased,
   a trailing “.exe” is removed, a leading dash is removed, and any
   leading directory names are also removed (bug reujab#16).

 - The initialization scripts set `SILVER_SHELL`, so sysinfo will be
   consulted at most once per session, not once per command.

I also reorganized the code in the Command::Init branch of main for
readability’s sake, and made the “unknown shell” error message not be
a panic.  (Panics are for _bugs_, not for errors with a cause outside
the program.)
zackw added a commit to zackw/silver that referenced this issue Nov 24, 2020
All silver subcommands look up the parent process’s name, and use
that, verbatim, to identify the shell in use.  This has several
problems:

 - On Windows, the parent process’s name may or may not end with
   “.exe”; there was special case logic to handle “powershell.exe”
   versus “powershell” but not “bash.exe” versus “bash”.  Relatedly,
   the parent process’s name ought to be interpreted case-
   insensitively on Windows; BASH.EXE is the same as bash.exe.

 - On Unix, there is a notion of a _login_ shell (invoked directly by
   login(1) for console logins, or sshd(1) for remote logins); these
   are identified by a leading dash in their process name, *not* by a
   special option.  There was no code to handle this at all (bug reujab#67).

 - sysinfo is slow, because it reads a whole bunch of files in
   /proc whether it’s going to need the data within or not.
   Some but not all of this overhead can be eliminated by using
   `System::new_with_specifics(RefreshKind::new())` instead of
   `System::new_all()`, but even then the wall-clock time for
   `silver init` drops from 0.021s to 0.002s (release build)
   if sysinfo is not invoked at all.

This patch addresses all these problems, as follows:

 - There is a new command line option `--shell` which may be used to
   explicitly set the shell in use.  Also, support for the
   `SILVER_SHELL` environment variable (used for this purpose in 1.1
   and previous) is restored.  sysinfo is only consulted if neither
   the command line option, nor the environment variable, is
   available.

 - No matter where we get the process name from, it is lowercased,
   a trailing “.exe” is removed, a leading dash is removed, and any
   leading directory names are also removed (bug reujab#16).

 - The initialization scripts set `SILVER_SHELL`, so sysinfo will be
   consulted at most once per session, not once per command.

I also reorganized the code in the Command::Init branch of main for
readability’s sake, and made the “unknown shell” error message not be
a panic.  (Panics are for _bugs_, not for errors with a cause outside
the program.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request good first issue Good for newcomers medium Medium difficulty
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants