[26941] in Source-Commits
Re: /svn/athena r25843 - in trunk/debathena/debathena: . thirdparty
daemon@ATHENA.MIT.EDU (Benjamin Kaduk)
Thu Mar 7 22:42:33 2013
Date: Thu, 7 Mar 2013 22:42:23 -0500 (EST)
From: Benjamin Kaduk <kaduk@MIT.EDU>
To: Jonathan D Reed <jdreed@MIT.EDU>
cc: source-commits@MIT.EDU
In-Reply-To: <201303072146.r27LkhwV032629@drugstore.mit.edu>
Message-ID: <alpine.GSO.1.10.1303072217080.9389@multics.mit.edu>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; format=flowed; charset=US-ASCII
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
