ev3dev / brickstrap

Tool for bootstrapping Debian and creating bootable image files for embedded systems
MIT License
36 stars 26 forks source link

Add work around for empty/missing /etc/shells. #48

Closed cmacq2 closed 8 years ago

cmacq2 commented 8 years ago

The add-shell tool from debianutils package may choke on empty/missing /etc/shells, preventing packages like dash and bash from being configured properly.

As per request, this PR isolates and supersedes the corresponding change from PR #45

dlech commented 8 years ago

Carrying on the conversation from https://github.com/ev3dev/brickstrap/pull/45#discussion_r51057017... I see that the bug has already been fixed in unstable, so it seems like there is no longer anything to work around.

cmacq2 commented 8 years ago

Yes, OTOH this workaround basically compensates for the fact that brickstrap does something different from what normal apt-get install debianutils is supposed to do.

Normal installation of debianutils is supposed to copy the /usr/share/debianutils/shells template to /etc/shells but for some reason this doesn't happen when using brickstrap. (You'll not that this is basically the first thing the workaround tries to do.)

So I'd keep the workaround because there is other 'funny' stuff going on here as well. (It's not like the workaround is overly intricate, plus the code is backwards+forwards compatible: it won't fall over if the shells template is missing/removed from a future debianutils package.)

dlech commented 8 years ago

workaround basically compensates for the fact that brickstrap does something different from what normal apt-get install debianutils is supposed to do.

Actually, debianutils is working as intended. if /etc/shells exists already, it does not copy the template. https://sources.debian.net/src/debianutils/4.7/debian/postinst/

dlech commented 8 years ago

Well, since debianutils is a required package, I guess I'm ok with leaving this in.

cmacq2 commented 8 years ago

Actually, debianutils is working as intended

But not in our context: that was what stumped me before I figured out the work around:

Hence: workaround. Problem vanishes. And even with the update/revert to debianutils 4.7 I'd just leave this in as a safety catch/insurance against this stuff breaking in the future. (Because the knock-on effects of a broken dash or bash are no good.)

dlech commented 8 years ago

I wonder if a preinst script is touching /etc/shells and that is what is causing the problem. It's probably better to move this before the loop that runs the preinst scripts.

cmacq2 commented 8 years ago

How would that improve anything? Either the preinst scripts generate a non-empty /etc/shells in which case, fine (presumably) and the workaround is not triggered; or the scripts remove it/empty it in which case the workaround must be executed after the scripts run.

Either way after preinst seems the correct moment to invoke it.

dlech commented 8 years ago

Either the preinst scripts generate a non-empty /etc/shells in which case, fine (presumably) and the workaround is not triggered;

If a preinst script generates a non-empty file, then /bin/sh will be missing from /etc/shells because that comes from the template.

cmacq2 commented 8 years ago

If a preinst script generates a non-empty file, then /bin/sh will be missing from /etc/shells because that comes from the template.

As I understand it: /bin/sh will be added during dpkg --configure -a when dash is configured and the dash package invokes the add-shell tool to register itself in /etc/shells.

On normal Debian installations that step should do nothing (because /bin/sh is already listed in the template as you noted), but if /bin/sh isn't listed in /etc/shells during the dpkg --configure -a then add-shell should add it.

cmacq2 commented 8 years ago

By the way I've rebased the PR against master and fixed up the info/warning message thing.

cmacq2 commented 8 years ago

Is this good to go or does it require further discussion/changes?

dlech commented 8 years ago

My gut feeling tells me that just copying /usr/share/debianutils/shells to /etc/shells before running the preinst scripts is the "right thing" to do but I am tired of arguing about it. The PR is good enough as-is and I will merge when I have a chance to run through an image build again.