On Tue, 12 Mar 2013 13:23:39 +0100 intrigeri <intrigeri@???> wrote: > Hi,
>
> Alan wrote (11 Mar 2013 21:15:00 GMT) :
> > I have a working basic implementation in
> > feature/remember_installed_packages. If you want to give a hand,
> > please have a look and/or test and give feedback.
>
> Here's a first quick "static" review.
> Thanks. I just pushed a new version of the branch (warning: I rewrote
the whole history).
> > /etc/NetworkManager/dispatcher.d/70-apt-upgrade.sh
>
> This filename is misleading: it suggests the script runs apt-get
> upgrade, which it (rightly) does not. I believe
> 70-upgrade-xyz-packages.sh would be a better name.
> Renamed to 70-upgrade-additional-software.sh
> One also would have to find something sensible to put in place of xyz.
> "persistent" is no valid candidate, since not all packages stored in
> persistence are upgraded by this script -- nor installed by the other,
> by the way, so the naming issue should be addressed everywhere else in
> this branch too.
> I renamed everything to upgrade-additional-software
> > +pp_log() {
>
> Please ensure the function names are unique in a nicer way than with
> a fixed prefix... like, by using more descriptive names (e.g.
> rename `pp_install' to `install_persistent_packages').
> Well I was tired of shell and rewrite the script in python, so a lot of
comments doesn't apply anymore.
> Also, it's customary to send usage output to stderr.
> Done.
> Also, it would be great to have some meaningful exit code for the
> whole script.
> Done.
> > +++ b/config/chroot_local-patches/install-persistent-packages.diff
>
> In case you're not aware, this will have to go into tails-greeter at
> merge time. So perhaps just apply this patch to t-g right now,
> wrapped with a [ -x /usr/local/sbin/tails-persistent-packages ]
> test, so that t-g is ready for when we have this feature in the main
> Tails dev branches? (The naming issue should be addressed first, to
> minimize overhead, of course :)
> There's already a feature/remember_installed_packages branch in
tails-greeter's git, including this.
