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

Should we remove SHOPT_BASH and the code predicated on it? #238

Open
krader1961 opened this issue Dec 18, 2017 · 31 comments
Open

Should we remove SHOPT_BASH and the code predicated on it? #238

krader1961 opened this issue Dec 18, 2017 · 31 comments
Assignees

Comments

@krader1961
Copy link
Contributor

krader1961 commented Dec 18, 2017

I would love to know who thought that implementing a Bash compatibility feature was a good idea. It's fine to adopt good ideas from other shells. It's not fine to try and make ksh 100% bash compatible. If someone needs 100% bash compatibility they should just use bash. Note that the documentation implies that this adds a shopt bash compatibility command. But that's only true if you invoke it with the external command name bash.

This feature isn't in the ksh93u+ from 2012 that is the most commonly available version. It looks like it was added in 2014. So I'd like to drop it. Individual bash features can be added where a good case can be made for doing so but they should always be available, not just if the shell was invoked as bash.

P.S., This topic is a FAQ for non-POSIX shells like Fish and Elvish. And the answer is the same. Use bash (or zsh, or whatever) if that's what you need.

@krader1961 krader1961 added the RFC label Dec 18, 2017
@krader1961 krader1961 changed the title Should we remove SHOPT_BASH and the code based on it? Should we remove SHOPT_BASH and the code predicated on it? Dec 18, 2017
@kdudka
Copy link
Contributor

kdudka commented Dec 18, 2017

If someone spent time on implementing the option and getting it upstream in 2014 (when upstream was nearly inactive), it must have been for a very good reason. Please do not remove it unless you know the reasoning behind.

In general, compatibility options are useful. Especially Zsh does very good job in maintaining compatibility with other shells. I know the compatibility is not 100% and it will likely never be. Still having these options available makes porting legacy scripts at least possible without rewriting them from scratch.

@siteshwar
Copy link
Contributor

SHOPT_BASH option was present in ksh93u+. You can see it in the code here.

@philippe56
Copy link

I always unset SHOPT_BASH before compiling ksh, and I vote for the complete removal of this code. (BTW, I also unset SHOPT_EDPREDICT, that does scale and turns out to be annoyingly slow on huge history files).

@krader1961
Copy link
Contributor Author

SHOPT_BASH option was present in ksh93u+.

Okay, but it doesn't seem to be operational. Copy that ksh binary to a file named bash and run it. You should be able to execute the shopt command. But none of the ksh93u implementations I have access to behave that way. Also, set -o should list a bash off option. If you build ksh from the master branch those things work.

In general, compatibility options are useful.

Except that isn't what this feature does. Assuming by "compatibility options" you mean features, perhaps disabled by default, that make it easier to port a bash script to ksh. I have no objection to such features assuming they otherwise mesh well with the existing ksh features. What this option appears to attempt to do is make ksh behave like bash when the ksh binary is invoked as bash. That is, be a faithful implementation of bash behavior. And that is just plain nuts. If I want bash behavior I'll use the bash command. This feature is like saying you prefer Python to Perl but because you have a bunch of Perl scripts you don't want to have to rewrite there should be a Perl emulation mode added to Python.

I don't doubt the person who implemented this feature felt fully justified in doing so. But even good software engineers make bad decisions. In this case I'd bet the person who implemented this did not solicit feedback on the idea before they wrote the code.

@krader1961
Copy link
Contributor Author

Also, if you search the ast-developers list archive, https://marc.info/?l=ast-developers&w=2&r=1&s=shopt_bash&q=b, there is exactly one message by David Morano in 2015 that mentions this option. And that message is a FYI that building ksh fails if the feature is enabled.

@siteshwar
Copy link
Contributor

siteshwar commented Dec 19, 2017

Some of the code under SHOPT_BASH is operational even without enabling bash emulation mode. For e.g. test for local keyword. SHOPT_BASH macro has been enabled by default for long time, so it will likely cause regressions when we remove code related to it.

I agree that adding bash emulation mode in ksh was a bad decision, however given the current state of code, we will have to be exteremely careful while removing any code related to it.

