Hi Helmut,
Helmut Grohne wrote:
> At present, /bin/sh is always diverted. The diversion takes one of three
> forms:
>
> A. diverted by package dash, pointing to dash
> B. diverted by package bash (but performed by dash.postinst), pointing
> to bash
> C. local diversion
>
> The local diversion gives freedom to administrators to manually point
> their /bin/sh at bash and seems to be still in use. dash.postinst takes
> care not to mess with such diversions and that should continue to work.
Sure, that makes sense (re local diversions).
> Unless there is a local diversion, dash.postinst sets up either A or B
> depending on a debconf question defaulting to A. Since /bin/sh normally
> points to dash when not diverted, the diversion A seems unnecessary. B
> will have to continue to exist for as long as dash supports changing
> /bin/sh via debconf.
My ideal would be to go even further and not include /bin/sh in any
shell's package (in other words, to treat the symlink something like a
configuration file). That way, one could bootstrap with any choice of
shell instead of dash needing to be essential.
We're already closer to that ideal than we used to be, since bash
doesn't ship /bin/sh nowadays; only dash does.
What would be required to make that work?
1. On initial install (bootstrap), we need to be able to run preinst
for essential packages. Most preinsts are shell scripts with
/bin/sh shebang, so this means that one of the following would
have to happen:
1a. run preinst for dash before any other package; it could use
e.g. a #!/usr/bin/perl script to install the /bin/sh symlink
1b. as custom logic in the bootstrap utility, write the /bin/sh
symlink
1b'. introduce a new maintainer script that is specifically for
bootstrapping. (I would love this; it's probably worth a
separate DEP, but it's overkill for this application alone.)
1c. include the /bin/sh symlink in a separate "sh-is-dash"
package
Of these, 1b seems simple enough so it's surmountable.
2. On upgrade of dash in existing systems, we need to avoid removing
the /bin/sh symlink:
- if another package is diverting /bin/sh, this happens
automatically (good)
- if dash is diverting /bin/sh, preinst can handle this by passing
ownership of the diversion to another package
- after the patch proposed below, there is a third case: if no
package is diverting /bin/sh, dash's preinst can still handle
this by creating a diversion on behalf of another package
So the patch below doesn't lose flexibility in that dimension (good).
3. After an upgrade, we want to end up with a clean state with no
diversions. We could handle that by removing the diversion in
dash's postinst.
Upshot: the proposal below doesn't bring us closer to that ideal, but
it doesn't bring us further away either. And I like the idea of
making the default configuration simpler.
[...]
> --- dash-0.5.11+git20210120+802ebd4/debian/dash.postinst 2021-03-04
> 10:22:32.000000000 +0100
> +++ dash-0.5.11+git20210120+802ebd4/debian/dash.postinst 2021-06-03
> 20:19:40.000000000 +0200
> @@ -25,7 +25,7 @@
> diverter=$(dpkg-divert --listpackage $dfile)
> truename=$(dpkg-divert --truename $dfile)
>
> - if [ "$diverter" = dash ]; then
> + if [ -z "$diverter" ]; then
> # good.
> return
> fi
> @@ -35,7 +35,7 @@
> return
> fi
>
> - if [ -n "$diverter" ] && [ "$diverter" != bash ]; then
> + if [ "$diverter" != bash ]; then
> # Let dpkg-divert error out; we are not taking
> # over the diversion, unless we added it
nit: this should say "removing" instead of "taking over".
> # ourselves on behalf of bash.
> dpkg-divert --package dash --no-rename --remove $dfile
> echo "This should never be reached"
> exit 1
Isn't this reachable now, in the "$diverter" = dash case? Or is the idea that
the drop_obsolete_diversion case is meant to have taken care of that? I think
treating this as reachable would make the code easier to reason about (and then
we can simplify further after this code has been live in one stable release).
> @@ -45,7 +45,6 @@
> fi
>
> dpkg-divert --package bash --no-rename --remove $dfile
> - dpkg-divert --package dash --no-rename --divert $distrib --add $dfile
dash is the only package that ships /bin/sh, so this has no effect on what
happens during unpacking. Good.
[...]
> @@ -58,18 +57,20 @@
> diverter=$(dpkg-divert --listpackage $dfile)
> truename=$(dpkg-divert --truename $dfile)
>
> - if [ "$diverter" != dash ]; then
> + if [ "$diverter" = dash ]; then
> + # Should not happen. Handle anyway.
> + dpkg-divert --package dash --no-rename --remove "$dfile"
> + if [ -n "$truename" ]; then
> + rm -f "$truename"
> + fi
> + elif [ -n "$diverter" ]; then
> # good.
> return
> fi
>
> # Donate the diversion and sh symlink to the bash package.
The previous control flow was
if <already in intended state>; then
# good.
return
fi
get into intended state
The new control flow is
if <case 1>; then
partly handle it
elif <already in intended state>; then
return
fi
handle the rest
I find the new control flow harder to follow: it takes longer to get
the "what do we do if we're already in a good state?" case out of the
way, and it splits up the other cases.
Could this say something like the following instead?
if [ -n "$diverter" ] && [ "$diverter" != dash ]; then
# good.
return
fi
# Donate the diversion and sh symlink to the bash package.
ltarget=$(echo $ltarget | sed s/dash/bash/)
if [ -n "$diverter" ]; then
dpkg-divert --package dash --no-rename --remove $dfile
if [ -n "$truename" ]; then
rm -f "$truename"
fi
fi
dpkg-divert --package bash --no-rename --divert $distrib --add $dfile
replace_with_link $dfile $ltarget $distrib
Or if we want to make this briefer, the first test can be written as
if [ "${diverter:-dash}" != dash ]; then
[...]
> @@ -104,6 +104,20 @@
> fi
> }
>
> +drop_obsolete_diversion() {
> + dfile=$1 ltarget=$2 distrib=${3:-$dfile.distrib}
> + diverter=$(dpkg-divert --listpackage "$dfile")
> + truename=$(dpkg-divert --truename "$dfile")
> + actualtarget=$(readlink "$dfile")
> +
> + if [ "$diverter" != dash ] || [ "$truename" != "$distrib" ] || [
> "$actualtarget" != "$ltarget" ]; then
> + # Not our diversion or a non-trivial one.
> + return
> + fi
> + dpkg-divert --package dash --no-rename --remove "$dfile"
> + rm -f "$truename"
> +}
> +
> add_shell() {
> if ! type add-shell > /dev/null 2>&1; then
> return
> @@ -124,6 +138,10 @@
> /usr/share/man/man1/sh.distrib.1.gz \
> /usr/share/man/man1/ash.1.gz
> add_shell
> +elif [ "$1" = configure ] && dpkg --compare-versions "$2" lt
> 0.5.11+git20210120+802ebd4-1.1~; then
> + drop_obsolete_diversion /bin/sh dash
> + drop_obsolete_diversion /usr/share/man/man1/sh.1.gz dash.1.gz
> /usr/share/man/man1/sh.distrib.1.gz
> + add_shell
> elif [ "$1" = configure ] && dpkg --compare-versions "$2" lt 0.4.18; then
Doesn't this make this 'elif' clause unreachable, because 0.4.18 < 0.5.11?
Summary: I like what this patch aims to do; I think it needs some tweaking to
be easier to read but then it should be good to apply.
Thanks,
Jonathan
