Re: [Tails-dev] Please review and merge feature/remember_in…

Delete this message

Reply to this message
Autor: Alan
Data:  
Dla: tails-dev
Temat: Re: [Tails-dev] Please review and merge feature/remember_installed_packages
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