EDIT: SHOPT_BASH was enabled by default only since the last beta release, so my comment about it being "enabled by default for long time" was incorrect.

@krader1961
Copy link
Contributor Author

Some of the code under SHOPT_BASH is operational even without enabling bash emulation mode.

That makes me a sad puppy. Adding a local keyword as an alternative to using typeset inside a function is a reasonable thing to do. But that should not have been tied to the bash emulation mode.

I'll see if the bash emulation mode can safely be disabled and leave just the other aspects such as the local keyword. If not I'll open an issue to remind us to remove the bash emulation mode.

@dannyweldon
Copy link

dannyweldon commented Dec 19, 2017

I think there needs to be more attention given to what @kdudka as a representative of Redhat says. If you change ksh too much, you run the very real risk of it never being accepted back into the mainstream distros, mainly because they have paying customers to support, and they will definitely come first. Meaning that they may be forced to stick with the old code and keep backporting fixes. This would not be good for ksh acceptance.

I was delighted when I discovered the bash emulation options for ksh and thought it was a stroke of genius. If the ubiquitous bash can't catch up to ksh technically with it's type system, then make ksh try to emulate bash while bringing along the korn shell's benefits. Sure, it would be better to make local a standard builtin and the bash emulation a compilation option. Probably all new experimental features should be made to be a compile time option, or at least maybe that's how it was done before, whereas maybe it's better now to have them in separate branches now that the source is in git; I'm not sure.

Don't think that ksh could not again rise to the top of the shell ecosystem. There have been some surprising changes to the Linux world recently, with nothing being sacred any more:

  • systemd replacing sysv init
  • xfs the default filesystem on redhat instead of ext4
  • debian using dash for init scripts (this would have been a good case to use ksh instead)
  • the rise in the use of clang/LLVM instead of gcc
  • alternative implementations of php and ruby (similarly ksh could be a serious alternative implementation of bash if someone spent more time on it)

Anyway, just removing unfinished experimental features adds nothing except an appearance of productivity and takes away valuable work that someone has spent time contributing. That doesn't mean that all contributions should be accepted or ksh could end up like zsh, but this is code that was accepted by the author @dgkorn, if that means anything. This also applies to #234.

@krader1961
Copy link
Contributor Author

krader1961 commented Dec 20, 2017

@dannyweldon,

If my peers at RedHat, SuSE, or any other distro deem certain changes too radical, by which I mean they depart too far from existing ksh93u+ behavior, I am happy to drop those proposed changes. I do not think dropping bash emulation is in that category. Dropping support for the local keyword on the other hand is definitely in that category. That removing the SHOPT_BASH symbol and the code predicated on it might remove code we want to retain while simultaneously removes code we want to jettison simply reflects how messy the current code base is and why caution needs to be exercised in making such changes.

As I said earlier the bash emulation is equivalent to asking that the Python language have a means of emulating the Perl language.

Your reference to issue #234 is interesting. That feature implementation is clearly incomplete. Despite many years passing since the original changes were merged. And the feature is not in any of the ksh93 implementations any one is using AFAICT unless they are building from this project's source.

There is a lot of bit rot in this project. I've already found several instances where the code fails to compile if various #define symbols involving "experimental" features are changed. And even if the code does compile with those build time symbol change I'll bet $100 that there are platforms we care about where it fails to run correctly when those symbols are changed.

If the community feels the bash emulation should be retained and improved I will stop contributing. Bash is an active project. It makes zero sense to commit the ksh team to tracking that moving target. It will be hard enough to fix all the extant issues with ksh specific features. Consider the Fish and Elvish shell projects. Both have made it an explicit anti-goal to maintain POSIX 1003 compatibility. Obviously ksh can't do that given its origins. But I see no reason this project should commit to running bash or zsh scripts with full fidelity.

@siteshwar
Copy link
Contributor

SHOPT_BASH option is not enabled on RHEL or Fedora. It is not enabled on openSUSE either. It was turned on by default in the last beta release.

Related log from RELEASE file :

