Bug #14818

Improve how we deal with dependencies in build hooks

Added by anonym 2017-10-09 12:21:15 . Updated 2018-03-14 11:08:54 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Build system
Target version:
Tails_3.6
Start date:
2017-10-09
Due date:
% Done:

100%

Feature Branch:
feature/improved-dependency-mechanism-for-build-hooks
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:

Description

Subtasks

History

#1 Updated by intrigeri 2017-10-10 05:27:18

  • Target version changed from Tails_3.5 to Tails_3.6

(This is based on the devel branch so can’t make it into 3.4.)

#2 Updated by intrigeri 2017-10-10 05:51:41

  • Status changed from Confirmed to In Progress
  • Assignee changed from intrigeri to anonym
  • QA Check changed from Ready for QA to Dev Needed

Let’s wait for a build on Jenkins to succeed so one can compare the resulting packages lists: the only build I see there was broken just like the build of the devel branch. I’ll check that next time this lands on my plate for review.

Wrt. the “…/authentication/sphere/S.gpg-agent can be removed (by gpg-agent?)” comment, the other day I was suspecting logind rather than gpg-agent, but on Debian KillUserProcesses=no is the default, so it’s not that; but on Buster (and Stretch?) gpg-agent is run as a systemd user service so it might be stopped when a session is terminated. Anyways. When /var/lib/monkeysphere/authentication/ does not exist I think we should abort the build: that’s what we do generally to ensure that we don’t keep such things around when they become obsolete, e.g. config/chroot_local-hooks/45-disable-unneeded-dbus-services.

Regarding build_hook_dependency:

  • I dislike the name (surprise!) for two reasons:
    • it does not tell what the function does (there’s no verb that expresses this)
    • “build” can be easily interpreted as the verb here, i.e. “please build this hook’s dependency”, which does not make sense
    • how about ensure_hook_dependency_is_installed?
  • note that only the simplest possible syntax of apt-get install is supported: e.g. install package=$version or package/$codename won’t work; I don’t think this is a problem at this point, let’s not bother fixing it until we need it, but perhaps s/Ensure that the packages passed as arguments/Ensure that the packages whose names are passed as arguments/?

Other than that, code review passes :)

#3 Updated by anonym 2018-02-01 16:17:46

  • % Done changed from 50 to 60
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> When /var/lib/monkeysphere/authentication/ does not exist I think we should abort the build: that’s what we do generally to ensure that we don’t keep such things around when they become obsolete, e.g. config/chroot_local-hooks/45-disable-unneeded-dbus-services.

Let’s try it. It would even be another data point indicating that the theory about the directory being removed is correct.

> Regarding build_hook_dependency:
>
> * I dislike the name (surprise!) for two reasons:
> it does not tell what the function does (there’s no verb that expresses this)
> “build” can be easily interpreted as the verb here, i.e. “please build this hook’s dependency”, which does not make sense
> how about ensure_hook_dependency_is_installed?

Done.

> * note that only the simplest possible syntax of apt-get install is supported: e.g. install package=$version or package/$codename won’t work; I don’t think this is a problem at this point, let’s not bother fixing it until we need it, but perhaps s/Ensure that the packages passed as arguments/Ensure that the packages whose names are passed as arguments/?

Done. I also added a sanity check to make this explicitly unsupported (although I think apt-mark would fail => build fail).

Let’s see what Jenkins thinks…

#4 Updated by anonym 2018-02-02 14:08:58

  • Assignee changed from anonym to intrigeri

anonym wrote:
> intrigeri wrote:
> > When /var/lib/monkeysphere/authentication/ does not exist I think we should abort the build: that’s what we do generally to ensure that we don’t keep such things around when they become obsolete, e.g. config/chroot_local-hooks/45-disable-unneeded-dbus-services.
>
> Let’s try it. It would even be another data point indicating that the theory about the directory being removed is correct.
>
> > Regarding build_hook_dependency:
> >
> > * I dislike the name (surprise!) for two reasons:
> > it does not tell what the function does (there’s no verb that expresses this)
> > “build” can be easily interpreted as the verb here, i.e. “please build this hook’s dependency”, which does not make sense
> > how about ensure_hook_dependency_is_installed?
>
> Done.
>
> > * note that only the simplest possible syntax of apt-get install is supported: e.g. install package=$version or package/$codename won’t work; I don’t think this is a problem at this point, let’s not bother fixing it until we need it, but perhaps s/Ensure that the packages passed as arguments/Ensure that the packages whose names are passed as arguments/?
>
> Done. I also added a sanity check to make this explicitly unsupported (although I think apt-mark would fail => build fail).
>
> Let’s see what Jenkins thinks…

I did a bad merge conflict resolution, so I did another push (commit:62fc5b89bc8876b1c41c795bf2a16c8c69516290), but now it looks good:

#5 Updated by intrigeri 2018-02-03 07:15:04

  • Assignee changed from intrigeri to bertagaz

At first glance it looks like the implementation of ensure_hook_dependency_is_installed is not set -e-friendly but we’ll see how it fares in practice. Other than that, skimming over it it LGTM, I’ll let the RM review and check that the resulting packages list is identical to the one we get on the devel branch.

#6 Updated by anonym 2018-02-09 11:59:54

intrigeri wrote:
> At first glance it looks like the implementation of ensure_hook_dependency_is_installed is not set -e-friendly but we’ll see how it fares in practice.

I’m interested in what made you think this is the case (learning opportunity! :)). I had a quick look but couldn’t figure it out…

(… but I realized there’s a shell expansion issue which I fixed with commit:048350ce329e76adf267932dbc2e523625386780)

#7 Updated by intrigeri 2018-02-09 13:18:00

> intrigeri wrote:
>> At first glance it looks like the implementation of ensure_hook_dependency_is_installed is not set -e-friendly but we’ll see how it fares in practice.

> I’m interested in what made you think this is the case (learning opportunity! :)). I had a quick look but couldn’t figure it out…

In [ -z "${*}" ] && return, if -z "${*}" is false, then I would assume that under set -e the calling script will abort. No?

Same for is_package_installed "${p}" && continue, I would expect the calling script to abort if is_package_installed "${p}" returns non-zero.

#8 Updated by anonym 2018-02-10 13:33:30

intrigeri wrote:
> > intrigeri wrote:
> >> At first glance it looks like the implementation of ensure_hook_dependency_is_installed is not set -e-friendly but we’ll see how it fares in practice.
>
> > I’m interested in what made you think this is the case (learning opportunity! :)). I had a quick look but couldn’t figure it out…
>
> In [ -z "${*}" ] && return, if -z "${*}" is false, then I would assume that under set -e the calling script will abort. No?
>
> Same for is_package_installed "${p}" && continue, I would expect the calling script to abort if is_package_installed "${p}" returns non-zero.

Actually, the left operand of && (and ||, but in that case it’s already clear) is considered “explicitly tested” just like the conditional of an if statement, i.e. it is allowed to fail without aborting under set -e. That said, I’m not a fan of short circuiting, and your reaction strengthens this conviction, so I’ve converted to full if statements for clarity.

#9 Updated by intrigeri 2018-02-11 06:43:56

> Actually, the left operand of && (and ||, but in that case it’s already clear) is considered “explicitly tested” just like the conditional of an if statement, i.e. it is allowed to fail without aborting under set -e.

OOOOK!

> That said, I’m not a fan of short circuiting, and your reaction strengthens this conviction, so I’ve converted to full if statements for clarity.

Thanks. I prefer code that does not rely on such subtle, non obvious behavior.

#10 Updated by intrigeri 2018-02-19 14:27:02

  • Assignee changed from bertagaz to intrigeri

(As per discussion with anonym, in order to make bertagaz’ review plate lighter and increase the chances he can deal with what’s left there in a timely manner.)

#11 Updated by intrigeri 2018-02-19 14:43:05

Code review passes (not a surprise since I had already reviewed it incrementally :)

#12 Updated by intrigeri 2018-02-19 16:49:51

I should have a full test suite run done within 3.5 hours so expect a merge either late tonight of early tomorrow, if everything goes well :)

#13 Updated by intrigeri 2018-02-19 20:21:24

  • Status changed from In Progress to Fix committed
  • % Done changed from 60 to 100

Applied in changeset commit:59e9c68d126e40d52772e1101edb978d056f4d94.

#14 Updated by intrigeri 2018-02-19 20:22:17

  • Assignee deleted (intrigeri)
  • QA Check changed from Ready for QA to Pass

#15 Updated by bertagaz 2018-03-14 11:08:54

  • Status changed from Fix committed to Resolved