jcnelson / vdev

A device-file manager for *nix
GNU General Public License v3.0
101 stars 13 forks source link

Pleas use POSIX shell script instead of dash #42

Open tokiclover opened 9 years ago

tokiclover commented 9 years ago

Please consider removing dash hardcoding usage for plain POSIX shell script instead. See 1 reference for a wealth of POSIX sh usage. Normally replacing dash with sh should work out the box if not using dash-ism which I did for sys-fs/vdev Gentoo (derivative) distro package.

This issue is really a serious one and should get a bit of attention... because you cannot expect other distros to be using dash as Debian derivative ones do.

Thanks.

fbt commented 9 years ago

Is a dash dependency that much of a problem?

tokiclover commented 9 years ago

Is a dash dependency that much of a problem?

Of course it is! Only Debian derivative distro have /bin/sh symlinked to /bin/dash. Other distros uses other shells... who know whatever might be used for that. This is where using a standard is usefull when targetting numerous distros out there whithout needing a specific shell when a default shell is already available. Hence using POSIX shell scripts. And this how it's done in numerous projects by default.

fbt commented 9 years ago

Huh? This has nothing to do with the #!/bin/sh link. vdev has dash in the shebangs. There is no risk of using the wrong shell here.

P.S. I'm not saying I'm against using POSIX shell in a piece of software that's meant to be distro-agnostic, but #!/bin/sh not being dash in a non-problem.

jonlap commented 9 years ago

Well, scripts in general and especially in cases like this really should be POSIX compliant, if possible. Assumption that everyone would be willing to "install all the shells!" on their systems is also a little bit strange in my opinion.

tokiclover commented 9 years ago

Especially to execute a couple of commands... there is no excuse to require dash for this; and then, requiring dash at all in this case is practically (dare I say unforgivable?) mistake.

I did took a look on various scripts... really nothing stand out but a couple of easy things are done each time. So, using sed -e 's/dash/sh/g' -i */*/*.sh */*.sh would be enough. However, some scripts have not the .sh suffix which is a pain in the ass to look for them... Stopping here.

tokiclover commented 9 years ago

@fbt: Ignone the symlink part if you like or if you think it's out side the scope. -- That's only a side track of the main issue.

jcnelson commented 9 years ago

@tokiclover @skry

To the best of my knowledge, the only non-POSIX feature I make use of in the helper scripts is the local directive for having function-local variables. However, I think you will find that there are multiple shells on each UNIX-like OS that implement it, so my reliance on it should not be of immediate concern regarding portability. I would not be surprised if a future iteration of POSIX included it in the standard, since it is so wide-spread.

My reason for using dash is not because I need dash-specific features, but instead because I need to test with a minimal shell that is readily available and as POSIX-compliant as possible, modulo the availability of the local keyword. Hard-coding /bin/dash in the shebang is done as a testing measure, in order to make it easier for me to reproduce bugs that others discover. But as @tokiclover discovered, it's meant to be easy to swap /bin/dash out for whatever shell the user wants--my use of dash should not be construed as an intentional coupling to Debian (any reliance on dash-specific features should be considered a bug).

@tokiclover The only script that does not end in .sh is the daemonlet script (vdevd/helpers/LINUX/daemonlet). This is because the daemonlet program is considered to be part of vdevd, not a helper script (it's meant to supervise and communicate with a long-running subprocess on vdevd's behalf, such as a stateful helper script, or a helper script that will frequently receive many device requests). The fact that it is implemented as a shell script today is an implementation detail that is likely to change in the future. It is unfortunate that it is located in the helpers directory, since placing it there seems to have caused confusion. I will move it.

fbt commented 9 years ago

On the topic of local: I've just tested in in some of the more conservative shells, and it seems to be implemented everywhere. Even posh supports it.

tokiclover commented 9 years ago

There should not be an issue with the local builtin usage that I know at the moment. I am not talking about scrtict conformance to POSIX, such non confromance usage is understandable. I do use broke POSIX conformance with that local builtin as well.

It's just that the script should be modified to use plain /bin/sh shebang and leave the implementation to the system or user. I am not familiar with Debian derivative distros, however, you can pretty much test whatever you like/want by symlinking /bin/sh to /bin/dash either by hand or by using Debian utility that manage that symlink if any.

Please fix this ASAP to permit wide testing for no Debian derivative distros. You cannot expect every one to have many shells for no reasons at all. More complicated projects like autotools et al use plain POSIX shell conformance. You cannot avoid this issue at all. And it's best to fix it sooner rather than later.

Thanks.

fbt commented 9 years ago

As a packager, I don't really get the ASAP nature of the problem. You can just depend your package on dash till vdev is no longer in heavy testing and doesn't need this, no? I do.

jcnelson commented 9 years ago

@tokiclover You can also symlink /bin/dash to your favorite shell, if you don't want to install dash.

EDIT: Symlink your favorite shell to /bin/dash, provided that you mention as much in any subsequent bug reports (thanks @fbt!).

fbt commented 9 years ago

That might not be a good idea.

To elaborate: bash, as an example, has an sh mode that is enabled if it's called as sh. Not as dash. So there might be inconsistencies that would produce unwanted behaviour in vdev's handlers.

jcnelson commented 9 years ago

@fbt Thanks for the listing! Point 31 is particularly concerning--if I'm reading it right, it basically means that shell functions won't be able to set globally-scoped variables. That would severely restrict the programming model I'm aiming to provide with the daemonlet program (i.e. statefulness in device helpers).

I mis-typed earlier (fixed above)--I meant to suggest to @tokiclover that one alternative to replacing /bin/dash in each helper would be to choose a preferred POSIX-y shell (e.g. posh, pdksh) and symlink that to /bin/dash, so the scripts would not need to be modified.

tokiclover commented 9 years ago

Come on @jcnelson... I am not talking about only my own preferences nor my own conveniences; I am just pointing the way it's done in really ubiquitous projects out there to facilitate testing and integration without requiring absolutely useless dependencies. -- There is absolutely no reasons to require dash at all. So remove it because this is way it's done in this kind of project aimed to wide usage. Really, no need to turn around looking for unnecessary reasons.

jcnelson commented 9 years ago

@tokiclover As I have explained, there is no dependency on dash because the scripts do not rely on any dash-specific features. Any dash-isms in the code are considered bugs. As you yourself have pointed out, you may assume that it will be safe for downstream packagers to replace all instances of #!/bin/dash with a shell that (1) meets POSIX/SUSv3 requirements and (2) honors the local keyword. For example, you should be able to use posh or pdksh(*) in place of dash.

However, my tree will continue to use #!/bin/dash until I consider vdev to be stable enough for everyday use. As @fbt points out, ensuring that both I and early testers use the same shell makes debugging easier, since our using the same shell implementation helps rule out the chance that hard-to-reproduce bugs are due to subtle differences between my /bin/sh and your /bin/sh.

You are of course free to make whatever downstream changes you deem necessary. Again, any dash-isms you encounter are considered bugs, so feel free to report them here and I will remove them. Again, you should have no problem picking an alternative shell that meets the above two requirements (I have successfully used vdev with bash in non-POSIX mode, for example). Once we have a stable release, we can revisit the issue of supporting multiple shells via the traditional #!/bin/sh symlink approach.

Now, is there anything else I can help you with?

EDIT: (*) With pdksh in particular, it will be necessary to alias local to typeset first.

jaromil commented 9 years ago

Note: ZSh is a very portable and awesome shell. It runs well the vdev scripts in place of dash.