14-06-02 +When compiled with the SHOPT_BASH and run with the name bash,
      the shell now uses dynamic scoping for name() and function name.
      In addition the builtins declare and local are supported.
      The SHOPT_BASH option is on by default in the Makefile.
      More work remains with the bash compatibility option.

The code around SHOPT_BASH was work under progress when it was enabled by default. This code is likely to be untested and will be a source of many issues if we start using it. Bash is a complex beast and it will require significant amount of work to support bash compatibility. Current codebase has large number of issues and supporting bash compatibility makes it worse. None of the active contributors is willing to spend effort to continue to support such option.

I can see if we continue to support bash compatibility, there will be a use case for it, but the cost of maintainability outweighs it's benefits. It would be best to deprecate this option in next stable release.

@KeithBierman
Copy link

KeithBierman commented Dec 20, 2017 via email

@kdudka
Copy link
Contributor

kdudka commented Dec 20, 2017

@siteshwar Thank you for doing the research on this! Given the facts, I agree that removing the feature (or at least disabling it by default) is a reasonable thing to do before the next release of ksh.

@krader1961
Copy link
Contributor Author

krader1961 commented Dec 21, 2017

Thanks for commenting, @KeithBierman. This project definitely needs more involvement from people in the open source community like yourself. But I disagree with your assessment that this is "more similar to C vs. C++." There is no question that both ksh, bash, and zsh derive from the original Bourne shell (the "C" language in your analogy). But there is no equivalent to the C++ language standard that binds them. This is very much like asking that Python have a mode that allows it to run Perl programs.

"Bug for bug compatible" is one of the phrases I heard many years ago when I worked for Sequent Computer Systems. That was because even though a particular behavior is objectively wrong, whether or not measured against a published standard versus a specific implementation, the reality is that you sometimes need to exhibit the same "bug". As an open source project I don't see any reason we should try to be "bug for bug" compatible with bash.

Issue #220 notes that the local keyword doesn't work if using the my_func() {...} syntax but does work when using the function my_funct {...} syntax is 99.99% likely to be related to this half-baked change.

@dannyweldon
Copy link

Okay, I would have liked to have had it kept, but I will concede that it is probably best to remove it. The code will still be there in the history if someone really wants to resurrect it to complete it. But this begs the question on how new features are going to be accepted, especially as like SHOPT_FIXEDARRAY, they often involve multiple touch points over multiple files. I am assuming that the preference would be for working PRs that merge cleanly without the conditional compilation?

And Kurt, I think bash and ksh are different shell dialects, whereas perl and python are completely different languages, but lets not quibble. :-)

@krader1961
Copy link
Contributor Author

Well, it's more like ksh is C++, bash is Objective-C and zsh is C#. They all share antecedents and thus have a lot in common but they are distinct languages. Imperfect analogies aside the core issue here is that we should not commit to a bug-for-bug compatible bash or zsh emulation mode. As I said earlier I have no objection to borrowing good ideas from other shells (or languages like Python) as long as the primary motivation is to improve ksh as opposed to making it able to run scripts written for those other shells.

I have no objection to conditional compilation while a complex feature is being implemented. Although I think every effort should be made to implement a feature in self-contained commits that don't break existing behavior so that conditional compilation is not needed. And those #if statements should be removed at the earliest opportunity. We don't want different ksh dialects. If a feature is enabled on platform X it should be enabled on platform Y.

Obviously there may be exceptions to the above guideline. One that comes to mind is being able to build ksh as an alternative to busybox.

@krader1961
Copy link
Contributor Author

Anyway, just removing unfinished experimental features adds nothing except an appearance of productivity...

It makes it easier to focus on non-experimental features. If a feature is still experimental 5+ years since the last stable release it should be jettisoned.

How many of the current test failures, or general test flakiness, is because of experimental features? I have no idea and I'll bet no one else does. But removing code we aren't committed to maintaining definitely makes it easier to understand and improve the remaining code and tests. Pruning dead wood is as important to software as it is to horticulture.

@krader1961 krader1961 self-assigned this Dec 23, 2017
@kshji
Copy link

kshji commented Dec 30, 2017

