Project

General

Profile

Feature #14720

Feature #9005: Tails Installer without a splash screen

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

Added by anonym 12 months ago. Updated 16 days ago.

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

100%

QA Check:
Pass
Feature Branch:
bugfix/15590-14849-14720-14810-installer-fixes-for-3.9, installer:bugfix/15590-14849-14720-14810-installer-fixes-for-3.9
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.


Related issues

Blocked by Tails - Bug #14810: Tails Installer allows buggy "Reinstall (delete all data)" on too small USB sticks Resolved 10/07/2017

Associated revisions

Revision a2630100 (diff)
Added by anonym 10 months 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

Revision 48a05f21
Added by intrigeri 2 months ago

Merge branch 'bugfix/15590-14849-14720-14810-installer-fixes-for-3.9' into devel (Fix-committed: #15590, #14849, #14720, #14810)

History

#1 Updated by anonym 12 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 12 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 12 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 11 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 11 months ago

  • Assignee changed from anonym to intrigeri

#6 Updated by intrigeri 11 months 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 11 months 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 11 months 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 11 months 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 11 months 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 11 months 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 10 months 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 10 months 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 10 months 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.

#15 Updated by u 8 months ago

@anonym & @kurono: apparently you need to decide who of you two will prepare the branch so that it's mergeable, see https://labs.riseup.net/code/issues/14720#note-11 and/or anonym could merge this without the problematic commit.

#16 Updated by kurono 8 months ago

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

#17 Updated by bertagaz 6 months ago

  • Target version changed from Tails_3.6 to Tails_3.7

#18 Updated by kurono 6 months ago

  • Related to Bug #14810: Tails Installer allows buggy "Reinstall (delete all data)" on too small USB sticks added

#19 Updated by kurono 6 months ago

  • Related to Bug #14810: Tails Installer allows buggy "Reinstall (delete all data)" on too small USB sticks added

#20 Updated by kurono 6 months ago

  • Related to deleted (Bug #14810: Tails Installer allows buggy "Reinstall (delete all data)" on too small USB sticks)

#21 Updated by bertagaz 4 months ago

  • Target version changed from Tails_3.7 to Tails_3.8

#22 Updated by kurono 3 months ago

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

u wrote:

@anonym & @kurono: apparently you need to decide who of you two will prepare the branch so that it's mergeable, see https://labs.riseup.net/code/issues/14720#note-11 and/or anonym could merge this without the problematic commit.

Finally I managed to clean this mess a bit. Take a look of the branch: feature/9005-Improve-tails-installer
It passed all the the test scenarios in my local machine.

#23 Updated by intrigeri 3 months ago

  • Assignee changed from anonym to kurono

Finally I managed to clean this mess a bit. Take a look of the branch: feature/9005-Improve-tails-installer
It passed all the the test scenarios in my local machine.

Thanks, I'll look into it! Depending on the nature of the changes this might be suitable for 3.8 or 3.9, we'll see.

#24 Updated by kurono 3 months ago

  • Assignee changed from kurono to intrigeri

#25 Updated by intrigeri 3 months ago

  • Target version changed from Tails_3.8 to Tails_3.9

#26 Updated by intrigeri 3 months ago

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

Please rebase your work on top of the current upstream master branch, as opposed to the packaging (tails/master) branch.

#27 Updated by kurono 3 months ago

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

intrigeri wrote:

Please rebase your work on top of the current upstream master branch, as opposed to the packaging (tails/master) branch.

Done.

#28 Updated by intrigeri 3 months ago

Code looks good to me. Depending on the 2 other reviews I still have to do, I may test this alone or together with other proposed branches. I'll be back here shortly.

#29 Updated by intrigeri 3 months ago

I'll test in a batch with #15590 and #14810 once the latter is ready.

#30 Updated by intrigeri 3 months ago

  • Related to deleted (Bug #14810: Tails Installer allows buggy "Reinstall (delete all data)" on too small USB sticks)

#31 Updated by intrigeri 3 months ago

  • Blocked by Bug #14810: Tails Installer allows buggy "Reinstall (delete all data)" on too small USB sticks added

#32 Updated by intrigeri 2 months ago

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

#33 Updated by intrigeri 2 months ago

Merged into bugfix/15590-14849-14720-14810-installer-fixes-for-3.9 (both liveusb-creator.git and tails.git) except the logo change (quilt won't take binary stuff), built and uploaded a package.

#34 Updated by intrigeri 2 months ago

Test suite passes on Jenkins. Next step: test manually to confirm the bugs the branch is meant to fix are gone and that nothing else is broken.

#35 Updated by intrigeri 2 months ago

  • % Done changed from 60 to 90
  • QA Check changed from Ready for QA to Pass
  • Feature Branch changed from kurono:installer/feature/9005-Improve-tails-installer to bugfix/15590-14849-14720-14810-installer-fixes-for-3.9, installer:bugfix/15590-14849-14720-14810-installer-fixes-for-3.9

#36 Updated by intrigeri 2 months ago

  • Status changed from In Progress to Fix committed
  • Assignee deleted (intrigeri)
  • % Done changed from 90 to 100

Merged into devel!

#37 Updated by intrigeri 16 days ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF