Project

General

Profile

Bug #14810

Feature #9005: Tails Installer without a splash screen

Tails Installer allows buggy "Reinstall (delete all data)" on too small USB sticks

Added by intrigeri 12 months ago. Updated 18 days ago.

Status:
Resolved
Priority:
Elevated
Assignee:
-
Category:
Installation
Target version:
Start date:
10/07/2017
Due date:
% Done:

100%

QA Check:
Pass
Feature Branch:
bug/14810-buggy-reinstall
Type of work:
Code
Blueprint:
Starter:
Affected tool:
Installer

Description

I have a (virtual) 4.3GB USB stick with an older Tails installed. I start Tails/Buster that has the newer Installer, it detects that my destination device has Tails installed already to the default option is "Upgrade". So far, so good. But it also lets me choose "Reinstall (delete all data)", which does not work: it is stuck at the partitioning step (iirc). I guess we should hide that button on too small USB sticks and only allow upgrading.

(I suspect that's the fallout from an invisible merge conflict between kurono's branch and mine, i.e. Git can't notice things conflict because different code is modified on both branches, but the result of the merge does not work as it should.)


Related issues

Related to Tails - Bug #14958: Tails Installer 5.0 proposes to upgrade even if the destination stick is already up-to-date Confirmed 11/11/2017
Blocks Tails - Bug #15590: Installer reinstall option displays incorrect stick capacity Resolved 05/07/2018
Blocks Tails - Feature #14720: Fix self.opts.partition vs. self.force_reinstall semantics Resolved 09/25/2017

Associated revisions

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 intrigeri 11 months ago

  • Assignee changed from anonym to kurono
  • Priority changed from Normal to Elevated

This is a regression => bumping priority.

kurono, perhaps you would like to work on this? Ideally it should be done for 3.3 but don't worry too much: let's first focus on polishing (if needed) and merging the work you've already submitted for QA; this one can wait a bit more.

#2 Updated by kurono 11 months ago

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

#3 Updated by u 8 months ago

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

Possibly too late for 3.5, postponing.

#4 Updated by u 8 months ago

  • Related to Bug #14958: Tails Installer 5.0 proposes to upgrade even if the destination stick is already up-to-date added

#5 Updated by intrigeri 7 months ago

Let's aim at fixing this in time for Tails 3.7 i.e. a good, well-tested fix shall be merged by the end of April. Taking into account some time for reviewing and acting upon the review, I think an initial branch shall be proposed by April 15. kurono, would this work for you? If not, please reassign to anonym and I'll discuss the next steps with him. Thanks!

#6 Updated by bertagaz 6 months ago

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

#7 Updated by kurono 6 months ago

  • Related to Feature #14720: Fix self.opts.partition vs. self.force_reinstall semantics added

#8 Updated by kurono 6 months ago

  • Related to Feature #14720: Fix self.opts.partition vs. self.force_reinstall semantics added

#9 Updated by kurono 6 months ago

  • Related to deleted (Feature #14720: Fix self.opts.partition vs. self.force_reinstall semantics)

#10 Updated by bertagaz 5 months ago

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

#11 Updated by kurono 3 months ago

  • Status changed from Confirmed to In Progress
  • Assignee changed from kurono to intrigeri
  • QA Check set to Ready for QA
  • Feature Branch set to bug/14810-buggy-reinstall

intrigeri wrote:

Let's aim at fixing this in time for Tails 3.7 i.e. a good, well-tested fix shall be merged by the end of April. Taking into account some time for reviewing and acting upon the review, I think an initial branch shall be proposed by April 15. kurono, would this work for you? If not, please reassign to anonym and I'll discuss the next steps with him. Thanks!

Sorry for the late answer. I have added a fix for this problem. I have also made a related small cleaning of the code. It passes all the test cases in my machine. Looking forward for your comments.

#12 Updated by intrigeri 3 months ago

  • Assignee changed from intrigeri to kurono
  • % Done changed from 0 to 10
  • QA Check changed from Ready for QA to Dev Needed

Great to see you're on it! Please rebase your work on top of the upstream branch i.e. master.

#13 Updated by kurono 3 months ago

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

intrigeri wrote:

Great to see you're on it! Please rebase your work on top of the upstream branch i.e. master.

Done :)

#14 Updated by intrigeri 3 months ago

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

#15 Updated by intrigeri 3 months ago

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

kurono wrote:

intrigeri wrote:

Great to see you're on it! Please rebase your work on top of the upstream branch i.e. master.

Done :)