I like to say: ksh is ksh, bash is bash, ...
All those are children of Born Shell. We have also Posix specs. Dash is full posix and only posix, if I have understood correctly. Debian like to use only posix compatible properties in critical scripts. Not bad idea. I used long time only "bourne shell" syntax in ksh/bash ... It was safe when I done porting to many different *nix and customers. Never know which version of sh was possible to use.
Ksh have to include bourne shell + posix + ksh88 + ksh93 "2012" version. Example all regexp and json extensions have been nice. Don't remove those :). That's the problem: all those who has used some extensions, don't know that those extensions are something new, even experimental and maybe not anymore in next version ...

If I sometimes like to use bash some special feature, then I use bash, not bash by ksh. I like more ideas that if some of those (ksh, bash, zsh,...) add some good idea (real usable extension) which is needed, not easy to done using current syntax, then it should be new builtin extension.
=> those which are included in ksh should be there, those which only works using bash, should be removed. It's too heavy, expensive, ... to try to be everything. No need.

Thank you and continue your excellent updating process even I don't like every moves, too old and lazy to learn new .... I really hope that younger find root of sh's. => building env use today/tomorrow envs.

@krader1961
Copy link
Contributor Author

Thank you for commenting @kshji and I hope you continue to comment. You reinforced my observation that if someone needs a bash feature they explicitly invoke that program to run their script. Why would someone expecting bash behavior for an obscure bash feature expect ksh to implement the same behavior?

@krader1961
Copy link
Contributor Author

For your enjoyment from the src/cmd/ksh93/bltins/typeset.c module:

#if SHOPT_BSH
                if (flag & NV_EXPORT) nv_offattr(np, NV_IMPORT);
#endif /* SHOPT_BSH */

Notice the typo: SHOPT_BSH rather than SHOPT_BASH.

@krader1961
Copy link
Contributor Author

@kdudka, In light of our discussion in issue #240 are you still okay with removing this feature? So far I've only taken one baby step to do so: PR #372. If you believe that making ksh act like bash when invoked by that name is something we want (or need) to support say so. Personally I think that any end user that is using this "feature" (scare quotes are intentional) is crazy. As has been discussed ad nauseam it is impossible to make ksh faithful to every bash version, past and future. I have no objection to adopting useful features from bash, zsh, fish, python, or any other language. But trying to make ksh behave with limited, let alone full, fidelity to any of those implementations is absurd IMHO.

@DavidMorano
Copy link

DavidMorano commented Jan 19, 2018

@krader1961

One obvious reason to retain the ability of KSH to act like BASH when called as BASH is to reduce the memory requirement for small embedded spaces. One binary will be in system memory serving as both the KSH program and the BASH program simultaneously (not to mention the saving of space on disk, although of lesser importance). There have been some small embedded UNIXi where there has only been some relatively small numbers of system-memory megabytes available for everything. Yes, some people want to put UNIX/Linux in incredibly small memory spaces. For myself, I would never want to live on such a technology edge (such small system-memory spaces) myself -- if I could help it. But some projects out there do want to live in such small spaces. Just something to consider. My feeling is that small embedded developers have not yet seen these present efforts to modernize the KSH code, and so are not around to comment.

It is a little fuzzy now, but there was a time in the past, between maybe ten years ago to fifteen years ago, when the AST developers at the time (David Korn and Glenn Fowler, maybe others) tried to react (handle) the need to make KSH as small as possible, to make it more suited to very small embedded UNIXi spaces. It is possible that there still remains some projects out there that want to put KSH in very small memory spaces. Incidentally, the KSH built-ins that substitute for regular UNIXi utilities are also a very good way to reduce over-all memory requirements. A typical built-in mimicking a regular system utility takes up a very good deal less memory requirement than a full fledged additional process. This is beside the point that the KSH built-ins are so much faster when called within loops in shell code (and some such usages). In general, all KSH built-ins are much (much) faster than spawning regular processes when the built-ins are within any kind of loop in a shell-language program.

Also, many projects use BASH and KSH exactly for the reason that these two shells are (or have been) the preeminent POSIX compliant shells. So having KSH mimic BASH is reasonable (since they share a lot in the way of POSIX compatibility), but having KSH mimic other languages or shells indeed does not make much sense (IMHO), but neither would that ever be likely needed.

