Bug #12527

Duplicate gpg-agent killing code introduced in the build-tails script

Added by intrigeri 2017-05-10 11:16:51 . Updated 2017-05-12 09:19:52 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Build system
Target version:
Tails_3.0~rc1
Start date:
2017-05-10
Due date:
% Done:

100%

Feature Branch:
bugfix/12527-really-fix-unmounting-on-build-cleanup
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:
289

Description

commit:75060eeee5f87e1aac472ebeb4596b2a7e3df2f3 feels wrong: cleanup runs remove_build_dirs that kills blocking processes already, in a way that’s both more generic (even processes other than gpg-agent are killed) and more accurate (unrelated gpg-agent processes are not affected). So I’m not quite sure why we need to duplicate this. Reading https://jenkins.tails.boum.org/view/RM/job/build_Tails_ISO_feature-stretch-unfrozen/lastFailedBuild/console I see “fuser: command not found”; I suspect that’s why the existing code doesn’t work. So I suggest making the existing (and IMO better) code work, instead of duplicating it. What do you think?

Subtasks

Related issues

Related to Tails - Bug #12537: Killall is not installed in the VM Duplicate 2017-05-12

History

#1 Updated by intrigeri 2017-05-10 11:56:11

#2 Updated by bertagaz 2017-05-10 13:56:05

  • Status changed from Confirmed to In Progress

Applied in changeset commit:2fbc3ea0bd0110d2893a06e5cfdd9162b305b033.

#3 Updated by bertagaz 2017-05-10 20:38:10

  • Assignee changed from bertagaz to intrigeri
  • % Done changed from 0 to 50
  • QA Check set to Ready for QA
  • Feature Branch set to bugfix/12527-really-fix-unmounting-on-build-cleanup

intrigeri wrote:
> commit:75060eeee5f87e1aac472ebeb4596b2a7e3df2f3 feels wrong: cleanup runs remove_build_dirs that kills blocking processes already, in a way that’s both more generic (even processes other than gpg-agent are killed) and more accurate (unrelated gpg-agent processes are not affected). So I’m not quite sure why we need to duplicate this. Reading https://jenkins.tails.boum.org/view/RM/job/build_Tails_ISO_feature-stretch-unfrozen/lastFailedBuild/console I see “fuser: command not found”; I suspect that’s why the existing code doesn’t work. So I suggest making the existing (and IMO better) code work, instead of duplicating it. What do you think?

Indeed, I misunderstood the error message. I’ve pushed a branch removing the duplicated code, and it works fine with just the addition of ‘psmisc’ to the vagrant build VM, see https://jenkins.tails.boum.org/job/build_Tails_ISO_bugfix-12527-really-fix-unmounting-on-build-cleanup/1/console

Please merge if happy.

#4 Updated by intrigeri 2017-05-11 08:48:22

  • Assignee changed from intrigeri to bertagaz
  • % Done changed from 50 to 70
  • QA Check changed from Ready for QA to Dev Needed

Wait, we need this fix on the stable branch too, no? It seems to be based on feature/stretch. Other than that, the commit looks good, so feel free to merge the branch yourself in stable, devel and feature/stretch once you’ve first rebased it on stable.

#5 Updated by anonym 2017-05-11 09:18:03

intrigeri wrote:
> Wait, we need this fix on the stable branch too, no?

Yes.

Also, since these fixes are “later” in the Git history than the base box version bump, we are affected by the issue identified in Feature #12409#note-32. For instance, I had actually built the current base box before the commit installing psmisc into the basebox, so the invocation of killall failed => build failure due to set -e. So, please also release a new base box (or review’n’merge feature/12409-improved-vagrant-base-box-versioning).

#6 Updated by bertagaz 2017-05-11 10:42:25

anonym wrote:
> intrigeri wrote:
> > Wait, we need this fix on the stable branch too, no?
>
> Yes.

I did base it on feature/stretch cause I didn’t see this error on stable or devel. I’ll rebase that branch on stable.

> Also, since these fixes are “later” in the Git history than the base box version bump, we are affected by the issue identified in Feature #12409#note-32. For instance, I had actually built the current base box before the commit installing psmisc into the basebox, so the invocation of killall failed => build failure due to set -e. So, please also release a new base box (or review’n’merge feature/12409-improved-vagrant-base-box-versioning).

Ok, I ought to r’n’m the Feature #12409 branch before the end of the week, so I’ll do that.

#7 Updated by intrigeri 2017-05-11 10:46:07

> I did base it on feature/stretch cause I didn’t see this error on stable or devel.

Right, this can only happen with GnuPG 2.x.

> I’ll rebase that branch on stable.

No need to then, you can merge it as-is into feature/stretch :)

> Ok, I ought to r’n’m the Feature #12409 branch before the end of the week, so I’ll do that.

Please save some time (after r’n’m-ing) for stabilizing by the end of the week anything it might break once deployed ;)

#8 Updated by bertagaz 2017-05-12 08:52:33

  • related to Bug #12537: Killall is not installed in the VM added

#9 Updated by bertagaz 2017-05-12 09:09:46

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

Applied in changeset commit:e82b1b6e5f95a8e12f78204a8297f9cb6f64aa06.

#10 Updated by bertagaz 2017-05-12 09:19:52

  • Status changed from Fix committed to Resolved
  • Assignee deleted (bertagaz)
  • QA Check changed from Dev Needed to Pass

Merged!