Well, if you did it you forgot to (force-)push:

git diff --stat origin/master...kurono/bug/14810-buggy-reinstall
 debian/README.source                     |  266 +++++++++
 debian/changelog                         | 1344 ++++++++++++++++++++++++++++++++++++++++++
 debian/compat                            |    1 +
 debian/control                           |   48 ++
 debian/copyright                         |   44 ++
 debian/gbp.conf                          |    5 +
 debian/rules                             |   20 +
 debian/source/format                     |    1 +
 debian/source/lintian-overrides          |    6 +
 debian/tails-installer.lintian-overrides |    2 +
 debian/tails-installer.manpages          |    1 +
 tails_installer/creator.py               |   17 +-
 tails_installer/gui.py                   |    8 +-
 tools/7-Zip-License.txt                  |   79 ---
 tools/7z.dll                             |  Bin 601088 -> 0 bytes
 tools/7z.exe                             |  Bin 141824 -> 0 bytes
 tools/dd.exe                             |  Bin 342016 -> 0 bytes
 tools/syslinux.exe                       |  Bin 71168 -> 0 bytes
 18 files changed, 1753 insertions(+), 89 deletions(-)

#16 Updated by kurono 3 months ago

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

intrigeri wrote:

kurono wrote:

intrigeri wrote:

Great to see you're on it! Please rebase your work on top of the upstream branch i.e. master.

Done :)

Well, if you did it you forgot to (force-)push:

[...]

ahh sorry my local master branch was a mess, I have made a new commit for this ticket:

git diff --stat origin/master...kurono/bug/14810-buggy-reinstall
tails_installer/creator.py | 17 +++++++++------
tails_installer/gui.py | 8 +++----
2 files changed, 15 insertions(
), 10 deletions(-)

#17 Updated by intrigeri 3 months ago

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

First code review:

  • I kind of liked doing self.__button_force_reinstall.set_visible(False) before we check if reinstalling makes sense, and then once that done, possibly doing self.__button_force_reinstall.set_visible(True). Is there a reason why you changed this, as opposed to just wrapping the latter with if device['is_device_big_enough_for_reinstall']?
  • Without testing, at first glance it looks like previously we would only display the reinstall button when the target device already has Tails. It looks like we'll now display it all the time because you removed if not self.opts.partition. Am I guessing right? Feel free to tell me "you're wrong, just test it" :)

Otherwise, looks good to me. Once we've clarified these 2 things I'll test and hopefully merge.

#18 Updated by intrigeri 3 months ago

  • Blocks Bug #15590: Installer reinstall option displays incorrect stick capacity added

#19 Updated by intrigeri 3 months ago

  • Related to deleted (Feature #14720: Fix self.opts.partition vs. self.force_reinstall semantics)

#20 Updated by intrigeri 3 months ago

  • Blocks Feature #14720: Fix self.opts.partition vs. self.force_reinstall semantics added

#21 Updated by kurono 3 months ago

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

intrigeri wrote:

First code review:

  • I kind of liked doing self.__button_force_reinstall.set_visible(False) before we check if reinstalling makes sense, and then once that done, possibly doing self.__button_force_reinstall.set_visible(True). Is there a reason why you changed this, as opposed to just wrapping the latter with if device['is_device_big_enough_for_reinstall']?

I think that is not longer necessary, because self.__button_force_reinstall.set_visible is false by default and it only becomes true when device['is_device_big_enough_for_reinstall']. In any other case it should remain false.

  • Without testing, at first glance it looks like previously we would only display the reinstall button when the target device already has Tails. It looks like we'll now display it all the time because you removed if not self.opts.partition. Am I guessing right? Feel free to tell me "you're wrong, just test it" :)

I have removed both statements if not self.opts.partition and self.__button_force_reinstall.set_visible(True) so it should not get visible there.

Otherwise, looks good to me. Once we've clarified these 2 things I'll test and hopefully merge.

I have run all the test cases again locally and they passed. I think it needs a new test case though :)

#22 Updated by intrigeri 3 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.

#23 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.

#24 Updated by intrigeri 2 months ago

  • % Done changed from 10 to 90
  • QA Check changed from Ready for QA to Pass

I confirm the problem I've reported is gone :)

#25 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!

#26 Updated by intrigeri 18 days ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF