Bug #17646

Establish a coding standards baseline on our Ruby code base

Added by intrigeri 2020-04-22 05:51:58 . Updated 2020-05-19 13:19:34 .

Status:
Needs Validation
Priority:
Normal
Assignee:
anonym
Category:
Target version:
Tails_4.7
Start date:
Due date:
% Done:

40%

Feature Branch:
feature/17646-ruby-coding-standards-baseline+force-all-tests
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:

Description

Our Ruby code base is far from respecting best practices documented in the Ruby style guide: at the time I’m creating this issue, rubocop reports thousands of offenses.
This is not going to change overnight, let’s take it one step at a time and cut ourselves some slack.

For this first iteration, my main goal is to establish a Ruby coding standards baseline (not necessarily perfect), that would be encoded in such a way that:

  1. Developers can enable their IDE’s Rubocop integration to avoid introducing regressions wrt. said baseline.
  2. We can later add CI checks to ensure we don’t regress wrt. said baseline.
  3. It’s clear whether we diverge on purpose from the Ruby community’s established best practices, or not.
  4. We can propose & discuss changes to our coding standards. For example, we could decide to either respect rule X of the Ruby style guide, or to consciously diverge from it.

Subtasks

Related issues

Related to Tails - Bug #17675: Fix ShellCheck errors in our shell code base Needs Validation
Related to Tails - Bug #13459: Scenario "Booting Tails from a USB drive in UEFI mode" is fragile Needs Validation 2017-07-12

History

#1 Updated by intrigeri 2020-04-22 05:52:42

  • Feature Branch set to feature/17646-ruby-coding-standards-baseline+force-all-tests

#2 Updated by intrigeri 2020-04-22 06:00:44

So far, I’ve tried to create a branch that can be evaluated with the least amount of review work possible:

  • Most commits were generated automatically; I documented how they were generated.
  • Only a few commits were prepared by hand.

I would suggest the reviewer to not bother reviewing the changes generated automatically, but instead go through each commit one after the other, reproduce the automatically generated ones, review + cherry-pick the manual ones, and at several points in the process, verify that the current result matches the state of my branch at that point.

I appreciate that even this approach will take quite some time.

#3 Updated by intrigeri 2020-04-22 06:03:30

Meta: I’m doing this work as a fun volunteer project (even though I could argue that it fits into the bitrot management FT’s role, this approach is more pleasurable to me). Reviewing will of course be FT core work.

#4 Updated by intrigeri 2020-04-23 08:54:59

intrigeri wrote:
> So far, I’ve tried to create a branch that can be evaluated with the least amount of review work possible:

… and half-way through it I realized that this would not work because I committed the manual Rubocop config changes together with the automatic changes. So I gave up. Someone will have to do the full review, which will surely be a burden but IMO it’s a good investment for the future!

#5 Updated by intrigeri 2020-05-04 12:04:55

  • related to Bug #17675: Fix ShellCheck errors in our shell code base added

#6 Updated by intrigeri 2020-05-11 09:38:47

I’ll now try to stabilize this branch and send it to review ASAP, because:

  • Resolving merge conflicts wrt. updates done in stable won’t get any funnier over time.
  • My branch has a couple bug fixes (at least one robustness problem and a test that did not actually test what was intended).
  • Today I’ve realized I’ve started doing more refactoring on this branch, than does not fit into the scope of this issue. I should stop :)
  • This has been, and still is, fun volunteer work, but I expect it’ll turn into “Yet Another Piece Of WIP” sooner than I think ⇒ time to wrap this up!

#7 Updated by intrigeri 2020-05-14 13:44:56

  • related to Bug #13459: Scenario "Booting Tails from a USB drive in UEFI mode" is fragile added

#8 Updated by intrigeri 2020-05-14 13:58:44

  • Status changed from In Progress to Needs Validation
  • Assignee changed from intrigeri to anonym
  • Target version set to Tails_4.7

Hi @anonym,

The last 2 full test suite runs on Jenkins passed, modulo Bug #17278.
I would understand if you dreaded this a little, but I think it’s time to review and merge this branch!

I understand this is not exactly high priority, but given the nature of this branch, the more delays, the more merge conflicts, so the earlier this gets merged, the less extra work I’ll do. Also, I’d love to get the satisfaction to call this done :)

If there’s any Rubocop rule I’ve followed (or tweaked) that you strongly disagree with, to the point that you’d rather diverge from the defaults, please let me know: there’s value to strict’ish adherence to Ruby community standards, but IMO we have a little bit of wiggle room if something feels really too painful, or something.

Related: Bug #13459 goes together with this one.

Finally, I’ve not thought about integration into CI yet. On the one hand, it would probably be simple to integrate this into our Jenkins setup. OTOH, I’m tempted to use this as an opportunity to set this up in GitLab CI: there’s really no reason to wait 4+ hours in order to get the kind of feedback that Rubocop provides, and there’s probably no reason to hide this stuff behind closed doors; also, it could be a first step to get unit tests and linting tests running before integration tests. Anyway, I’m diverging and I really, really don’t want to block on this here at this point: let’s first get something worth testing :)

Cheers!

#9 Updated by anonym 2020-05-14 14:19:18

@intrigeri wrote:
> I would understand if you dreaded this a little, but I think it’s time to review and merge this branch!

Woah: “73 files changed, 3785 insertions(+), 3112 deletions(-)”

I’m not exactly sure how to even approach this. I guess I’ll mostly look at each commit’s intention (mainly reading the commit messages), skim the code and only object if there’s something particularly awkward, and otherwise just assume that you didn’t introduce any backdoor (:D) and that RuboCop’s transformations are safe (= don’t change behavior).

Let me know if you have any tips/ideas, or think the above approach isn’t good enough!

#10 Updated by intrigeri 2020-05-14 14:36:09

> I’m not exactly sure how to even approach this. I guess I’ll mostly look at each commit’s intention (mainly reading the commit messages), skim the code and only object if there’s something particularly awkward, and otherwise just assume that you didn’t introduce any backdoor (:D) and that RuboCop’s transformations are safe (= don’t change behavior).

This approach looks perfect to me for the first part of the branch, that’s mostly about automated (rubocop --only $CHECK --auto-correct) trivial layout/style/lint changes. I suppose most of that huge diff stat comes from there.

Now, to be honest, a bunch of the commits that come after those are more involved and may warrant a more careful review: I found myself in a clean up and refactoring vortex, which I had to forcibly extract myself (by stabilizing it + sending to review). At least, IIRC I managed to make atomic commits, including 1-commit-per-refactoring-step, and I tried to make the commit messages name the applied refactoring step: I’ve been thinking all along “I don’t want to create a monstrosity of a branch that nobody will want to review”. Let’s see how close to succeeding I’ve been.

#11 Updated by anonym 2020-05-19 12:39:51

  • Status changed from Needs Validation to In Progress
  • Assignee changed from anonym to intrigeri
  • % Done changed from 0 to 40

I think your work has broken try_tor() in some subtle way.

Just looking at the test job’s overview I was struck by how many instances of “Job ​run ​aborted: ​nothing ​happened ​since ​60 ​minutes” it was hit by. Indeed, Scenario: I am notified when Additional Software fails to install a package fails a lot at the

Then I see the "The installation of your additional software failed" notification after at most 300 seconds


step. In the debug log I see craziness like:

03:16:45.477855098: try_for: attempt 288 (3586.91s elapsed of 300s)...

Now, stuff like “301s elapsed of 300s” are to be expected, but this is clearly not right. I ran the following crappy shell snippet on jenkins, and it printed all debug.log:s containing a try_for() failure that has run 100s longer than it should:

find /var/lib/jenkins/jobs/ -name debug.log | while read log; do
  sed -n "s/^.*try_for: attempt .*(\([0-9]\+\)\(\.[0-9]\+\)\?s elapsed of \([0-9]\+\)\(\.[0-9]\+\)\?s).*/\1 \3/p" $log | while read a b; do
    if [ "$((${a} - 100))" -gt "${b}" ]; then
      echo $log
      break
    fi
  done
done | tee results.txt


and the only hits were from this branch. However, looking at the changes to try_for(), I don’t get it. Sure, the change of base exception for UniqueTryForTimeoutError and the differences in how we rescue exceptions are suspect, but look at the log around when the timeout occurs:

02:21:47.141593840: try_for: failed by code block returning failure
02:21:48.141978983: try_for: attempt 24 (289.58s elapsed of 300s)...
02:21:58.585131849: try_for: failed by code block returning failure
02:21:59.585565741: try_for: attempt 25 (301.02s elapsed of 300s)...
02:22:11.017073641: try_for: failed by code block returning failure
02:22:12.017424330: try_for: attempt 26 (313.45s elapsed of 300s)...

In all cases, no exception are thrown, the attempts just fail because the code block fails. It’s as if there is no timeout at all.

I’ll look a bit more…

#12 Updated by anonym 2020-05-19 12:54:50

  • Status changed from In Progress to Needs Validation
  • Assignee changed from intrigeri to anonym

anonym wrote:
> Sure, the change of base exception for UniqueTryForTimeoutError […] [is] suspect […]

This might be it. The very first run times out in this strange way, and it is at commit:970864144a4ef17f91fd82693c1f8bad2e668e7d, where less changes have been made to try_for(). The only one that stands out to me is:

-class UniqueTryForTimeoutError < Exception
+class UniqueTryForTimeoutError < RuntimeError

So I am gonna try to revert commit:794bb06db606215f491679935a4a56afb8c426a1 and see what Jenkins thinks! That will be in run #39 and on.

I also merged stable and fixed a conflict so this branch builds again.

#13 Updated by anonym 2020-05-19 13:19:34

anonym wrote:
> I ran the following crappy shell snippet on jenkins, and it printed all debug.log:s containing a try_for() failure that has run 100s longer than it should:
> […]
> and the only hits were from this branch.

Nope! Apparently I wasn’t looking carefully enough as there were other hits:

/var/lib/jenkins/jobs/test_Tails_ISO_devel/builds/2047/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_stable/builds/2324/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_stable/builds/2343/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_feature-17646-ruby-coding-standards-baseline-force-all-tests/builds/27/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_feature-17646-ruby-coding-standards-baseline-force-all-tests/builds/26/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_feature-17646-ruby-coding-standards-baseline-force-all-tests/builds/10/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_feature-17646-ruby-coding-standards-baseline-force-all-tests/builds/4/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_feature-17646-ruby-coding-standards-baseline-force-all-tests/builds/1/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_feature-17646-ruby-coding-standards-baseline-force-all-tests/builds/37/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_feature-17646-ruby-coding-standards-baseline-force-all-tests/builds/38/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_feature-17646-ruby-coding-standards-baseline-force-all-tests/builds/32/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_feature-17646-ruby-coding-standards-baseline-force-all-tests/builds/23/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_feature-17646-ruby-coding-standards-baseline-force-all-tests/builds/14/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_feature-17646-ruby-coding-standards-baseline-force-all-tests/builds/20/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_feature-17646-ruby-coding-standards-baseline-force-all-tests/builds/22/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_feature-17646-ruby-coding-standards-baseline-force-all-tests/builds/13/archive/build-artifacts/debug.log
/var/lib/jenkins/jobs/test_Tails_ISO_test-17596-support-unset-xdg-session-type/builds/3/archive/build-artifacts/debug.log

However, this branch is suspiciously over-represented so I think investigating this still is warranted — this branch seems to make it worse!

Stepping back and analysing what I’m seeing, I notice:

  • It is very odd that the only place this error occurs in this branch is in the I see the "..." notification after at most ... seconds step. It’s not doing anything differently than most other invocations.
  • It’s worth noting that this branch has seen this specific try_for() both succeed (e.g. #17 and fail (e.g. #8 without triggering this bug, so it isn’t deterministic (yay…).