Tails

#1 Updated by intrigeri 2013-07-19 09:09:44

• Starter set to Yes

#2 Updated by intrigeri 2013-10-04 01:23:26

• Category set to Installation

#3 Updated by BitingBird 2014-06-09 10:06:10

• Subject changed from liveusb-creator vs. empty partition upgrade to Liveusb-creator vs. empty partition upgrade

#4 Updated by BitingBird 2014-06-09 15:32:45

• Subject changed from Liveusb-creator vs. empty partition upgrade to liveusb-creator vs. empty partition upgrade

#5 Updated by intrigeri 2014-12-01 18:52:50

• related to Bug #7659: "Upgrade from ISO" doesn't error on fresh disk added

#6 Updated by BitingBird 2015-01-04 19:06:19

• Affected tool set to Installer

#7 Updated by intrigeri 2015-04-24 03:12:03

• Subject changed from liveusb-creator vs. empty partition upgrade to Tails Installer should refuse upgrading a device that hasn't Tails installed
• Description updated

#8 Updated by intrigeri 2015-04-24 03:12:37

• related to Feature #8865: Implement "Install from ISO" in Tails Installer added

#9 Updated by intrigeri 2015-04-24 03:15:08

• Description updated

#10 Updated by intrigeri 2015-04-24 03:16:33

• Assignee set to kurono

kurono, I’ve updated the description to match my current understanding of the problem and what should be done to address it. Do you want to take it, e.g. for 1.4.1 or 1.5?

#11 Updated by BitingBird 2015-04-24 04:48:08

• has duplicate Bug #9275: Can "Clone & Upgrade" to a small device which does not have Tails installed added

#12 Updated by kurono 2015-04-29 03:39:45

• Target version set to Tails_1.4.1

#13 Updated by kurono 2015-05-05 18:33:35

• Assignee deleted (kurono)
• QA Check set to Ready for QA
• Feature Branch set to feature/5623-Installer-should-refuse-empty-device

#14 Updated by intrigeri 2015-05-05 19:48:41

• Assignee set to intrigeri

#15 Updated by intrigeri 2015-05-06 10:08:43

• Feature Branch changed from feature/5623-Installer-should-refuse-empty-device to kurono:feature/5623-Installer-should-refuse-empty-device

#16 Updated by intrigeri 2015-05-06 10:09:01

• Status changed from Confirmed to In Progress
• % Done changed from 0 to 20

#17 Updated by intrigeri 2015-05-06 10:14:32

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

Thanks! I didn’t test it, but the code looks good.

Two suggestions:

• I think the “GPT + vfat + partition label” test should live in a dedicated function. This would make the code more readable, and I bet we’ll need to reuse it elsewhere when porting Tails Installer to regular Debian/Ubuntu.
• I’m pretty sure that the “The selected device does not have a Tails installation, it is required for the upgrade process.” new string is not up to our GUI standards => please ask sajolida or tails-ux@ for a better phrasing (you’ll want to point them at this ticket, so that they understand in which case the string is shown, even if they don’t read/understand the code).

#19 Updated by kurono 2015-05-12 17:11:41

• Assignee deleted (kurono)
• QA Check changed from Dev Needed to Ready for QA

#20 Updated by intrigeri 2015-05-13 08:16:29

• Assignee set to intrigeri

#21 Updated by intrigeri 2015-05-15 19:07:39

• Feature Branch changed from kurono:feature/5623-Installer-should-refuse-empty-device to feature/5623-Installer-should-refuse-empty-device

Build and uploaded a package, the topic branch should have it now.

#22 Updated by intrigeri 2015-05-15 19:08:12

• % Done changed from 20 to 30

… and code review looks as good as one can expect on this codebase.

#23 Updated by intrigeri 2015-05-16 08:55:12

• Assignee changed from intrigeri to kytv
• % Done changed from 30 to 50

Wrote a couple automated tests to ensure this works as designed. Kill Your TV, may you please run usb_install.feature (and all other features that are somehow affected by usb.rb) and review my tests? Then, once happy, please reassign to me for merging.

