Tails

#1 Updated by anonym 2017-05-30 20:12:39

• Assignee changed from anonym to intrigeri
• QA Check set to Info Needed

I think this should be fixed until 3.0 and I volunteer to do so as soon as we have an agreement about the the problems and the proposed solutions in the description.

#2 Updated by intrigeri 2017-05-31 07:53:28

• Assignee changed from intrigeri to anonym
• QA Check changed from Info Needed to Dev Needed

> This in turn breaks all automated tests using Tor, since the Tor is ready step runs systemctl is-system-running. The placement of that check in that step is my second issue: it doesn’t belong. It should be moved into a dedicated step, run in a scenario about the health of the system. A failure would then in itself immediately signal something quite descriptive about the actual failure, instead of falsely signaling that something about little-t tor is utterly broken, blocking a huge part of the test suite.

Well, IMO it’s good that we run systemctl is-system-running as often as we can, before testing other stuff, to identify issues as early as possible. IIRC we can’t run it for offline scenarios since this check will fail if Tor hasn’t started or bootstrapped. But I agree the current way this is implemented gives suboptimal feedback. Perhaps renaming the “Tor is ready” step to describe more accurately what we test in it would help. If we don’t easily agree on a solution here, let’s maybe move this to a dedicated ticket: it’s definitely not a 3.0 blocker IMO so I’d like to focus on the fuzzy patch issue.

> Lastly, my third problem: given how we order our build hooks, i.e.:
>

> config/chroot_local-hooks/01-check-for-dot-orig-files
> config/chroot_local-hooks/19-install-tor-browser-AppArmor-profile
> 

> we don’t detect this source of fuzzy patches (but it’s the only relevant one at the
> moment). IIRC we had a reason for this at some point, but I cannot recall or
> rediscover it.

We do it this way to fail earlier, and report something clear about why the build fails, when one of our chroot_local-patches is fuzzy: it’s plausible that a hook can fail due to a fuzzy patch, and then we would be left wondering what’s going on if we did this check later.

> Otherwise I propose that we move this hook to one of the very last ones,

I think we should run it both very early and very late. Perhaps a symlink would do the job?

> To avoid frequent FTBFS we should add it to DOT_ORIG_WHITELIST however.

“it” == ?

> (BTW: shouldn’t we rm -f ${DOT_ORIG_WHITELIST}? Why would we want to ship crap? Assuming, of course, that these files don’t matter, which I might be wrong about.)

Good idea.

#3 Updated by anonym 2017-05-31 12:25:27

• Subject changed from When our Tor Browser AppArmor profile patch is fuzzy all Tor-related automated tests break to We don't detect when our Tor Browser AppArmor profile patch is fuzzy and leaves a .orig file
• Status changed from Confirmed to In Progress
• Assignee changed from anonym to intrigeri
• % Done changed from 0 to 50
• QA Check changed from Dev Needed to Ready for QA
• Feature Branch set to bugfix/12617-fuzzy-patch-detection-improvements

intrigeri wrote:
> > This in turn breaks all automated tests using Tor, since the Tor is ready step runs systemctl is-system-running. The placement of that check in that step is my second issue: it doesn’t belong. It should be moved into a dedicated step, run in a scenario about the health of the system. A failure would then in itself immediately signal something quite descriptive about the actual failure, instead of falsely signaling that something about little-t tor is utterly broken, blocking a huge part of the test suite.
>
> Well, IMO it’s good that we run systemctl is-system-running as often as we can, before testing other stuff, to identify issues as early as possible. IIRC we can’t run it for offline scenarios since this check will fail if Tor hasn’t started or bootstrapped. But I agree the current way this is implemented gives suboptimal feedback. Perhaps renaming the “Tor is ready” step to describe more accurately what we test in it would help. If we don’t easily agree on a solution here, let’s maybe move this to a dedicated ticket: it’s definitely not a 3.0 blocker IMO so I’d like to focus on the fuzzy patch issue.

Yeah, I don’t agree at all with your current argument so let’s not count on a quick agreement. I created Bug #12621, so let’s focus on the fuzzy patch issue in this ticket (which I renamed accordingly).

> > Lastly, my third problem: given how we order our build hooks, i.e.:
> > […]
> > we don’t detect this source of fuzzy patches (but it’s the only relevant one at the
> > moment). IIRC we had a reason for this at some point, but I cannot recall or
> > rediscover it.
>
> We do it this way to fail earlier, and report something clear about why the build fails, when one of our chroot_local-patches is fuzzy: it’s plausible that a hook can fail due to a fuzzy patch, and then we would be left wondering what’s going on if we did this check later.

True.

> > Otherwise I propose that we move this hook to one of the very last ones,
>
> I think we should run it both very early and very late. Perhaps a symlink would do the job?

Yes, brilliant!

> > To avoid frequent FTBFS we should add it to DOT_ORIG_WHITELIST however.
>
> “it” == ?

“it” == /etc/apparmor.d/torbrowser.orig

The current branch should fix all issues this ticket now is about. Please review’n’merge!

#4 Updated by intrigeri 2017-05-31 12:58:39

• Assignee changed from intrigeri to anonym
• QA Check changed from Ready for QA to Dev Needed

I think you forgot to push this branch.

#5 Updated by intrigeri 2017-05-31 13:38:11

• Assignee changed from anonym to intrigeri
• QA Check changed from Dev Needed to Ready for QA

#6 Updated by intrigeri 2017-05-31 13:45:44

• Assignee changed from intrigeri to anonym
• QA Check changed from Ready for QA to Info Needed

I dislike commit:3cad513c7c9d16daa9734009b5ee4ca1b7dc042b: IMO failing hard and early is the best way to make us (Foundations Team) notice when something is wrong, and the way it’s done currently makes it trivial to understand the root cause of the problem (we receive a build notification failure, scroll to the bottom, and then it’s clearly written what’s wrong). I wonder where the “That patch becomes fuzzy too often” rationale comes from: before the last unfuzzy-ing in May 2017, last times we had to do it were in March and July 2016, so perhaps it’s not that big a burden and doesn’t warrant building known-bad ISOs just for the sake of avoiding FTBFS?

Other than that, code review passes!

#7 Updated by anonym 2017-06-01 01:43:42

• QA Check changed from Info Needed to Dev Needed

intrigeri wrote:
> I dislike commit:3cad513c7c9d16daa9734009b5ee4ca1b7dc042b: IMO failing hard and early is the best way to make us (Foundations Team) notice when something is wrong, and the way it’s done currently makes it trivial to understand the root cause of the problem (we receive a build notification failure, scroll to the bottom, and then it’s clearly written what’s wrong). I wonder where the “That patch becomes fuzzy too often” rationale comes from: before the last unfuzzy-ing in May 2017, last times we had to do it were in March and July 2016, so perhaps it’s not that big a burden and doesn’t warrant building known-bad ISOs just for the sake of avoiding FTBFS?

I’m worrying about when the signal to noise ratio becomes too high, but I now realize this should only affect devel (since it follows the latest snapshots). So I agree, I’ll revert that commit, and push a refreshed patch as a fix for this ticket instead.

#8 Updated by anonym 2017-06-01 16:56:57

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

Done!

#9 Updated by intrigeri 2017-06-01 19:25:55

• Assignee changed from intrigeri to anonym
• QA Check changed from Ready for QA to Dev Needed

I’ll have a look once it builds (again) on Jenkins: I’m sorry to be painful but I’m tired of dealing with Bug #12611 myself on everyone’s topic branches.

#10 Updated by intrigeri 2017-06-02 06:18:24

• Assignee changed from anonym to intrigeri
• QA Check changed from Dev Needed to Ready for QA

#11 Updated by intrigeri 2017-06-02 06:20:51

• % Done changed from 60 to 70

Code review passes, waiting for a build.

#15 Updated by intrigeri 2017-06-02 12:11:54

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

Applied in changeset commit:cf5a2f661a10c096f168ab6a89b035946e418f6d.

#16 Updated by intrigeri 2017-06-02 12:20:30

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

#17 Updated by intrigeri 2017-06-05 15:20:49

• has duplicate Bug #11233: Build doesn't abort when Tor Browser AppArmor patches does not apply cleanly added

#18 Updated by intrigeri 2017-06-12 16:07:42

• Status changed from Fix committed to Resolved