Project

General

Profile

Feature #14720

Feature #9005: Improve Tails Installer UX: phase 1

Fix self.opts.partition vs. self.force_reinstall semantics

Added by anonym 3 months ago. Updated about 1 month ago.

Status:
In Progress
Priority:
Normal
Assignee:
Category:
Installation
Target version:
Start date:
09/25/2017
Due date:
% Done:

60%

QA Check:
Dev Needed
Feature Branch:
kurono:installer/feature/9005-Improve-tails-installer, installer:debian_feature/9005-post-5.0.1-fixes, feature/9005-post-5.0.1-fixes
Type of work:
Code
Blueprint:
Starter:
Affected tool:
Installer

Description

This is a follow-up on #8859 and #8860. Details will follow in the comments.

Associated revisions

Revision a2630100 (diff)
Added by anonym about 1 month ago

Test suite: test re-installing over an existing Tails installation.

As kurono's work on #14720 showed, the Installer's "re-install"
feature is a bit fragile, so this is a preemptive regression test for
when he starts working on it again. :)

Refs: #14720

History

#1 Updated by anonym 3 months ago

From intrigeri's code review (#8860#note-40):

I'm a bit confused by the self.opts.partition vs. self.force_reinstall semantics. I understand we need to treat them differently in gui.py, but I wonder why it has to leak into creator.py. But perhaps I'm just confused because some other areas where force_reinstall is used are unclear to me:

  • I've not tested yet but the code suggests that "If there is valid Tails partition", we'll display an "Upgrade" button while we're going to partition the device and thus delete any existing persistent volume. That feels weird to me, but I don't have the big picture in mind. If that's what sajolida wants, fine. If that was unspecified, then I don't know :)
  • We now seem to do "Remove parent drives if a valid partition exists" even for initial installation, while we previously did that only for upgrades. That feels weird at first glance. If this is correct, a comment explaining why would be welcome.

#2 Updated by anonym 3 months ago

From #8860#note-47:

To me it seems like all this is baggage from when Tails Installer only had one mode, and didn't switch between them depending on the target device, like now. In fact, I feel the proper fix is to drop self.opts.partition, self.force_reinstall etc. from TailsInstallerWindow and LinuxTailsInstallerCreator, and make these properties of each drive, probably set when they are scanned in detect_supported_drives().

#3 Updated by kurono 2 months ago

anonym wrote:

From intrigeri's code review (#8860#note-40):

I'm a bit confused by the self.opts.partition vs. self.force_reinstall semantics. I understand we need to treat them differently in gui.py, but I wonder why it has to leak into creator.py. But perhaps I'm just confused because some other areas where force_reinstall is used are unclear to me:

  • I've not tested yet but the code suggests that "If there is valid Tails partition", we'll display an "Upgrade" button while we're going to partition the device and thus delete any existing persistent volume. That feels weird to me, but I don't have the big picture in mind. If that's what sajolida wants, fine. If that was unspecified, then I don't know :)
  • We now seem to do "Remove parent drives if a valid partition exists" even for initial installation, while we previously did that only for upgrades. That feels weird at first glance. If this is correct, a comment explaining why would be welcome.
The general idea is:
  • We have a drop down list with all the available USB sticks, we can use for installing Tails.
  • The selected device comes with all the relevant data for installing or upgrading Tails on it, like size, mount path, etc.
  • If the device has tails, we show in the list the relevant partition, for example /dev/sdc1
  • If not, we show the full device, for example /dev/sdc
  • if the device has tails, we delete the parent, otherwise we would have both /dev/sdc1 and /dev/sdc in the list, which is confusing and prone to errors
  • if the selected device has Tails, lets say /dev/sdc1, we show the upgrade button. If it is pressed, it keeps the partitions unchanged and including the persistent. It just deletes the files and copy the new ones. But in addition to the upgrade button, there is a link called reinstall, which forces to delete the partitions and create new ones (exactly like installing from scratch).
  • So the tricky part, is when the user selects reinstall, because we need to come back to the parent device /dev/sdc, that we have deleted. That is way we rescan the devices and leak force_reinstall to creator.py,
    so we can get the info from the parent.
  • On the other hand elf.opts.partition is an option that comes from the command line and self.force_reinstall is something I have add to seal to this special situation, that we did not have before, because we knew before hand exactly what operation we were going to apply. Now it becomes dinamic, we can switch from install to upgrade in the same device
  • Maybe there is a more elegant solution, to get the parent's info when a partition "/dev/sdc1" is going to be reinstalled.

