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

Try using bash for login startup #12058

Draft
wants to merge 1 commit into
base: master
from
Draft

Conversation

@rjmholt
Copy link
Member

rjmholt commented Mar 6, 2020

PR Summary

Fixes #12020.

PR Context

Profiles on some Linux systems contain bashisms that are not /bin/sh compatible, causing issues.

By trying bash first, we eliminate this problem.

PR Checklist

@@ -327,6 +327,12 @@ private static int ExecPwshLogin(string[] args, string pwshPath, bool isMacOS)
return Exec("/bin/zsh", execArgs);

This comment has been minimized.

Copy link
@PoshChan

PoshChan Mar 6, 2020

Collaborator

@rjmholt, your last commit had 1 failures in PowerShell-CI-static-analysis
Verify Markdown Links.Verify links in /home/vsts/work/1/s/CHANGELOG/README.md.preview.md should work

Tool reported URL as unreachable.
at <ScriptBlock>, /home/vsts/work/1/s/test/common/markdown/markdown-link.tests.ps1: line 124
124:                             throw "Tool reported URL as unreachable."
@gdoenlen

This comment has been minimized.

Copy link

gdoenlen commented Mar 6, 2020

Thanks @rjmholt. I was just about to do this myself when I saw your pr.

Much appreciated.

@rjmholt

This comment has been minimized.

Copy link
Member Author

rjmholt commented Mar 6, 2020

Much appreciated.

No worries. I'm responsible for this feature and have been wanting to fix it in WSL (especially because setting pwsh as your login shell there causes an error that's very hard to fix).

With your analysis, it makes sense why it wasn't working now, and I'm happy to enable the fix.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

iSazonov commented Mar 11, 2020

I think this can have negative consequences. What if user have some shells installed (this is a common case) on a system but configure tcsh as default and configure paths only for tcsh? In the case we call bash and get wrong paths.
If we want to make a workaround for dash we have two cases:

  • add explicit check for dash - it is very slow
  • catch error and call bash as last resort - I think it is best way.
@rjmholt

This comment has been minimized.

Copy link
Member Author

rjmholt commented Mar 11, 2020

tcsh

That's not a POSIX-compliant shell. We should be able to execute POSIX shell script from any POSIX-compliant shell; that's the definition of POSIX-compliant. The scripts and paths should be written for any /bin/sh to execute them, and our choice of implementation doesn't matter.

The issue is that dash is the default /bin/sh on some platforms and doesn't like the way paths are passed, while bash and zsh have no problem with it.

zsh is guaranteed on supported macOS versions, so that's ok. But on Linux we just don't know. However, bash is so very common that (1) it's what people actually write their scripts for when they mean to target /bin/sh and (2) it's probably worth looking for.

I agree that I don't like looking for another file at startup, but (1) it's only on Linux when -Login is used and (2) it's probably not actually that expensive. It's also very simple.

catch error and call bash as last resort - I think it is best way.

What do we catch though? We're p/Invoking into a syscall, so there's no exception. Theoretically it returns an error code, except it's exec so when you call it your whole process gets paved over. We successfully start the new program, but it's taken over our process, so when it fails later it's not the exec call that fails, it's our initial pwsh process.

The only solution to that is making the /bin/sh login/exec script failsafe, but then we need to pass the failure information into pwsh in the exec call and use it to tell ourselves to start bash. That's now a third CLR startup, which involves plenty of file reads as assemblies are loaded into memory... So I think testing for bash is going to be faster.

@rjmholt

This comment has been minimized.

Copy link
Member Author

rjmholt commented Mar 11, 2020

I just need to test this PR

@daxian-dbw

This comment has been minimized.

Copy link
Member

daxian-dbw commented Mar 13, 2020

@rjmholt Do dash and bash load the same profile files at startup?
Also, could this be an issue to Windows integration? It doesn't work with dash after all.

@@ -327,6 +327,12 @@ private static int ExecPwshLogin(string[] args, string pwshPath, bool isMacOS)
return Exec("/bin/zsh", execArgs);

This comment has been minimized.

Copy link
@mklement0

mklement0 Mar 16, 2020

Contributor

On macOS,/bin/sh is, in essence, bash (it's a separate binary that is a custom build of bash with a few changes hard-coded for better POSIX-compliance; verify with sh --version). As such, it does support the -l option, and I believe it should be used - no need to bring zsh into the mix, which can have side effects.

This comment has been minimized.

Copy link
@mklement0

mklement0 Mar 16, 2020

Contributor

Thinking some more about this: Since zsh is being invoked in POSIX-compatibility mode, there's probably little practical difference to using sh.

The zsh executable is about double the size of sh, but in terms of execution speed users probably won't notice the difference. [Update: The ratio is actually about 20, but sh is possibly just a shim; the /bin/bash executable is only slightly smaller than /bin/zsh]

If zsh continues to be used, I suggest at least amending the misleading comment.

This comment has been minimized.

Copy link
@rjmholt

rjmholt Mar 16, 2020

Author Member

No, we tried this. The version of bash it uses is too old to support -l in sh-compatibility mode and it wasn't starting properly. It's why the specific /bin/zsh behaviour exists; the version of zsh is much newer on macOS, and unlike bash, will be there in future versions. Rather than us wait for the executable to change, we just go straight for /bin/zsh.

The comment reflects the actual experimentation we did.

Anyway, this PR is about what shell we start with on Linux.

This comment was marked as outdated.

Copy link
@mklement0

mklement0 Mar 16, 2020

Contributor

Understood re focus of this PR; I just wanted to get the conversation started.

bash on macOS has been frozen at v3.2.57 for many versions of macOS, for licensing reasons, and won't get updated.

Even if it gets removed, /bin/sh (the system default shell, as opposed to the interactive user default shell, which is now zsh) must and will remain a (mostly) POSIX-compliant shell - whatever concrete shell act as /bin/sh.

sh on macOS definitely supports the -l option - just try /bin/sh -l, and you'll see that it processes both /etc/profile and ~/.profile without complaint (assuming POSIX-compliant scripts).

Are you saying that the option is supported but not correctly?

This comment has been minimized.

Copy link
@mklement0

mklement0 Mar 16, 2020

Contributor

I've given the tangent a new home: #12134

Also, with respect to this PR: I suggest holding off until we understand the root cause of #12020 update: Never mind, see #12020 (comment) (sigh)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.