Re: [Tails-dev] remembering installed packages (from TODO)

Delete this message

Reply to this message
Autore: sajolida
Data:  
To: The Tails public development discussion list
Oggetto: Re: [Tails-dev] remembering installed packages (from TODO)
On 12/03/13 13:23, 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.
>
>> /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.
>
> 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.
>
>> +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').
>
>> +       # Log arguments to the logfile defined in $PP_LOGFILE

>
> Please document the function's purpose in a way that can't be confused
> with documenting the next shell command, e.g. by moving the function
> doc just before the function (same for most other functions, not just
> this one.)
>
>> +       echo "$@" >> "$PP_LOGFILE"

>
> Please use syslog instead. See 20-time.sh for an example.
>
>> +pp_notify_user() {
>
> Please extract this function from 20-time.sh into
> usr/local/lib/tails-shell-library/$WHATEVER.sh, instead of duplicating
> the code.
>
>> +       # Shows the paths of persistent packageslists
>> [...]
>> +       # Shows the list of all the persistent packages

>
> "Shows" is a bit vague. "Print on standard output", perhaps?
>
> The aforementioned naming issue is obvious in "all the persistent
> packages".
>
>> +               echo ${persistent_dir}${PP_PACKAGES_LIST_FILE}

>
> Any good reason not to quote the argument to echo?
>
>> +       for package_list in $(pp_get_packages_lists); do
>> +               cat "${package_list}"
>> +       done

>
> "cat $(pp_get_packages_lists)" would be enough.
>
>> +pp_install_packages() {
>> +       # Install packages given as arguments
>> +       local package
>> +       for package in ${@}; do

>
> I think running apt-get once for all remembered packages would be more
> efficient and more robust: it gives APT the big picture of what we
> want, instead of asking it to paint it blindly bit by bit.
>
>> +       for package in ${@}; do

>
> Using ${} in this construct seems overkill to me, but I'd be happy to
> learn if it brings advantages I don't know about.
>
>> +                apt-get --quiet --no-remove --yes install
>>                  "${package}" 2>&1

>
> I think you want -o 'DPkg::Options::=--force-confold' too.
>
>>>> $PP_LOGFILE || \
>
> Well, no, please use your logging function instead of gratuitously
> breaking abstraction layers you've setup.
>
>> +       pp_install_packages "${packages}"

>
> Why pass the packages list as a single argument, while the
> pp_install_packages documentation reads "Install packages given as
> arguments"?
>         ^

>
> Actually, I'm not sure pp_install_packages is worth existing at all as
> a separate function, but well.
>
>> +       # The command which installs all persistent packages
>> [...]
>> +        # The command which upgrades all persistent packages

>
> Same naming problem, again.
>
>> +       echo "Will install the following packages: ${packages}"

>
> Why stdout instead of logging?
>
>> +        pp_notify_user "`gettext 'Tails persistence'`" "`gettext 'Tails is starting to upgrade persistent software.'`"

>
> I believe gettext was not initialized yet, so this has little chances
> to work. I'll be happy to review a further, tested version.
>
> Also, I'm not convinced about all the strings displayed to the user,
> and I'd like sajolida to have a look at it and suggest improvements.


Yesterday, we proposed to use the term "your additional software". So
that could become:

« Your additional software
Upgrading and installing your additional software... »

We're already using the gerund form in the notification of the time
synchronization, so let's stick to that.

« Your additional software
The upgrade was successful. »

« Your additional software
The upgrade failed. [Hint] »

Whenever the upgrade fails we should provide some more information to
the user, that could include the following, maybe using a link to the doc:

- more info or hints about why it failed
- hints about what else could be tried to do the upgrade: check the
network connection? restart?

But I wouldn't like to be left with just a failure message...

>> +        apt-get --quiet update 2>&1 >> $PP_LOGFILE && \

>
> log function, here too.
>
>> +        pp_notify_user "`gettext 'Tails persistence'`" "`gettext
>>          'Tails failed to upgrade persistent software.'`"

>
> Being given some clues about the actual issue (either displaying
> error messages, or pointing to the log file) would be helpful.
>
> In pp_upgrade, I'm not sure about what kind of user experience the
> sequence of desktop notifications and their timeouts create.
> Generally, I think I like it more when the first ("starting")
> notification stays displayed until it's replaced by the
> "finished" one.
>
>> +       # The command which shows help

>
> This is not exactly a command. s/shows/displays.
>
>> +       echo "Usage: $0 [command]"

>
> I don't think command is really optional here.
>
> Also, it's customary to send usage output to stderr.
>
> Also, it would be great to have some meaningful exit code for the
> whole script.
>
>> +++ 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 :)
>
> Cheers,