Ticket #3748 (closed enhancement: fixed)
mksh as a subshell
Reported by: | gloomyquazar | Owned by: | zaytsev |
---|---|---|---|
Priority: | minor | Milestone: | 4.8.33 |
Component: | mc-core | Version: | master |
Keywords: | subshell mksh | Cc: | d3m3t3r |
Blocked By: | Blocking: | ||
Branch state: | merged | Votes for changeset: | committed-master |
Description
Hello.
On my desktop and notebook I use FreeBSD together with mksh, which I like for it's small size, robustness, security... Recently I got to know that MC supports only bash, (t)csh, zsh and something else as main shells, hence my mksh doesn't work as a subshell, I can't just switch with Ctrl+O and work there, so I had to postpone using MC for now. Is there a way to add mksh for supported shells? Without a subshell working in MC gets very unconvenient.
With regards,
Sergey P.
Attachments
Change History
comment:2 in reply to: ↑ 1 Changed 8 years ago by andrew_b
- Blocked By 3692 added
- Milestone changed from 4.8.19 to Future Releases
Replying to gloomyquazar:
It would be nice to also add support for general Bourne /bin/sh as a subshell.
comment:3 Changed 8 years ago by zaytsev
If mksh has an equivalent of precmd (which, I think, it does) and you can cook up a patch to support it that is not impossible.
comment:4 Changed 8 years ago by gloomyquazar
Maybe the function like this will do?
function precmd {
<put something here>
}
PS1="$(precmd) $PS1 "
comment:5 Changed 8 years ago by zaytsev
As I said, if you manage to provide a clean working patch, we might eventually get it in.
comment:6 Changed 8 years ago by gloomyquazar
Could you please check this patch? It works OK for me.
comment:9 in reply to: ↑ 8 Changed 3 months ago by d3m3t3r
Replying to zaytsev:
Somebody turned this in a PR:
Added support for ksh/oksh/mksh Korn shell variants.
comment:10 Changed 3 months ago by zaytsev
- Milestone changed from Future Releases to 4.8.33
I assume you tested it and it works for you?
- I'm surprised that the "precmd" part was so simple. Is that due to static prompt? Is supporting user prompt impossible / problematic?
- I'm not sure that ksh supports HISTCONTROL... Does it really? If yes, good news.
comment:11 Changed 3 months ago by d3m3t3r
I need to fix it for mksh which is actually dumber than other pdksh/openbsd based variants. mksh doesn't support HISTCONTROL either \x placeholders in PS1 and recognizes not just ENV but $HOME/.mkshrc too.
Couldn't find any "precmd" like functionality in any variant so I suppose user prompt is not possible.
Since mksh seems to be quite different from other variants, would you recommend using distinct SHELL_KSH and SHELL_MKSH types? Or perhaps use single SHELL_KSH type but specific mc_shell->name?
comment:12 Changed 3 months ago by zaytsev
Since mksh seems to be quite different from other variants
I would add SHELL_MKSH if it's different enough, and SHELL_OKSH / SHELL_PDKSH (whichever official names make sense, didn't look into it) for clarity - you can use multiple enum values for the same case.
Couldn't find any "precmd" like functionality in any variant so I suppose user prompt is not possible.
It would be great to have this as a comment. Otherwise our children will wonder...
mksh doesn't support HISTCONTROL
But the others do, or everything is trashed with our cd commands?
comment:13 Changed 3 months ago by d3m3t3r
First, thanks much for the feedback. I've split mksh support under its own SHELL_MKSH type since its features differ so much it makes sense to handle it separately.
Some overview: There seems to be several implementations of ksh around but all but mksh (MirBSD ksh https://github.com/MirBSD/mksh) are based on pdksh (Public Domain ksh), in particular ports of OpenBSD ksh, e.g. https://github.com/dimkr/loksh and https://github.com/ibara/oksh. pdksh supports \x codes, variable and command substitution in PS1, HISTCONTROL (ignorespace & ignoredups) and ENV variable for interactive shell rc-file. mksh, on the other hand, doesn't support \x codes in PS1 either HISTCONTROL. It supports ENV variable for interactive shell rc-file but if it's not set the shell also tries ~/.mkshrc.
comment:16 Changed 2 months ago by zaytsev
Sorry, I still don't have time to look into it deeper... but I wonder, can actually the plain POSIX sh be supported with the fallback? Maybe you have the answer since you looked deeper into it.
comment:17 Changed 2 months ago by d3m3t3r
but I wonder, can actually the plain POSIX sh be supported with the fallback? Maybe you have the answer since you looked deeper into it.
I don't think the precmd fallback emulation in mc supports the plain POSIX shell because it relies on the command substitution in PS1 which according to the specification (https://pubs.opengroup.org/onlinepubs/9799919799/utilities/V3_chap02.html#tag_19_05_03) is unspecified.
comment:18 Changed 2 months ago by d3m3t3r
There's summary of the changes in my PR:
- Add support for mksh - uses fallback precmd emulation.
- Add support for pdksh-based ksh (e.g. OpenBSD's ksh) - supports user prompt.
- Enhance fallback precmd emulation to support user prompt (affect ash/dash/mksh).
I tested on Alpine Linux with busybox, dash, mksh, oksh (OpenBSD's ksh port) and loksh (another OpenBSD's ksh port) packages.
comment:20 Changed 10 days ago by zaytsev
- Branch state changed from no branch to on review
Branch: 3748_ksh_support
Initial changeset: b64512b750351d83691c02e5bf118d3e00dae85f
I have added some fixups, but it me it looks good. Thank you for your high quality contribution!
comment:22 Changed 10 days ago by ossi
i haven't looked very closely, but it doesn't seem right to me that a fallback to .profile is forced.
line 385 has a copy&pasto.
line 1199 has incorrect indentation.
comment:23 Changed 10 days ago by zaytsev
i haven't looked very closely, but it doesn't seem right to me that a fallback to .profile is forced.
Why? It seems to be the default for ksh according to the man pages.
line 385 has a copy&pasto.
line 1199 has incorrect indentation.
Both are fixed in subsequent commits. I didn't want to squash them because of the inline explanations.
comment:24 Changed 10 days ago by ossi
huh, i didn't notice that this is a patch _series_. 🤦🏻♂️
(did i already mention this year that i hate trac as a review platform?)
well, if it's the default, then it needs no override. but i suppose it needs some "convincing", because it's not a login shell.
i'm a bit wary about messing with the startup files in general, because i know what an utter mess it is, how some shells aren't even consistent with themselves (bash has build-time switches), and how everybody tries to work around it their own way. (you will find several "treatises" from me on that matter if you google hard enough.) and sourcing .profile in lieu of an rc file doesn't make things better. ah, well.
the merge's diff shows that the indentation mistake persists.
which inline explanations?
i'm an atomicity pedant, so i'd split the fixup patches and squash the hunks where appropriate. shared authorship can be documented in footers.
in the same vein, a commit near the end of the series contains some unrelated comment whitespace changes that i'd split off (or just discard).
but this is a matter of project policy, so whatever.
comment:25 Changed 10 days ago by zaytsev
Just tested on OpenIndiana with ksh and ksh93 - the new shell integration is working very nicely and is comfortable to use. The history is clobbered with commands though, because they don't seem to support HISTCONTROL.
I have added a commit to support ksh93 binary, I will squash commits before merge.
comment:26 Changed 8 days ago by andrew_b
- Votes for changeset set to andrew_b
- Branch state changed from on review to approved
comment:27 Changed 7 days ago by zaytsev
- Status changed from accepted to testing
- Votes for changeset changed from andrew_b to committed-master
- Resolution set to fixed
- Branch state changed from approved to merged
Merged to master: b683b6388e9249ab986d764763df1cb82fd1d888
comment:29 Changed 7 days ago by zaytsev
- Cc d3m3t3r added
Now testing the latest master on alpine and ash prompt seems to been affected.
- Before merge the prompt was zaytsev@alpine$ .
- After the merge the prompt is ~ $.
The default PS1 on Alpine should be \h:\w\$ . However, when starting mc as mc it somehow gets lost!
- If I start mc as PS1="\h:\w\$ " mc, then the default prompt is indeed used.
- If I start mc as PS1= mc, then our fallback is used.
- If I start mc as mc, then this special mini-prompt is used.
I think this might have to do with the following ticket:
https://gitlab.alpinelinux.org/alpine/aports/-/issues/12398
I'm not sure what to think of it:
On one hand, I guess we are now actually doing the "right thing".
On the other hand, we are not doing this for bash and zsh, only for fish to a certain extent.
Then again, I guess the reasoning for bash and zsh is that extra prompt stuff will always break the persistent buffer. For fish... no idea.
comment:30 Changed 7 days ago by zaytsev
I'm wrong, actually we are supporting custom PS1 for all other shells (bash, zsh), so, I guess the new way is the right way.
Also noted that there is a mysterious ^H hanging in the background for dash, but it was there before. Added a comment to #3010.
comment:31 Changed 5 days ago by d3m3t3r
For ash shell, the subshell would try to use $ENV, then ~/.profile and finally ~/.local/share/mc/ashrc rc files (e.g. shell-specific-rc-env-var, shell-specific-rc-file, mc-custom-shell-specific-rc-file just like with most other supported shells). In the subshell, PS1 (all environment variables in general) set in the selected rc-file would override PS1 inherited from mc itself. However, PS1 is somewhat special as we actually need to adjust it for the precmd-hack needed in some shells. My change allowed this "adjustment" to retain the original PS1 (see MC_PS1_SAVED) in the fallback prompt. Same thing can easily be done for other shells not using the fallback prompt but I considered it too far from the original scope of this ticket ("mksh").
On one hand, I guess we are now actually doing the "right thing".
Exactly. My assumption is the user would like to have his customized PS1 in the subshell.
The default PS1 on Alpine should be \h:\w\$ . However, when starting mc as mc it somehow gets lost!
I believe it's because this PS1 is set in /etc/profile which is not used when ash is executed in the subshell by mc.
comment:32 Changed 5 days ago by zaytsev
Same thing can easily be done for other shells not using the fallback prompt but I considered it too far from the original scope of this ticket ("mksh").
But I think this works already. I customized my PS1 for zsh in .zshrc and it's working in mc - same goes for bash, not sure about fish, but I this that it does as well. The only other suspect is tcsh...
Exactly. My assumption is the user would like to have his customized PS1 in the subshell.
Yes, now I get it. But initially I was concerned that the default has become less useful, it doesn't even include pwd :( but then again, probably anyone seriously using these shells is either OK with that, or has customized the prompt already.
I believe it's because this PS1 is set in /etc/profile which is not used when ash is executed in the subshell by mc.
I guess so, I tried to look into ways of telling ash/dash that it will be started as a login shell, but found nothing. Oh well, probably we'll just have to let the sh*t hit the fan and see if there is any backlash.
comment:33 Changed 4 hours ago by d3m3t3r
Actually, it seems to me the custom prompt in bash works accidentally rather than by design. The shell code sets PS1 at the end but the assignment has no effect for some reason (you need to replace '\n' by ';' there but I don't quite get why).
It would be nice to also add support for general Bourne /bin/sh as a subshell.