#4 Updated by kurono about 2 months ago

  • QA Check set to Ready for QA
  • Feature Branch set to feature/9005-Improve-tails-installer

I have found a better solution for the "leaking force_reinstall" problem.
Now the installer stores the parent information of valid tails partitions,
so if the user selects the reinstall link, we can get that info without having to make more extra weird tricks at creator.py

For the second point, as @anonym says we need to remove the parents for all the devices now, since we are relaying in the previous splash options. So, if we leave that as it was before, it will list valid partitions and their parents in the dropdown list, which is confusing.

#5 Updated by intrigeri about 1 month ago

  • Assignee changed from anonym to intrigeri

#6 Updated by intrigeri about 1 month ago

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

Looks good to me. So code review passes for #14720, #14721, #14722 and #14723, thanks! :)

Next step is to build & upload a Debian package and see what our test suite thinks of these changes. I'll do that tomorrow unless anonym takes this ticket back in the meantime.

#7 Updated by intrigeri about 1 month ago

  • Feature Branch changed from feature/9005-Improve-tails-installer to kurono:installer/feature/9005-Improve-tails-installer, installer:debian_feature/9005-post-5.0.1-fixes, feature/9005-post-5.0.1-fixes

Uploaded a package, let's see what Jenkins thinks.

#8 Updated by intrigeri about 1 month ago

  • % Done changed from 50 to 60

Two partial test suite runs passed 100% on Jenkins, but to be on the safe side I'll do a full run before merging.

#9 Updated by intrigeri about 1 month ago

Full test suite passed except 2 unrelated failures (1 known issue reported a few days ago, 1 probable local network problem).

#10 Updated by intrigeri about 1 month ago

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

I think I've found a regression introduced by this branch:

  1. I start Tails built from this branch
  2. I plug in a Tails 3.2 USB stick
  3. I start Tails Installer with DEBUG=1 in a Terminal
  4. I click "Reinstall"
  5. Here's what I see:
2017-11-10 11:54:54,533 [creator.py:296 (popen)] DEBUG: /sbin/sgdisk --print /dev/sdc1
2017-11-10 11:54:54,555 [gui.py:264 (run)] ERROR: 'NoneType' object has no attribute '__getitem__'
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/tails_installer/gui.py", line 200, in run
    self.live.drives[parent_data['device']] = parent_data
TypeError: 'NoneType' object has no attribute '__getitem__'
2017-11-10 11:54:54,557 [gui.py:265 (run)] DEBUG: Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/tails_installer/gui.py", line 200, in run
    self.live.drives[parent_data['device']] = parent_data
TypeError: 'NoneType' object has no attribute '__getitem__'

anonym says "actually, the way I'll resolve the conflict for that code, this bug should be fixed" so I'll let him merge the 3 topic branches into his, make sure it is fixed, and then reassign this + #14755 to me for a final round of QA.

#11 Updated by anonym about 1 month ago

intrigeri wrote:

anonym says "actually, the way I'll resolve the conflict for that code, this bug should be fixed" so I'll let him merge the 3 topic branches into his, make sure it is fixed, and then reassign this + #14755 to me for a final round of QA.

I failed to combine commit 51b15fe237636f845c2230023a3eba393bfbf0c4 and my fix for #14755, so my plan is to merge kurono's feature/9005-Improve-tails-installer without that commit (its HEAD), postponing this ticket, but closing #14721, #14722, #14723.

#12 Updated by intrigeri about 1 month ago

  • Target version changed from Tails_3.3 to Tails_3.5

Postponing, then. I'll let you two decide who's going to work on this during next cycle.

#13 Updated by kurono about 1 month ago

  • Assignee changed from anonym to kurono

intrigeri wrote:

Postponing, then. I'll let you two decide who's going to work on this during next cycle.

I did not have enough time today to review this but I will take care for the next cycle.

#14 Updated by intrigeri about 1 month ago

I did not have enough time today to review this but I will take care for the next cycle.

Well, for now there's nothing to review: anonym didn't manage to integrate this change with his fix for #14755.

Also available in: Atom PDF