Hi Jonathan,
it's rare for me to receive such detailed and quick feedback on a patch.
Thank you very much!
On Wed, Jun 09, 2021 at 12:40:58AM -0700, Jonathan Nieder wrote:
> 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.
Oh. That's an interesting vision. Personally, I was hoping that dash
could keep being the primary provider of /bin/sh and that we could make
bash non-essential. Non-essential dash seems a little utopical to me.
> We're already closer to that ideal than we used to be, since bash
> doesn't ship /bin/sh nowadays; only dash does.
I don't quite understand how we are closer now that we have two
essential shells instead of just one, but I'll try to follow.
> 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
Difficult. The Debian policy explicitly keeps itself muted on what
happens during installation bootstrap. Such ordering is already frowned
upon and not a very reliable method as we have seen numerous times in
the past. In any case, we don't have any metadata that would allow us to
express such ordering.
> 1b. as custom logic in the bootstrap utility, write the /bin/sh
> symlink
What do you mean with "the bootstrap utility"? debootstrap?
cdebootstrap? multistrap? mmdebstrap? zdebootstrap? People using bare
apt?
I don't think we want to go down this road.
> 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.)
This one sounds sane to me. My primary motivation for working on this
bug happens to be improving the installation bootstrap to cover more use
cases. A quite central document on this work is
https://wiki.debian.org/Teams/Dpkg/Spec/InstallBootstrap. As it happens,
there is a proposal asking quite precisely for what you ask here.
> 1c. include the /bin/sh symlink in a separate "sh-is-dash"
> package
Likely base-files would depend on sh-is-dash | sh-is-bash | sh-is-...
I suppose that this approach would just work. It feels a bit excessive
in terms of small packages.
> Of these, 1b seems simple enough so it's surmountable.
Much to the contrary, 1b is the one that I'd rule out first due to the
existing zoo of bootstrap utilities.
> 2. On upgrade of dash in existing systems, we need to avoid removing
> the /bin/sh symlink:
I'm skipping the upgrade path, because I generally think that it is
solvable if we can form consensus on the goal.
> 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.
That much we agree on. I might put this to a weaker form though. The
default case (sh is dash) should work without diversions. I'm less sure
that non-default must work without diversions and that dash needs to be
non-essential.
You did not manage to fully convince me of the world where dash is no
longer essential. What you write feels to sketchy to me and I don't see
that much benefit, because dash is so small and widely used that I
wouldn't invest any time into that future. Non-essential bash is a
different story. Getting there actually requires fixing the diversion
mess left behind. :)
> 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.
\o/
> nit: this should say "removing" instead of "taking over".
Fixed. Thanks.
> > # 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).
drop_obsolete_diversion should have removed any diversion owned by dash.
For that reason, this code should be unreachable.
> 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.
Swapped and separated.
> Doesn't this make this 'elif' clause unreachable, because 0.4.18 < 0.5.11?
Yes, it does. It didn't do the right thing. Thanks for catching.
> 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.
Updated patch attached.
Helmut
diff --minimal -Nru dash-0.5.11+git20210120+802ebd4/debian/changelog
dash-0.5.11+git20210120+802ebd4/debian/changelog
--- dash-0.5.11+git20210120+802ebd4/debian/changelog 2021-03-04
10:22:32.000000000 +0100
+++ dash-0.5.11+git20210120+802ebd4/debian/changelog 2021-06-03
20:19:40.000000000 +0200
@@ -1,3 +1,10 @@
+dash (0.5.11+git20210120+802ebd4-1.1) UNRELEASED; urgency=medium
+
+ * Non-maintainer upload.
+ * Remove unnecessary diversion in case /bin/sh points to dash. (Closes: #-1)
+
+ -- Helmut Grohne <hel...@subdivi.de> Thu, 03 Jun 2021 20:19:40 +0200
+
dash (0.5.11+git20210120+802ebd4-1) unstable; urgency=medium
* New upstream snapshot.
diff --minimal -Nru dash-0.5.11+git20210120+802ebd4/debian/dash.postinst
dash-0.5.11+git20210120+802ebd4/debian/dash.postinst
--- 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,9 +35,9 @@
return
fi
- if [ -n "$diverter" ] && [ "$diverter" != bash ]; then
- # Let dpkg-divert error out; we are not taking
- # over the diversion, unless we added it
+ if [ "$diverter" != bash ]; then
+ # Let dpkg-divert error out; we are not removing the
+ # diversion, unless we added it
# ourselves on behalf of bash.
dpkg-divert --package dash --no-rename --remove $dfile
echo "This should never be reached"
@@ -45,7 +45,6 @@
fi
dpkg-divert --package bash --no-rename --remove $dfile
- dpkg-divert --package dash --no-rename --divert $distrib --add $dfile
# remove the old equivalent of $distrib, if it existed.
if [ -n "$truename" ]; then
rm -f "$truename"
@@ -58,18 +57,22 @@
diverter=$(dpkg-divert --listpackage $dfile)
truename=$(dpkg-divert --truename $dfile)
- if [ "$diverter" != dash ]; then
+ if [ -n "$diverter" ] && [ "$diverter" != dash ]; then
# good.
return
fi
+ 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
+ fi
+
# Donate the diversion and sh symlink to the bash package.
ltarget=$(echo $ltarget | sed s/dash/bash/)
- dpkg-divert --package dash --no-rename --remove $dfile
dpkg-divert --package bash --no-rename --divert $distrib --add $dfile
- if [ -n "$truename" ]; then
- rm -f "$truename"
- fi
replace_with_link $dfile $ltarget $distrib
}
@@ -85,7 +88,6 @@
if [ "$diverter" = ash ]; then
dpkg-divert --package ash --no-rename --remove $dfile
- dpkg-divert --package dash --no-rename --divert $distrib --add
$dfile
if [ "$truename" != "$distrib" ] && [ -e "$truename" ]; then
mv "$truename" "$distrib"
@@ -104,6 +106,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,8 +140,12 @@
/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.4.18; then
- 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
+ if dpkg --compare-versions "$2" lt 0.4.18; then
+ add_shell
+ fi
fi
if [ $debconf ]; then