@krader1961
Copy link
Contributor Author

One obvious reason to retain the ability of KSH to act like BASH when called as BASH is to reduce the memory requirement for small embedded spaces.

That doesn't make any sense to me. If you're that tight on memory and/or persistent storage you're not going to want to support multiple shells. You'll pick one and write all your scripts in its dialect. Especially if you have to deal with the fact your ksh binary that is pretending to be bash won't be 100% compatible with the real bash. So now you have to vet all your bash scripts to ensure they behave correctly when executed by ksh's bash emulation.

@DavidMorano
Copy link

That doesn't make any sense to me. ...

I agree, but legacy code (like perhaps having old code in both shells) does not usually make sense in the first place. Developers usually do not want to rewrite code that already is working, hence multiple similar languages.

But even outside of tight memory environments, the point remains: one binary in memory can support two
shell dialects. This may still make some sense to some developers or projects -- in a weirdo software mentality sort of way.

I think the real a question on this matter revolves around how good KSH was able to emulate BASH. If the BASH emulation was poor, then the feature is not going to be used. But if the emulation was fairly good, then leaving it in provides this possible modest memory saving device for projects or environments using both shell languages.

@kdudka
Copy link
Contributor

kdudka commented Jan 20, 2018 via email

@krader1961
Copy link
Contributor Author

I haven't done anything about removing the bash emulation because of this comment in src/cmd/ksh93/RELEASE:

14-06-02 +When compiled with the SHOPT_BASH and run with the name bash, the shell now uses dynamic scoping for name() and function name. In addition the builtins declare and local are supported. The SHOPT_BASH option is on by default in the Makefile. More work remains with the bash compatibility option.

This is another significant user noticeable, backward incompatible, change from ksh93u to ksh93v.

Those features (e.g., adding the local keyword) are worth keeping and removing them is too likely to be noticed by anyone using ksh93v rather than ksh93u (which is what is on all my systems). And yet we definitely do not want to support trying to be 100% bash compatible when the ksh binary is invoked as bash. We need to disentangle those two aspects of that change.

@siteshwar
Copy link
Contributor

I would prefer to keep all the code under SHOPT_BASH disabled by default in the next release.

@krader1961 We should defer adding features you mentioned in last comment. I am not ruling out the possiblity to add them in future (I see there is already a request to support local keyword in POSIX shells), but let's avoid adding them in next stable release.

@siteshwar siteshwar assigned siteshwar and unassigned krader1961 Jun 20, 2018
siteshwar added a commit that referenced this issue Jun 25, 2018
siteshwar added a commit that referenced this issue Jun 25, 2018
SHOPT_BASH macro was disabled by default in ksh93u release, so keep it
disabled.

Related: #238
siteshwar added a commit that referenced this issue Jun 25, 2018
Otherwise empty elements are created in shtab_options variable which
was causing crashes.

Related: #238
@krader1961
Copy link
Contributor Author

I propose removing the SHOPT_BASH symbol and all code predicated on it. We've been building ksh without that code for nearly a year. No one has complained. My earlier comments expressing concern about disabling that flag changing behavior appears to be unfounded. Not just empirically but because the nature of the feature means it only affects the program when invoked under the name bash. Which we do not test via any unit tests and it is unlikely (I hope) that anyone is actually using ksh as a bash clone.

As discussed earlier bash emulation is something we should not, and can not, support. Even if we had lots of people contributing changes it is a fool's errand to try and make ksh behave with full fidelity like bash.

@krader1961
Copy link
Contributor Author

@siteshwar I now believe my previous comment reached the wrong conclusion. We have committed that code guarded by SHOPT_BASH will be disabled in the next major release (2020.0.0). Mostly because the incompatible changes in behavior if that preprocessor symbol is defined are too risky. We have introduced support for a subset of the bash features such as a local command. It's not clear that there are any other such features we will want to introduce independent of emulating bash is the ksh binary is named bash. So I would like to propose we simplify the project by removing the SHOPT_BASH preprocessor symbol and the code predicated on it. This came to my attention while working on issue #507.

