Hi,
Alan wrote (21 Apr 2013 01:13:39 GMT) :
> I belive the 1st step of additional software installation is ready,
> and hope somebody will have time to review it for 0.18.
Yeah, we're getting closer :)
I've pushed some fixes and improvements on top of the topic branch,
when the change looked trivial to me: I wanted to save some
industrious back'n'forth. However, I had no time to test my commits
yet, sorry about that. Hopefully the time/energy we save by having me
do the changes rather than writting comments is greater than the time
it will take someone to detect and fix my mistakes...
Here's a quick review.
The freeze is pretty close, so I'll tag my comments below this way, so
that our efforts can be better targetted:
- [blocker]: IMHO, this should be fixed before we merge the branch
- [todo-later]: IMHO, before we merge the branch, we should agree
upon what must be done later, and someone should commit to have it
done in time for 0.18.1
- [bonus]: would be nice to improve, but no big deal right now
> Activation_FILE="/var/lib/live-additional-software/activated"
My understanding of the FHS makes me think this would be better suited
for /var/run. I went ahead and changed it.
> set_activated()
> packages = get_additional_packages()
> if not packages:
> syslog.syslog("WARNING: no packages to install, exiting")
> return True
How about moving the call to set_activated() after checking if there
are packages to install? I assume we don't want to later "upgrade"
software that we did not install on the first place, do we?
(I mean, we're lucky the "upgrade" operation, in the way it's
currently implemented, installs software when missing, but I'd rather
not silently depend on this weird behaviour.)
[todo-later]
> apt_get_env['PATH'] = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
What's the intent?
[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?
[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.
[todo-later]
> try:
> subprocess.check_call(["/usr/local/sbin/tails-notify-user", title, body])
> except:
> syslog.syslog("WARNING: unable to notify the user. The notification was: %s %s" % (title, body))
I guess the only use of the exception handler is to help us developers
debug things when they go wrong. In this case, perhaps logging
stdout/stderr/returncode of the tails-notify-user call would
be useful?
[todo-later]
> with open(package_list) as f:
> for line in f:
Does this throw an exception on file read errors?
[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.
[todo-later]
> syslog.syslog("WARNING: the update failed.")
> _notify(_("Your additional software"),
> _("The upgrade failed. This might be due to a network problem. Please check your network connexion or try to restart Tails."))
Ditto.
[todo-later]
> if install_additional_packages():
> _notify(_("Your additional software"),
> _("The installation was successful."))
I may have missed some of the logics,
but shouldn't this read "The upgrade was successful" instead?
[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?
[bonus]
> os.makedirs(activation_file_dir)
Does this throws an exception on failure?
[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? 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?
[todo-later] (but trivial, so just doing it is probably easier than
creating a ticket)
> ++# install persistent packages
> ++if [ -x /usr/local/sbin/tails-additional-software ] ; then
> ++ /usr/local/sbin/tails-additional-software install
> ++fi
I guess this was great at devel time (and I may even have suggested
it, so that t-g works both with Tails images that have this branch
merged, and with those that haven't), but perhaps we'll want to make
t-g fail hard if the needed program can't be found, once the branch is
merged? Please TODO++ if you agree :)
> +++ b/config/chroot_local-includes/etc/NetworkManager/dispatcher.d/70-upgrade-addition
> al-software.sh
> @@ -0,0 +1,15 @@
> +#!/bin/sh
> +
> +set -e
How does NM react when one of the dispatcher hooks fails? Does it run
the next ones? If it does not, then I doubt we want a failure of
70-upgrade-* to prevent running the next ones, so perhaps we want to
catch the hook's exit code so that the whole process does not abort.
[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?
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.
Cheers,
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
