Hi,
On Tue, 30 Apr 2013 10:05:23 +0200 intrigeri <intrigeri@???> wrote:
> 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.
>
Thanks for reviewing this
> 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.
>
I think so too, but last time I asked somebody (you?) told me to put
this file in /var/lib. Perhaps a misunderstanding on the usage of the
file?
> > 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.)
>
I implemented it like that on purpose: if you call install (which is
the intended behaviour of upconing the "activate additional software
packages" button in Tails-greeter) and there is a configuration file
in the persistence, then additional software packages are activated
(even if the file is empty). I choosed that so that if you modify the
configuration file and reconnects the network later on, then the
behaviour is coherent with what would happen if the file was not empty
(the new additional software are installed). Makes sense?
> [todo-later]
>
> > apt_get_env['PATH'] =
> > "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
>
> What's the intent?
>
Otherwise apt-get fails because some of the commands it calls is
missing from the environment NetworkManager uses to execute its hooks.
> [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?
>
So that the logs are in english, and easier for us to read when we
recieve a bugreport (otherwise the logged output of apt-get is
localized).
> [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?
No, I could call:
syslog.syslog(syslog.LOG_WARNING, "[...]")
But that wouldn't print "WARNING:" in the logs. This might however not
be an issue.
> 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.
>
Would you like me to transform it in order to remove "WARNING:" from
the message?
> [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?
Yes, to do later.
> [todo-later]
>
> > with open(package_list) as f:
> > for line in f:
>
> Does this throw an exception on file read errors?
Yes, and the script would exit. Do you think it is good?
> [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.
Do you mean logging this information in a file readeable by the
amnesia user?
> [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?
Yes.
> [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?
You're right.
> [bonus]
>
> > os.makedirs(activation_file_dir)
>
> Does this throws an exception on failure?
Yes, OSError.
> [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?
>
Why not, but what would be the bonus? I don't see why we want to pass
an option that is not required.
> [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 :)
>
Agreed
> > +++
> > 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.
No idea, todo++
> [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.
>
I imagine it fails to install, then when network pops up, the upgrade
succeeds. But I add the test to my todo list.
Cheers
