[26948] in Source-Commits
Re: /svn/athena r25843 - in trunk/debathena/debathena: . thirdparty thirdparty/debian thirdparty/lists
daemon@ATHENA.MIT.EDU (Jonathan Reed)
Fri Mar 8 11:44:57 2013
Mime-Version: 1.0 (Apple Message framework v1084)
Content-Type: text/plain; charset=us-ascii
From: Jonathan Reed <jdreed@MIT.EDU>
In-Reply-To: <alpine.GSO.1.10.1303072217080.9389@multics.mit.edu>
Date: Fri, 8 Mar 2013 11:44:48 -0500
Cc: source-commits@MIT.EDU
Message-Id: <012FCB4A-6BB8-496F-9DE0-4954D08E919B@MIT.EDU>
To: Benjamin Kaduk <kaduk@MIT.EDU>
Content-Transfer-Encoding: 8bit
Issues addressed in r25845.
On Mar 7, 2013, at 10:42 PM, Benjamin Kaduk wrote:
> Seems okay, ACK (but note inline comments).
>
> On Thu, 7 Mar 2013, Jonathan D Reed wrote:
>
>> Added: trunk/debathena/debathena/thirdparty/debian/copyright
>> ===================================================================
>> --- trunk/debathena/debathena/thirdparty/debian/copyright (rev 0)
>> +++ trunk/debathena/debathena/thirdparty/debian/copyright 2013-03-07 21:46:43 UTC (rev 25843)
>> @@ -0,0 +1,31 @@
>> +This software and its Debian packaging are licensed as follows:
>> +
>> +Copyright (c) 2012, Massachusetts Institute of Technology
>
> It's 2013, now.
>
>> Added: trunk/debathena/debathena/thirdparty/debian/thirdparty
>> ===================================================================
>> --- trunk/debathena/debathena/thirdparty/debian/thirdparty (rev 0)
>> +++ trunk/debathena/debathena/thirdparty/debian/thirdparty 2013-03-07 21:46:43 UTC (rev 25843)
>> @@ -0,0 +1,123 @@
>> +#!/bin/sh
>> +#
>> +# debathena-thirdparty-installer
>
> Is this comment stale?
>
>> +
>> +LISTDIR=/var/lib/debathena-thirdparty
>> +LOG=/var/log/athena-thirdparty
>> +STATFILE=/var/lib/debathena-thirdparty/status
>> +FLAGFILE=/var/lib/debathena-thirdparty/update_required
>> +
>> +status=ok
>> +statusmsg="Nothing to do"
>> +
>> +complain() {
>> + status="error"
>> + statusmsg="$*"
>> +}
>> +
>> +whine() {
>> + status="warning"
>> + statusmsg="$*"
>> +}
>> +
>> +save_state() {
>> + rm -f $statfile
>
> $statfile is properly spelled $STATFILE .
>
>> + echo "$status|$statusmsg" > $STATFILE
>> +}
>> +
>> +save_success() {
>> + status="ok"
>> + statusmsg="$*"
>> +}
>> +
>> +finish() {
>> + echo "*Ending thirdparty installation at $(date)"
>> + echo "-----"
>> + echo
>> + rm -f /var/run/athena-nologin
>> + save_state
>> + exit
>> +}
>> +
>> +should_install() {
>> + # Removing any debathena package is not allowed.
>> + if apt-get -s install "$@" | egrep -q '^Remv debathena-'; then
>> + return 1
>> + else
>> + return 0
>> + fi
>> +}
>> +
>> +
>> +
>> +
>
> Seems a rather generous use of whitespace here.
>
>> +install() {
>> + if [ 0 != "$(id -u)" ]; then
>
> I suppose string versus numerical comparison is irrelevant for 0.
>
>> + echo "Root privileges required for installation." >&2
>> + exit 1
>> + fi
>> + touch /var/run/athena-nologin
>> + exec 3>&1
>> + exec >> $LOG 2>&1
>> + trap finish EXIT
>> + echo "** Beginning thirdparty installation at $(date)"
>> + export DEBIAN_FRONTEND=noninteractive
>> + echo "** Required package list:"
>> + cat "${LISTDIR}/dependencies"
>> + echo "** Trying monolithic transaction..."
>> + if should_install $(cat ${LISTDIR}/dependencies) &&
>> + apt-get -y install $(cat ${LISTDIR}/dependencies); then
>
> I might split this into two nested if statements. The condition for whether we try the monolithic transaction is should_install(); if that succeeds but apt-get -y install fails, that seems like a runtime error and it's unclear whether we want to try installing one-by-one in that case.
>
>> + echo "**Monolithic transaction succeeded"
>> + else
>> + echo "** Installing required packages one by one..."
>> + for pkg in $(cat "${LISTDIR}/dependencies"); do
>> + echo "** Installing $pkg..."
>> + if ! should_install $pkg; then
>> + echo "** Installation of $pkg is IMPOSSIBLE"
>> + complain "Some required packages failed to install"
>
> This puts the state in 'error' so that we notice and have to figure out what went wrong, since these are dependencies and not recommendations?
>
>> + else
>> + apt-get -y install $pkg
>> + if [ $? != 0 ]; then
>> + echo "** Installation of $pkg FAILED"
>> + complain "Some required packages failed to install"
>
> It might be nice to have the error messages be different for the two cases here?
>
>> + fi
>> + fi
>> + done
>> + fi
>> + echo "** Recommended package list:"
>> + cat "${LISTDIR}/recommendations"
>> + echo "** Installing recommended packages..."
>> + for pkg in $(cat "${LISTDIR}/recommendations"); do
>> + echo "** Installing $pkg..."
>> + if ! should_install $pkg; then
>> + echo "** Installation of $pkg is IMPOSSIBLE"
>> + whine "Some optional packages failed to install"
>> + else
>> + apt-get -y install $pkg
>> + if [ $? != 0 ]; then
>> + echo "** Installation of $pkg FAILED"
>> + whine "Some optional packages failed to install"
>
> Likewise for these notification messages.
>
>> + fi
>> + fi
>> + done
>> + rm $FLAGFILE
>
> Should this be rm -f to avoid set -e sadness?
> Hmm, or I guess we don't set -e?
>
>> + save_success "Packages installed ok"
>> +}
>> +
>> +case "$1" in
>> + install)
>> + install
>> + ;;
>> + up-to-date)
>> + [ -e "$FLAGFILE" ] && exit 1
>> + exit 0
>> + ;;
>> + *)
>> + echo "Usage: $0 [install | up-to-date]"
>> + exit 1
>> + ;;
>> +esac
>> +exit 0
>> +
>> +
>> +
>>
>>
>>
>> Added: trunk/debathena/debathena/thirdparty/generate-package-list.pl
>> ===================================================================
>> --- trunk/debathena/debathena/thirdparty/generate-package-list.pl (rev 0)
>> +++ trunk/debathena/debathena/thirdparty/generate-package-list.pl 2013-03-07 21:46:43 UTC (rev 25843)
>> @@ -0,0 +1,149 @@
>> +#!/usr/bin/perl
>> +
>> +$| = 1;
>> +use warnings;
>> +use strict;
>> +use locale; # for sort
>> +use Cwd qw(abs_path);
>> +use File::Basename;
>> +
>> +use Getopt::Std;
>> +use AptPkg::Config '$_config';
>> +use AptPkg::Cache;
>> +
>> +my $path = dirname(abs_path($0));
>> +our ($opt_n, $opt_l, $opt_d) = (0,
>> + $path . "/lists",
>> + $path);
>
> Gratuitous newlines?
>
>> +
>> +getopts('nl:d:');
>> +
>> +sub debug {
>> + if ($opt_n) {
>> + print join(' ', 'D:', @_, "\n");
>> + }
>> +}
>> +
>> +my $codename = `lsb_release -sc`;
>> +die "Can't determine codename" unless ($? == 0);
>> +chomp($codename);
>> +
>> +(-d $opt_l) || die "$opt_l is not a directory";
>> +(-d $opt_d) || die "$opt_d is not a directory";
>> +
>> +print "Using lists in $opt_l\nWriting output files to $opt_d\n";
>> +
>> +debug("Initializing APT cache");
>> +# Initialize the APT configuration
>> +$_config->init;
>> +my $cache = AptPkg::Cache->new;
>> +my $policy = $cache->policy;
>> +
>> +my %packages = ();
>> +my %depends = ();
>> +
>> +open(COMMON, join('/', $opt_l, 'common')) || die "Can't open $opt_l/common";
>> +debug("Reading 'common' file...");
>> +while (<COMMON>) {
>> + chomp;
>> + s/^\s+//;
>> + s/\s+$//;
>> + next if /^#/;
>> + next unless /\S/;
>> + if (/^-/) {
>> + warn "Ignoring invalid package exclusion in the common file, line $.";
>> + next;
>> + }
>> + if (/^(\S+) \| (\S+)$/) {
>> + debug("Examining conditional line: $_");
>> + foreach my $p ($1, $2) {
>> + debug("Checking for $p");
>> + if ($cache->{$p} && $cache->{$p}->{VersionList}) {
>> + debug("Adding $p to dependencies");
>> + $packages{$p} = 1;
>> + last;
>> + }
>> + }
>> + unless (exists($packages{$1}) || exists($packages{$2})) {
>> + warn "Could not satisfy conditional dependency: $_!";
>> + }
>> + } elsif (/^(\S+)(?: (\S+))+$/) {
>> + my ($pkg1, @rest) = (split(' ', $_));
>> + $packages{$pkg1} = 1;
>> + $depends{$pkg1} = \@rest;
>> + } elsif (/^\?(\S+)$/) {
>> + debug("Adding $1 to recommendations");
>> + $packages{$1} = 2;
>> + } else {
>> + debug("Adding $_ to dependencies");
>> + $packages{$_} = 1;
>> + }
>> +}
>> +close(COMMON);
>> +
>> +open(DIST, join('/', $opt_l, $codename)) || die "Can't open $opt_l/$codename";
>> +debug("Reading distro-specific file");
>> +while (<DIST>) {
>> + chomp;
>> + s/^\s+//;
>> + s/\s+$//;
>> + next if /^#/;
>> + next unless /\S/;
>> + if (/^-(\S+)$/) {
>> + if (exists($packages{$1})) {
>> + debug("Deleting $1 from package list.");
>> + delete($packages{$1});
>> + } else {
>> + warn("Package $1 is not in package list, so can't remove it.");
>> + }
>> + } elsif (/^\?(\S+)$/) {
>> + debug("Adding $1 to recommendations");
>> + $packages{$1} = 2;
>> + } else {
>> + debug("Adding $_ to dependencies");
>> + $packages{$_} = 1;
>> + }
>> +}
>> +close(DIST);
>> +
>> +foreach my $pkgname (sort(keys(%packages))) {
>> + my $pkg = $cache->{$pkgname};
>> + if (! $pkg) {
>> + debug("Removing $pkgname as it can't be found in the APT cache.");
>> + delete($packages{$pkgname});
>> + if (exists($depends{$pkgname})) {
>> + foreach (@{$depends{$pkgname}}) {
>> + debug("Removing $_ because we removed $pkgname");
>> + delete($packages{$_});
>> + }
>> + }
>> + next;
>> + }
>> + if (! $pkg->{VersionList}) {
>> + debug("Removing $pkgname as it has no version candidate");
>> + delete($packages{$pkgname});
>> + if (exists($depends{$pkgname})) {
>> + foreach (@{$depends{$pkgname}}) {
>> + debug("Removing $_ because we removed $pkgname");
>> + delete($packages{$_});
>> + }
>> + }
>> + next;
>> + }
>> +}
>> +
>> +debug("Writing out lists");
>> +open(DEPS, '>', join('/', $opt_d, 'dependencies')) || die "Can't open $opt_d/dependencies for writing";
>> +open(RECS, '>', join('/', $opt_d, 'recommendations')) || die "Can't open $opt_d/dependencies for writing";
>> +foreach (sort(keys(%packages))) {
>> + if ($packages{$_} == 2) {
>> + print RECS "$_\n";
>> + } else {
>> + print DEPS "$_\n";
>> + }
>> +}
>> +close(DEPS);
>> +close(RECS);
>> +print "Done.\n";
>> +exit 0;
>> +
>
> My perl is not so great, so I can't review this particularly well, but it looks okay from what I can see.
>
>
> -Ben
