Hi,
Alan wrote (01 May 2013 18:36:11 GMT) :
> On Tue, 30 Apr 2013 10:05:23 +0200 intrigeri <intrigeri@???> wrote:
>> > 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?
Yes, maybe. I seem to remember I might have advised you to go look for
inspiration in tails-greeter. The bad news is that t-g also mistakenly
uses /var/lib for session runtime data. Anyway..
>> > 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?
Well... I now see what you mean, but I happen to dislike it.
Having behaviour consistent between "there was nothing in the config
file" and "there was something in the config file" (at the time that
I expect it to be read and used) does not look like a worthy goal to
me. Rather the contrary.
As a potential user (and a very happy one that you're working on it!),
the way I understand it, this option/feature is about having stuff
automatically installed *at login time* (and possibly upgraded
later on, I don't care).
So, having packages installation triggered by a network disconnect /
reconnect would be useless and possibly painful behaviour to me:
1. I can't really rely on it because I guess it'll be undocumented
bonus behaviour that we probably won't test at release time,
and may be broken at any time.
2. Once the notify message is fixed, the feedback I have once the
stuff is effectively installed will tell me about an *upgrade*
having succeeded, and I certainly don't expect this to mean
some additional packages were *installed*.
3. So, I won't take any benefit from it, while it will eat system
resources behind my back, in a way that will be hard
to understand.
What do you, and others, think?
>> > 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.
OK. I'm surprised NM gives an environment that lacks one of the
needed components, since we've never seen this before in our other
hooks. E.g. 60-ttdnsd.sh uses `service', that's in /usr/sbin.
So, I wonder which directory is missing.
Are you sure you did not rather experience this behaviour outside of
the NM dispatcher, e.g. by running the script by hand in some shell
environment that was lacking the sbin components in $PATH?
>> [blocker]: verify this is needed; if yes, add a comment explaining why
>> (and possibly implement it in a better way); if not, remove it.
Still true, IMHO. Sorry to be hard to convince.
>> > 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).
OK.
>> [blocker]: verify this is needed; if yes, add a comment explaining
>> why; if not, remove it.
Please do this, then.
>> > 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, "[...]")
Thanks for checking. I'm glad this is possible.
> 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?
I don't care much about removing the "WARNING" prefix (well, full caps
seems a bit heavy to me, and unusual when I look at my own syslog --
why not "Warning:" if you want to keep it?), but I do care about using
the proper data structures syslog is offering us, that is the message
importance. (Rationale: not using the well-known syslog message
structured format is a code-smell; it's no big deal in practice in
Tails, but I dislike adding such code-smells to our codebase at this
stage of the project, as it encourages poor code quality for other
future contributions.)
>> [todo-later]
Still true IMHO. Needs to be written down somewhere, and needs someone
who volunteers to do it in time for 0.18.1.
>> > 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.
OK. Please write it down on the ticket, then.
>> > 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?
Yes, dying early and hard in unexpected situations seems good to me.
I was looking for places where it was not obvious, to a non Python
programmer, that the error checking was actually done. But perhaps the
Python standard library has sane enough defaults that make it
useless :)
>> > 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?
No, logging it *somewhere*, e.g. in syslog, as the rest of the
technical low-level logs output by this program, would be totally fine
with me :)
>> [todo-later]
=> TODO++
>> > 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.
> ???
What I meant is "same here": I'd like to have access to the APT
stdout/stderr/returncode.
From The Collaborative International Dictionary of English v.0.48 [gcide]:
Ditto \Dit"to\, adv.
As before, or aforesaid; in the same manner; also.
[1913 Webster]
I suggest `apt-get install dict-gcide' for offline usage.
>> [todo-later]
=> TODO++
>> > 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]
> Yes.
OK. I'll let you fix it, then.
>> > 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.
Then I'll let you change it, then, OK?
(Code-smells bla, again :)
>> > os.makedirs(activation_file_dir)
>>
>> Does this throws an exception on failure?
> Yes, OSError.
Thanks for checking.
>> > + 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.
The bonus would be that:
0. I'm not sure no `apt-get update' corner case requires user input.
I'd rather not read the source to check, and I bet you don't either.
1. Passing --yes makes it so this program will just go on working if
`apt-get update' requires user confirmation some day, in some
corner cases, for some reason.
2. Passing --yes expresses the needs and intent of your code.
This code is meant to be run non-interactively, so it can't run
commands that may need user input, so it has to tell this to
apt-get:
--yes
Automatic yes to prompts; assume "yes" as answer to all prompts
and run non-interactively.
Relying on the fact `apt-get update' currently might never
require user input looks like relying on pure luck to me.
I'd rather not see Tails rely on such unspecified behaviour.
Makes sense?
>> [todo-later] (but trivial, so just doing it is probably easier than
>> creating a ticket)
Still the case IMHO.
>> > ++# 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
=> TODO++, then.
>> > +++
>> > 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]
OK.
>> 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.
I guess so. Which means the automatic install won't work offline.
If your tests confirm it, perhaps it would be worth a note in the
end-user documentation -- what do you think?
> But I add the test to my todo list.
OK, excellent!
Cheers,
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
