Hi,
This is an answer to blockers only. I belive they are all fixed.
On Tue, 30 Apr 2013 10:05:23 +0200 intrigeri <intrigeri@???> wrote:
> [blocker]: verify this is needed; if yes, add a comment explaining why
> (and possibly implement it in a better way); if not, remove it.
>
> > apt_get_env['LANG'] = "C"
>
> Why?
>
For logging in english, comment added in commit d5d8dd1.
> [blocker]: verify this is needed; if yes, add a comment explaining
> why; if not, remove it.
>
> > syslog.syslog("WARNING: [...]")
>
> Is the "WARNING" prefix the way one sets the importance of a given
> syslog message, with this syslog library? Else, if it's merely
> a ad-hoc way to pack this information with the actual message, then
> I suggest using whatever facility (no pun intended) this library
> provides to set the syslog message importance in a more structured
> way. Sorry, I was not in the mood of learning Python's syslog library,
> so I did not do it myself.
>
Fixed in commit f690185.
> [blocker]
>
> > syslog.syslog("WARNING: installation of %s failed" % "
> > ".join(packages))
>
> In this case, as a user, I'd like to have access to the APT
> stdout/stderr/returncode.
Stdout/stderr was already logged. Returncode added in commit 89a7ed1.
> [blocker]
>
> > if os.path.isfile(ACTIVATION_FILE):
> > return True
> > else:
> > return False
>
> Just curious, as a non-Python-programmer: wouldn't `return
> os.path.isfile(ACTIVATION_FILE)' work just as fine, and be nicer?
Fixed in commit be19887.
> [blocker]
>
> > + apt_get_returncode = _launch_apt_get(["apt-get",
> > + "--quiet",
> > + "update"])
>
> Adding `--yes' here too would be more consistent and future-proof,
> wouldn't it?
Done in commit 80cd937.
> If we go this way, perhaps the args we always want to
> pass to apt-get (`--yes' and `--quiet') could be stored in a variable
> on top of the script, and transparently added by _launch_apt_get?
>
I prefer to do that later (it will change more code and might require
more time/testing).
> [blocker]
>
> Also, what does happen in the following user story:
>
> 1. I boot Tails on January 1st
> 2. I setup persistence for APT lists and packages, and I reboot.
> 3. I activate persistence, I install a few packages, and I list them
> in live-additional-software.conf. Shutdown.
> 4. I boot Tails on February 1st
> 5. I activate persistence
> => The APT indices are expired and not valid anymore, so
> `install_additional_packages' fails, right?
> What's the APT indices expiration time exactly?
>
Each APT list contains a "Valid-Until:" field.
> At least understanding how Tails behaves in this case is a [blocker],
> as is evaluating how good/bad this is. But probably we can ship the
> whole thing as is right now, and keep further improvements in this
> area for later.
>
Testing shows that when lists are expired, the initial install fails and
I get the following output:
Reading package lists...
Building dependency tree...
Reading state information...
The following extra packages will be installed:
vim-gui-common
Suggested packages:
cscope vim-doc
The following NEW packages will be installed:
vim-gnome vim-gui-common
0 upgraded, 2 newly installed, 0 to remove and 0 not upgraded.
Need to get 0 B/1166 kB of archives.
After this operation, 2454 kB of additional disk space will be used.
WARNING: The following packages cannot be authenticated!
vim-gui-common vim-gnome
E: There are problems and -y was used without --force-yes
WARNING: installation of vim-gnome failed
Then, if there is network, the installation is successful after the
update.
This doesn't seem me that bad, even though I think that explaining to
the user what's happening in that case would be a nice bonus.
Cheers
