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,