@krader1961
Copy link
Contributor Author

FWIW, There is yet another problem with the attempt to emulate bash when the ksh binary is invoked as bash. In the real bash program doing set --noglob is an error:

$ /bin/bash
bash-3.2$ set --noglob
bash: set: --: invalid option
set: usage: set [--abefhkmnptuvxBCHP] [-o option] [arg ...]

But when using ksh as a bash alternative that is silently treated as set -o noglob. I actually like the ksh behavior. But it is incompatible with bash. Providing yet another illustration for why trying to be a faithful emulation of bash behavior when invoked as bash is a fools errand.

@dannyweldon
Copy link

Running set -o noglob works in bash 4.3.48:

$ echo $BASH_VERSION
4.3.48(1)-release
$ set -o noglob
$ set -o | grep noglob
noglob         	on

@krader1961
Copy link
Contributor Author

krader1961 commented Oct 19, 2019

@dannyweldon The issue is that ksh, when running under the name bash, makes set --noglob synonymous with set -o noglob. The real bash does not allow that. My point is that the bash emulation by ksh is not faithful to the real bash behavior.

$ bash
bash-5.0$ set --noglob
bash: set: --: invalid option
set: usage: set [-abefhkmnptuvxBCHP] [-o option-name] [--] [arg ...]
bash-5.0$ exit
$ ln -s /usr/local/bin/ksh93v ~/bash
$ ./bash
bash$ set --noglob
bash $

My point is that the attempt to faithfully emulate bash behavior, even on something this basic, is broken. It's even worse than I thought. Here is the real bash behavior:

$ bash
bash-5.0$ set -o | grep glob
noglob         	off

Here is ksh93v pretending to be bash:

$ ./bash
bash$ set -o | grep glob
glob                     on
globstar                 off

Trying to emulate bash is hopeless. Or at least so expensive that the cost cannot be justified relative to the value.

citrus-it pushed a commit to citrus-it/ast that referenced this issue Apr 15, 2021
This allows ksh to be compiled with versions of tcc that define
__dso_handle in libtcc1.a, i.e., versions as of this commit:
https://repo.or.cz/tinycc.git/commit/dd60b20c

Older versions of tcc still fail to compile ksh, although now they
fail after reaching the libdll feature test. I'm not sure if fixing
that is feasible since even if I hack out the failing libdll
feature test, ksh fails to link with a '__dso_handle' error.

src/lib/libast/comp/atexit.c,
src/lib/libast/features/lib,
src/lib/libast/vmalloc/vmexit.c:
- From what I've been able to gather the only OSes with support
  for on_exit are Linux and SunOS 4. However, on_exit takes two
  arguments, so the macro that defines it as taking one argument
  is incorrect. Since Solaris (SunOS 5) no longer has this call
  and the macro breaks on Linux, the clean fix is to remove it
  (atexit(3) is used instead).

src/lib/libast/include/ast.h:
- When compiling with tcc on FreeBSD, pretend to be gcc 2.95.3
  instead of gcc 9.3.0. This stops /usr/include/math.h from
  activating gcc 3.0+ math compiler builtins that don't exist on
  tcc, while still identifying as gcc which is needed to avoid
  other FreeBSD system header breakage.

src/cmd/builtin/Mamfile,
src/cmd/builtin/features/pty,
src/lib/libdll/Mamfile,
src/lib/libdll/features/dll:
- tcc forbids combining the -c compiler flag with -l* linker flags.
  Use the -lm flag in the iffe feature tests instead of the
  Mamfiles. This avoids iffe combining -lm with the -c flag.

src/lib/libast/vmalloc/malloc.c:
- Fix failure to compile with -D_std_malloc.
  This patch is from OpenSUSE:
  https://build.opensuse.org/package/view_file/shells/ksh/ksh93-malloc-hook.dif
  As it turns out tcc needs this change to build ksh with
  -D_std_malloc, so it has been applied.

Co-authored-by: Martijn Dekker <martijn@inlv.org>
Resolves: ksh93#232
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants