Code review comment for lp:~mterry/deja-dup/no-python2

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

Hey Michael! Some review before some general thoughts :)

Code-wise:
I didn't spot anything horrible here at least. Everything should be working fine. It builds as well and seems to work on my existing install (even removing duplicity).

Tests:
I couldn't get the tests passing on sbuild (wily or xenial). The failing tests are here:
60% tests passed, 37 tests failed out of 93

Total Test time (real) = 82.80 sec

The following tests FAILED:
Errors while running CTest
  48 - permission.test (OTHER_FAULT)
  50 - no-space.test (OTHER_FAULT)
  51 - threshold-full.test (OTHER_FAULT)
  52 - delete-never.test (OTHER_FAULT)
  53 - clean-cache.test (OTHER_FAULT)
  54 - excludes.test (OTHER_FAULT)
  55 - symlink-direct.test (OTHER_FAULT)
  56 - symlink-trickshot.test (OTHER_FAULT)
  57 - stop.test (OTHER_FAULT)
  58 - mkdir.test (OTHER_FAULT)
  59 - disk-small.test (OTHER_FAULT)
  60 - bad-volume.test (OTHER_FAULT)
  61 - symlink-exclude2.test (OTHER_FAULT)
  62 - encrypt-ask.test (OTHER_FAULT)
  63 - threshold-inc.test (OTHER_FAULT)
  64 - instance-error.test (OTHER_FAULT)
  65 - symlink-recursive.test (OTHER_FAULT)
  66 - read-error.test (OTHER_FAULT)
  67 - disk-full.test (OTHER_FAULT)
  68 - symlink-follow2.test (OTHER_FAULT)
  70 - delete-too-old.test (OTHER_FAULT)
  71 - symlink-subdir.test (OTHER_FAULT)
  72 - cancel.test (OTHER_FAULT)
  73 - encrypt-detect.test (OTHER_FAULT)
  75 - bad-hostname.test (OTHER_FAULT)
  76 - delete-just-right.test (OTHER_FAULT)
  77 - special-chars.test (OTHER_FAULT)
  79 - delete-too-few.test (OTHER_FAULT)
  80 - nag.test (OTHER_FAULT)
  83 - disk-full2.test (OTHER_FAULT)
  84 - verify.test (OTHER_FAULT)
  86 - clean-tempdir.test (OTHER_FAULT)
  87 - symlink-exclude.test (OTHER_FAULT)
  88 - symlink-follow.test (OTHER_FAULT)
  89 - cancel-noop.test (OTHER_FAULT)
  91 - symlink-loop.test (OTHER_FAULT)
  92 - clean-incomplete.test (OTHER_FAULT)

In general:
I feel quite uncomfortable about this change claiming "dropping python support" by installing duplicity on the fly (and so, dropping it as a recommend).
First, I guess that duplicity will need to go to universe so that it's not pulled when we build the iso. Does it means that duplicity isn't supported officially anymore?
Secondly (and that's my main point), from the code and my understanding, duplicity is the only supported backend, right? Meaning, deja-dup is useless without it? That means that we will ship useless software by default and anyone who wants to do backups will then have to install duplicity, assuming it's supported by default as it's coming from an application which is supported. I feel this is just ticking up a box "no python2 on the install" where it's lying to ourself: most of users will still have python2 installed if we apply the same semantic to multiple projects. That's the reason I don't feel that this change is fully right.

Of course, if déjà-dup works well without duplicity for other form of backups, I withdraw that comment :)

review: Needs Fixing

« Back to merge proposal