#24 Updated by kytv 2015-05-19 10:56:28

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

The tests look good (of course :), but due to what may be described as a regression in Tails Installer the following scenario fails:

  @keep_volumes
  Scenario: Try upgrading but end up installing Tails to a USB drive containing a Tails isohybrid installation # features/usb_install.feature:320
    Given a computer                                                                                           # features/step_definitions/common_steps.rb:66
    And I start Tails from DVD with network unplugged and I login                                              # features/step_definitions/common_steps.rb:154
    And I plug USB drive "isohybrid"                                                                           # features/step_definitions/common_steps.rb:92
    And I try a "Clone & Upgrade" Tails to USB drive "isohybrid"                                               # features/step_definitions/usb.rb:153
    But I am suggested to do a "Clone & Install"                                                               # features/step_definitions/usb.rb:179
      FindFailed: can not find USBSuggestsInstall.png on the screen.
      Line ?, in File ? (RuntimeError)
      features/usb_install.feature:325:in `But I am suggested to do a "Clone & Install"'
    And I kill the process "liveusb-creator"                                                                   # features/step_definitions/common_steps.rb:499
    And I "Clone & Install" Tails to USB drive "isohybrid"                                                     # features/step_definitions/usb.rb:141
    Then the running Tails is installed on USB drive "isohybrid"                                               # features/step_definitions/usb.rb:284
    But there is no persistence partition on USB drive "isohybrid"                                             # features/step_definitions/usb.rb:300
    And I unplug USB drive "isohybrid"

Instead of the string that was previously displayed (Unsupported filesystem iso9660..), on the screen is (The destination device is not a Tails device...).

#25 Updated by kytv 2015-05-19 10:56:51

• File usb_install.png added

#26 Updated by intrigeri 2015-05-19 12:01:28

• Assignee changed from intrigeri to kurono

> Instead of the string that was previously displayed (Unsupported filesystem iso9660..), […]

Kill Your TV: good catch, thanks! I should really have run the entire usb_install.feature before sneaking this ticket onto your plate. Sorry.

kurono: so, this means that the proposed branch currently introduces a regression when attempting to upgrade a Tails device that was created with hybrid ISO + cat/dd — it will be very confusing to users to be told that their Tails device “is not a Tails device”. However, I think we can easily work this around by making the error message we display a bit more generic, so that it takes the hybrid ISO + cat/dd use case into account; e.g. in Tails Upgrader, we say “was not created using Tails Installer” => please propose an alternate wording (ideally, we should just reuse the Upgrader’s one) and have it reviewed by -ux@. Note, though, that the message we currently (Tails 1.4) display when attempting to upgrade a device created with hybrid ISO + cat/dd helps a bit the user decide what to do, and we should IMO not lose that.

Also, I believe we can simply kill the code that handles iso9660 as a special case, since it is not reachable anymore, right?

On second thought, one concern I have with the current implementation is that it only works in GUI mode, right? (e.g. the iso9660 special handling looks like it should work in command-line mode as well) That’s a concern since we’ll pretty soon want to polish the user story we provide for command-line users (Feature #8861). Do you think this can be fixed by some light refactoring of the code you added?

#27 Updated by intrigeri 2015-06-05 15:38:34

• has duplicate Feature #6054: Tails Installer should hide the USB stick that contains the ISO added

#28 Updated by kurono 2015-06-24 08:44:41

• Assignee deleted (kurono)
• QA Check changed from Dev Needed to Ready for QA

#29 Updated by intrigeri 2015-06-28 02:10:09

• Assignee set to intrigeri

#30 Updated by intrigeri 2015-06-28 03:48:31

• Target version changed from Tails_1.4.1 to Tails_1.5

Too late => postponing.

#32 Updated by intrigeri 2015-07-03 07:59:00

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

kurono: I don’t understand why new code was added to the _refresh_overlay_slider method, which seems to be totally unrelated to the problem at hand. Isn’t there a better place to put it?

#35 Updated by kurono 2015-07-09 08:36:22

• Assignee deleted (kurono)
• QA Check changed from Dev Needed to Ready for QA

#36 Updated by intrigeri 2015-07-09 08:58:17

• Assignee set to intrigeri

#38 Updated by intrigeri 2015-07-09 11:47:05

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

Updated test suite, it’s all green.

However: is it on purpose that “USB device found” is displayed even when no suitable device is listed (in my case, the only device is empty and thus cannot be upgraded)? I find this a little bit confusing.

#39 Updated by kurono 2015-07-13 10:10:42

• Assignee deleted (kurono)
• QA Check changed from Info Needed to Ready for QA

I forgot to remove a line, that was removed in https://labs.riseup.net/code/issues/9130 anyway.
The new change is in https://git-tails.immerda.ch/kurono/liveusb-creator/log/?h=feature/5623-Installer-should-refuse-empty-device

#40 Updated by intrigeri 2015-07-16 06:36:31

• Assignee set to intrigeri

#41 Updated by intrigeri 2015-07-18 04:44:55

• Assignee changed from intrigeri to kurono
• % Done changed from 50 to 60
• QA Check changed from Ready for QA to Info Needed

Cool, thanks. Another (and hopefully last) question: on Feature #9734 you wrote " I decided to remove the exception since an user can have several devices conected and it does not make sense to completely stop the installer only because one of them is small". I think it totally makes sense => why not do the same on this branch as well?

#42 Updated by kurono 2015-07-21 12:45:28

• Assignee deleted (kurono)
• QA Check changed from Info Needed to Ready for QA

Ok, this is ready.

#43 Updated by intrigeri 2015-07-29 01:27:40

• Assignee set to anonym

(Assigning to the RM to make it clear that I won’t take care of this one.)

#44 Updated by intrigeri 2015-08-03 07:41:02

• Assignee changed from anonym to intrigeri

#45 Updated by intrigeri 2015-08-03 08:56:28

• Assignee changed from intrigeri to kurono
• QA Check changed from Ready for QA to Dev Needed
• Feature Branch changed from feature/5623-Installer-should-refuse-empty-device to kurono:feature/5623-Installer-should-refuse-empty-device

Thanks! However, this doesn’t merge cleanly into current master => please merge master into your topic branch, resolve conflicts, test, and resubmit for QA.

#46 Updated by kurono 2015-08-03 10:33:55

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

Ok! I have merged and tested it, and also added a minor change.
Branch: https://git-tails.immerda.ch/kurono/liveusb-creator/log/?h=feature/5623-Installer-should-refuse-empty-device

#47 Updated by intrigeri 2015-08-03 11:40:31

• % Done changed from 60 to 70

Code review passes, yay!

Still, two comments that will become important since you intend to extend the scope of your contributions to Tails, possibly to areas where we care a bit more about having a nice and clean Git history:

• commit 848824f: fixing merge conflicts should be done in the merge commit itself, not in a follow-up one; git commit --amend may help :)
• commit e43af9e: “small change to message” is not a terribly useful commit message; in this case, I would prefer something like “don’t localize error message a second time: this was done when defining the ‘message’ string already”; see what I mean?

Next step for me is to build an ISO with these changes in, and to run (+ adjust if needed) the tests I’ve written weeks ago.

#48 Updated by intrigeri 2015-08-04 03:09:51

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

Applied in changeset commit:d5c6915dfc8585ad8c033a2e5aaa603752ce6d0a.

#50 Updated by intrigeri 2015-08-04 03:11:21

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

Note that I’ve dared merging my test suite changes corresponding to this branch, since kytv already reviewed and tested them earlier.

#51 Updated by BitingBird 2015-08-11 10:42:15

• Status changed from Fix committed to Resolved

#52 Updated by intrigeri 2015-11-12 04:10:38

• related to Bug #10539: "Clone and upgrade" on Jessie pretends my Tails was not installed with our Installer added