Support for proper package upgrade.
authorDanek Duvall <danek.duvall@sun.com>
Mon, 13 Aug 2007 14:48:07 -0700
changeset 72 9deed41412b3
parent 71 d1c84919bea6
child 73 5318fc489305
Support for proper package upgrade.
src/modules/actions/attribute.py
src/modules/actions/directory.py
src/modules/actions/driver.py
src/modules/actions/file.py
src/modules/actions/generic.py
src/modules/actions/link.py
src/modules/client/image.py
src/modules/client/pkgplan.py
src/modules/manifest.py
src/modules/server/transaction.py
src/tests/simple-upgrade.ksh
--- a/src/modules/actions/attribute.py	Mon Aug 13 14:13:33 2007 -0700
+++ b/src/modules/actions/attribute.py	Mon Aug 13 14:48:07 2007 -0700
@@ -41,3 +41,10 @@
 
         def __init__(self, data=None, **attrs):
                 generic.Action.__init__(self, data, **attrs)
+
+                # XXX This is pretty hokey.  Is this okay, do we get rid of it
+                # in favor of just doing name/value attributes to start with, or
+                # do we find a better solution for upgrade?
+                if len(attrs) == 1:
+                        self.key_attr = "name"
+                        self.attrs["name"] = attrs.keys()[0]
--- a/src/modules/actions/directory.py	Mon Aug 13 14:13:33 2007 -0700
+++ b/src/modules/actions/directory.py	Mon Aug 13 14:48:07 2007 -0700
@@ -42,44 +42,46 @@
 
         name = "dir"
         attributes = ("mode", "owner", "group", "path")
+        key_attr = "path"
 
         def __init__(self, data=None, **attrs):
                 generic.Action.__init__(self, data, **attrs)
 
-        def preinstall(self, image):
-                """Client-side method that performs pre-install actions."""
-                pass
-
-        def install(self, image):
+        def install(self, image, orig):
                 """Client-side method that installs a directory."""
                 path = self.attrs["path"]
                 mode = int(self.attrs["mode"], 8)
                 owner = pwd.getpwnam(self.attrs["owner"]).pw_uid
                 group = grp.getgrnam(self.attrs["group"]).gr_gid
 
+                if orig:
+                        omode = int(orig.attrs["mode"], 8)
+                        oowner = pwd.getpwnam(orig.attrs["owner"]).pw_uid
+                        ogroup = grp.getgrnam(orig.attrs["group"]).gr_gid
+
                 path = os.path.normpath(os.path.sep.join(
                     (image.get_root(), path)))
-                try:
-                        os.mkdir(path, mode)
-                except OSError, e:
-                        if e.errno != errno.EEXIST:
-                                raise
 
-                try:
-                        os.chown(path, owner, group)
-                except OSError, e:
-                        if e.errno != errno.EPERM:
-                                raise
+                if not orig:
+                        try:
+                                os.mkdir(path, mode)
+                        except OSError, e:
+                                if e.errno != errno.EEXIST:
+                                        raise
+                elif mode != omode:
+                        os.chmod(path, mode)
 
-        def postinstall(self):
-                """Client-side method that performs post-install actions."""
-                pass
+                if not orig or oowner != owner or ogroup != group:
+                        try:
+                                os.chown(path, owner, group)
+                        except OSError, e:
+                                if e.errno != errno.EPERM:
+                                        raise
 
         def remove(self, image):
                 path = os.path.normpath(os.path.sep.join(
                     (image.get_root(), self.attrs["path"])))
 
-                print "removing directory:", path
                 try:
                         os.rmdir(path)
                 except OSError, e:
--- a/src/modules/actions/driver.py	Mon Aug 13 14:13:33 2007 -0700
+++ b/src/modules/actions/driver.py	Mon Aug 13 14:48:07 2007 -0700
@@ -42,6 +42,7 @@
 
         name = "driver"
         attributes = ("name", "alias", "class", "perms", "policy", "privs")
+        key_attr = "name"
 
         # XXX This is a gross hack to let us test the action without having to
         # be root.
@@ -59,7 +60,7 @@
         def __init__(self, data=None, **attrs):
                 generic.Action.__init__(self, data, **attrs)
 
-        def install(self, image):
+        def install(self, image, orig):
                 n2m = os.path.normpath(os.path.sep.join(
                     (image.get_root(), "etc/name_to_major")))
 
@@ -70,8 +71,15 @@
                     if line.split()[0] == self.attrs["name"]
                 ]
 
-                if major:
-                        return update_install(self, image)
+                # In the case where the the packaging system thinks the driver
+                # is installed and the driver database doesn't or vice versa,
+                # complain.
+                if (major and not orig) or (orig and not major):
+                        print "packaging system and driver database disagree", \
+                            "on whether '%s' is installed" % self.attrs["name"]
+
+                if orig:
+                        return self.update_install(image, orig)
 
                 args = ( self.add_drv, "-n", "-b", image.get_root() )
                 if "alias" in self.attrs:
@@ -98,12 +106,99 @@
 
                 retcode = subprocess.call(args)
                 if retcode != 0:
-                        print "%s (%s) action failed with return code %s" % \
+                        print "%s (%s) install failed with return code %s" % \
                             (self.name, self.attrs["name"], retcode)
 
-        def update_install(self, image):
-                # XXX This needs to run update_drv or something.
-                pass
+        def update_install(self, image, orig):
+                add_args = ( self.update_drv, "-b", image.get_root(), "-a" )
+                rem_args = ( self.update_drv, "-b", image.get_root(), "-d" )
+
+                nalias = self.attrs.get("alias", [])
+                oalias = orig.attrs.get("alias", [])
+                # If there's only one alias, we'll get a string back unenclosed
+                # in a list, so we need enlist it.
+                if isinstance(nalias, str):
+                        nalias = [ nalias ]
+                if isinstance(oalias, str):
+                        oalias = [ oalias ]
+                add_alias = set(nalias) - set(oalias)
+                rem_alias = set(oalias) - set(nalias)
+
+                if add_alias:
+                        add_args += (
+                            "-i",
+                            " ".join([ '"%s"' % x for x in add_alias ])
+                        )
+                if rem_alias:
+                        rem_args += (
+                            "-i",
+                            " ".join([ '"%s"' % x for x in rem_alias ])
+                        )
+
+                nperms = self.attrs.get("perms", [])
+                operms = orig.attrs.get("perms", [])
+                if isinstance(nperms, str):
+                        nperms = [ nperms ]
+                if isinstance(operms, str):
+                        operms = [ operms ]
+                add_perms = set(nperms) - set(operms)
+                rem_perms = set(operms) - set(nperms)
+
+                if add_perms:
+                        add_args += ( "-m", ",".join(add_perms) )
+                if rem_perms:
+                        rem_args += ( "-m", ",".join(rem_perms) )
+
+                nprivs = self.attrs.get("privs", [])
+                oprivs = orig.attrs.get("privs", [])
+                if isinstance(nprivs, str):
+                        nprivs = [ nprivs ]
+                if isinstance(oprivs, str):
+                        oprivs = [ oprivs ]
+                add_privs = set(nprivs) - set(oprivs)
+                rem_privs = set(oprivs) - set(nprivs)
+
+                if add_privs:
+                        add_args += ( "-P", ",".join(add_privs) )
+                if rem_perms:
+                        rem_args += ( "-P", ",".join(rem_privs) )
+
+                npolicy = self.attrs.get("policy", "")
+                opolicy = orig.attrs.get("policy", "")
+                if npolicy:
+                        add_args += ( "-p", npolicy )
+                if opolicy:
+                        rem_args += ( "-p", opolicy )
+
+                add_args += (self.attrs["name"], )
+                rem_args += (self.attrs["name"], )
+
+                if len(add_args) > 5:
+                        retcode = subprocess.call(add_args)
+                        if retcode != 0:
+                                print "%s (%s) upgrade (add) failed with " \
+                                    "return code %s" % \
+                                    (self.name, self.attrs["name"], retcode)
+
+                if len(rem_args) > 5:
+                        retcode = subprocess.call(rem_args)
+                        if retcode != 0:
+                                print "%s (%s) upgrade (remove) failed with " \
+                                    "return code %s" % \
+                                    (self.name, self.attrs["name"], retcode)
+
+        def remove(self, image):
+                args = (
+                    self.rem_drv,
+                    "-b",
+                    image.get_root(),
+                    self.attrs["name"]
+                )
+
+                retcode = subprocess.call(args)
+                if retcode != 0:
+                        print "%s (%s) removal failed with return code %s" % \
+                            (self.name, self.attrs["name"], retcode)
 
         def generate_indices(self):
                 return {
--- a/src/modules/actions/file.py	Mon Aug 13 14:13:33 2007 -0700
+++ b/src/modules/actions/file.py	Mon Aug 13 14:48:07 2007 -0700
@@ -42,54 +42,63 @@
 
         name = "file"
         attributes = ("mode", "owner", "group", "path")
+        key_attr = "path"
 
         def __init__(self, data=None, **attrs):
                 generic.Action.__init__(self, data, **attrs)
                 self.hash = "NOHASH"
 
-        def preinstall(self, image):
-                """Client-side method that performs pre-install actions."""
-                pass
-
-        def install(self, image):
+        def install(self, image, orig):
                 """Client-side method that installs a file."""
                 path = self.attrs["path"]
                 mode = int(self.attrs["mode"], 8)
                 owner = pwd.getpwnam(self.attrs["owner"]).pw_uid
                 group = grp.getgrnam(self.attrs["group"]).gr_gid
 
-                temp = os.path.normpath(os.path.sep.join(
-                    (image.get_root(), path + "." + self.hash)))
-                path = os.path.normpath(os.path.sep.join(
+                final_path = os.path.normpath(os.path.sep.join(
                     (image.get_root(), path)))
 
-                stream = self.data()
-                tfile = file(temp, "wb")
-                shasum = generic.gunzip_from_stream(stream, tfile)
+                # If we're upgrading, extract the attributes from the old file.
+                if orig:
+                        omode = int(orig.attrs["mode"], 8)
+                        oowner = pwd.getpwnam(orig.attrs["owner"]).pw_uid
+                        ogroup = grp.getgrnam(orig.attrs["group"]).gr_gid
 
-                tfile.close()
-                stream.close()
+                # If we're not upgrading, or the file contents have changed,
+                # retrieve the file and write it to a temporary location.
+                if not orig or orig.hash != self.hash:
+                        temp = os.path.normpath(os.path.sep.join(
+                            (image.get_root(), path + "." + self.hash)))
 
-                # XXX Should throw an exception if shasum doesn't match self.hash
+                        stream = self.data()
+                        tfile = file(temp, "wb")
+                        shasum = generic.gunzip_from_stream(stream, tfile)
 
-                os.chmod(temp, mode)
-                try:
-                        os.chown(temp, owner, group)
-                except OSError, e:
-                        if e.errno != errno.EPERM:
-                                raise
+                        tfile.close()
+                        stream.close()
+
+                        # XXX Should throw an exception if shasum doesn't match
+                        # self.hash
+                else:
+                        temp = final_path
 
-                os.rename(temp, path)
+                if not orig or omode != mode:
+                        os.chmod(temp, mode)
 
-        def postinstall(self):
-                """Client-side method that performs post-install actions."""
-                pass
+                if not orig or oowner != owner or ogroup != group:
+                        try:
+                                os.chown(temp, owner, group)
+                        except OSError, e:
+                                if e.errno != errno.EPERM:
+                                        raise
+
+                # This is safe even if temp == final_path.
+                os.rename(temp, final_path)
 
         def remove(self, image):
                 path = os.path.normpath(os.path.sep.join(
                     (image.get_root(), self.attrs["path"])))
 
-                print "removing file:", path
                 os.unlink(path)
 
         def generate_indices(self):
--- a/src/modules/actions/generic.py	Mon Aug 13 14:13:33 2007 -0700
+++ b/src/modules/actions/generic.py	Mon Aug 13 14:48:07 2007 -0700
@@ -110,8 +110,17 @@
         files.
         """
 
+        # 'name' is the name of the action, as specified in a manifest.
         name = "generic"
+        # 'attributes' is a list of the known usable attributes.  Or something.
+        # There probably isn't a good use for it.
         attributes = ()
+        # 'key_attr' is the name of the attribute whose value must be unique in
+        # the namespace of objects represented by a particular action.  For
+        # instance, a file's key_attr would be its pathname.  Or a driver's
+        # key_attr would be the driver name.  When 'key_attr' is None, it means
+        # that all attributes of the action are distinguishing.
+        key_attr = None
 
         def __init__(self, data=None, **attrs):
                 """Action constructor.
@@ -195,6 +204,27 @@
                 else:
                         return cmp(id(self), id(other))
 
+        def different(self, other):
+                """Returns True if other represents a non-ignorable change from self.
+                
+                By default, this means two actions are different if any of their
+                attributes are different.  Subclasses should override this
+                behavior when appropriate.
+                """
+
+                # We could ignore key_attr, or possibly assert that it's the
+                # same.
+                for a in self.attrs:
+                        if a not in other.attrs or self.attrs[a] != other.attrs[a]:
+                                return True
+
+                if hasattr(self, "hash"):
+                        assert(hasattr(other, "hash"))
+                        if self.hash != other.hash:
+                                return True
+
+                return False
+
         def generate_indices(self):
                 """Generate for the reverse index database data for this action.
 
@@ -226,15 +256,15 @@
 
                 return indices
 
-        def preinstall(self, image):
+        def preinstall(self, image, orig):
                 """Client-side method that performs pre-install actions."""
                 pass
 
-        def install(self, image):
+        def install(self, image, orig):
                 """Client-side method that installs the object."""
                 pass
 
-        def postinstall(self):
+        def postinstall(self, image, orig):
                 """Client-side method that performs post-install actions."""
                 pass
 
@@ -246,6 +276,6 @@
                 """Client-side method that removes the object."""
                 pass
 
-        def postremove(self):
+        def postremove(self, image):
                 """Client-side method that performs post-remove actions."""
                 pass
--- a/src/modules/actions/link.py	Mon Aug 13 14:13:33 2007 -0700
+++ b/src/modules/actions/link.py	Mon Aug 13 14:48:07 2007 -0700
@@ -39,11 +39,12 @@
 
         name = "link"
         attributes = ("path", "target")
+        key_attr = "path"
 
         def __init__(self, data=None, **attrs):
                 generic.Action.__init__(self, data, **attrs)
 
-        def install(self, image):
+        def install(self, image, orig):
                 """Client-side method that installs a link."""
                 # XXX The exists-unlink-symlink path appears to be as safe as it
                 # gets with the current symlink(2) interface.
@@ -63,5 +64,4 @@
                 path = os.path.normpath(os.path.sep.join(
                     (image.get_root(), self.attrs["path"])))
 
-                print "removing link:", path
                 os.unlink(path)
--- a/src/modules/client/image.py	Mon Aug 13 14:13:33 2007 -0700
+++ b/src/modules/client/image.py	Mon Aug 13 14:48:07 2007 -0700
@@ -193,8 +193,18 @@
 
         def get_manifest(self, fmri):
                 m = manifest.Manifest()
-                mcontent = file("%s/pkg/%s/manifest" % 
-                    (self.imgdir, fmri.get_dir_path())).read()
+
+                # If the manifest isn't there, download and retry.
+                try:
+                        mcontent = file("%s/pkg/%s/manifest" % 
+                            (self.imgdir, fmri.get_dir_path())).read()
+                except IOError, e:
+                        if e.errno != errno.ENOENT:
+                                raise
+                        retrieve.get_manifest(self, fmri)
+                        mcontent = file("%s/pkg/%s/manifest" % 
+                            (self.imgdir, fmri.get_dir_path())).read()
+
                 m.set_fmri(fmri)
                 m.set_content(mcontent)
                 return m
--- a/src/modules/client/pkgplan.py	Mon Aug 13 14:13:33 2007 -0700
+++ b/src/modules/client/pkgplan.py	Mon Aug 13 14:48:07 2007 -0700
@@ -25,10 +25,8 @@
 
 import errno
 import os
-import re
-import urllib
 
-import pkg.catalog as catalog
+import pkg.manifest as manifest
 
 class PkgPlan(object):
         """A package plan takes two package FMRIs and an Image, and produces the
@@ -41,15 +39,20 @@
         def __init__(self, image):
                 self.origin_fmri = None
                 self.destination_fmri = None
-                self.origin_mfst = None
-                self.destination_mfst = None
+                self.origin_mfst = manifest.null
+                self.destination_mfst = manifest.null
 
                 self.image = image
 
                 self.actions = []
 
         def __str__(self):
-                return "%s -> %s" % (self.origin_fmri, self.destination_fmri)
+                s = "%s -> %s\n" % (self.origin_fmri, self.destination_fmri)
+
+                for src, dest in self.actions:
+                        s += "  %s -> %s\n" % (src, dest)
+
+                return s
 
         def set_origin(self, fmri):
                 self.origin_fmri = fmri
@@ -94,38 +97,17 @@
                 # XXX Perhaps make the pkgplan creator make this explicit, so we
                 # don't have to check?
                 f = None
-                if self.origin_fmri == None:
+                if not self.origin_fmri:
                         try:
                                 f = self.image.get_version_installed(
                                     self.destination_fmri)
                                 self.origin_fmri = f
+                                self.origin_mfst = self.image.get_manifest(f)
                         except LookupError:
                                 pass
 
-                # if null package, then our plan is the set of actions for the
-                # destination version
-                if self.origin_fmri == None:
-                        self.actions = self.destination_mfst.actions
-                elif self.destination_fmri == None:
-                        # XXX
-                        self.actions = sorted(self.origin_mfst.actions,
-                            reverse = True)
-                else:
-                        # if a previous package, then our plan is derived from
-                        # the set differences between the previous manifest's
-                        # actions and the union of the destination manifest's
-                        # actions with the critical actions of the critical
-                        # versions in the version interval between origin and
-                        # destination.
-                        if not self.image.has_manifest(self.origin_fmri):
-                                retrieve.get_manifest(self.image,
-                                    self.origin_fmri)
-
-                        self.origin_mfst = self.image.get_manifest(
-                            self.origin_fmri)
-
-                        self.actions = self.destination_mfst.difference(
-                            self.origin_mfst)
+                self.actions = self.destination_mfst.difference(
+                    self.origin_mfst)
 
         def preexecute(self):
                 """Perform actions required prior to installation or removal of a package.
@@ -139,11 +121,11 @@
                         os.unlink("%s/pkg/%s/installed" % (self.image.imgdir,
                             self.origin_fmri.get_dir_path()))
 
-                for a in self.actions:
-                        if self.destination_fmri == None:
-                                a.preremove(self.image)
+                for src, dest in self.actions:
+                        if dest:
+                                dest.preinstall(self.image, src)
                         else:
-                                a.preinstall(self.image)
+                                src.preremove(self.image)
 
         def execute(self):
                 """Perform actions for installation or removal of a package.
@@ -151,12 +133,18 @@
                 This method executes each action's remove() or install()
                 methods.
                 """
+
                 # record that we are in an intermediate state
-                for a in self.actions:
-                        if self.destination_fmri == None:
-                                a.remove(self.image)
+
+                # It might be nice to have a single action.execute() method, but
+                # I can't think of an example where it would make especially
+                # good sense (i.e., where "remove" is as similar to "upgrade" as
+                # is "install").
+                for src, dest in self.actions:
+                        if dest:
+                                dest.install(self.image, src)
                         else:
-                                a.install(self.image)
+                                src.remove(self.image)
 
         def postexecute(self):
                 """Perform actions required after installation or removal of a package.
@@ -166,11 +154,11 @@
                 at such a time.
                 """
                 # record that package states are consistent
-                for a in self.actions:
-                        if self.destination_fmri == None:
-                                a.postremove()
+                for src, dest in self.actions:
+                        if dest:
+                                dest.postinstall(self.image, src)
                         else:
-                                a.postinstall()
+                                src.postremove(self.image)
 
                 # XXX should this just go in preexecute?
                 if self.origin_fmri != None and self.destination_fmri != None:
@@ -201,8 +189,9 @@
 
                 gen = (
                     (k, v)
-                    for action in self.actions
-                    for k, v in action.generate_indices().iteritems()
+                    for src, dest in self.actions
+                    if dest
+                    for k, v in dest.generate_indices().iteritems()
                 )
 
                 for idx, val in gen:
--- a/src/modules/manifest.py	Mon Aug 13 14:13:33 2007 -0700
+++ b/src/modules/manifest.py	Mon Aug 13 14:48:07 2007 -0700
@@ -97,10 +97,6 @@
         present in the image (which may be the null manifest).
         """
 
-        # XXX The difference methods work, but are wrong:  we must provide
-        # action equivalency relationships, so that we can easily discriminate
-        # related actions from independent ones.
-
         def __init__(self):
                 self.fmri = None
 
@@ -122,57 +118,45 @@
 
                 return r
 
-        def difference(self, other):
-                """Output is the list of positive actions to take to move from
-                other to self.  For instance, a file in self, but not in other,
-                would be added."""
-
-                sset = set([str(acs) for acs in self.actions])
-                oset = set([str(aco) for aco in other.actions])
-
-                for ats in self.attributes.keys():
-                        sset.add("%s=%s" % (ats, self.attributes[ats]))
+        def difference(self, origin):
+                """Return a list of action pairs representing origin and
+                destination actions."""
+                # XXX Do we need to find some way to assert that the keys are
+                # all unique?
 
-                for ato in other.attributes.keys():
-                        oset.add("%s=%s" % (ato, other.attributes[ato]))
-
-                dset = sset.symmetric_difference(oset)
-
-                positive_actions = []
+                sdict = dict(
+                    ((a.name, a.attrs.get(a.key_attr, id(a))), a)
+                    for a in self.actions
+                )
+                odict = dict(
+                    ((a.name, a.attrs.get(a.key_attr, id(a))), a)
+                    for a in origin.actions
+                )
 
-                for acs in self.actions:
-                        rep = str(acs)
-                        if rep in dset:
-                                positive_actions.append(acs)
-
-                return positive_actions
-
+                sset = set(sdict.keys())
+                oset = set(odict.keys())
 
-        def antidifference(self, other):
-                """Output is the list of negative actions to take to move from
-                other to self.  For instance, a file in other, but not in self,
-                would be undone.
-
-                XXX The antidifference must be reconciled with any replacement
-                actions on the positive side by a higher level routine."""
-
-                sset = set([str(acs) for acs in self.actions])
-                oset = set([str(aco) for aco in other.actions])
+                added = [(None, sdict[i]) for i in sset - oset]
+                removed = [(odict[i], None) for i in oset - sset]
+                changed = [
+                    (odict[i], sdict[i])
+                    for i in oset & sset
+                    if odict[i].different(sdict[i])
+                ]
 
-                for ats in self.attributes.keys():
-                        sset.add("%s=%s" % (ats, self.attributes[ats]))
-
-                for ato in other.attributes.keys():
-                        oset.add("%s=%s" % (ato, other.attributes[ato]))
+                # XXX Do changed actions need to be sorted at all?  This is
+                # likely to be the largest list, so we might save significant
+                # time by not sorting.  Should we sort above?  Insert into a
+                # sorted list?
 
-                dset = sset.symmetric_difference(oset)
+                # singlesort = lambda x: x[0] or x[1]
+                addsort = lambda x: x[1]
+                remsort = lambda x: x[0]
+                removed.sort(key = remsort)
+                added.sort(key = addsort)
+                changed.sort(key = addsort)
 
-                for aco in other.actions:
-                        rep = "%s" % acs
-                        if rep in dset:
-                                negative_actions.append(aco)
-
-                return negative_actions
+                return removed + added + changed
 
         def display_differences(self, other):
                 """Output expects that self is newer than other.  Use of sets
@@ -180,22 +164,15 @@
                 form, otherwise set member identities are derived from the
                 object pointers, rather than the contents."""
 
-                sset = set([str(acs) for acs in self.actions])
-                oset = set([str(aco) for aco in other.actions])
-
-                for ats in self.attributes.keys():
-                        sset.add("%s=%s" % (ats, self.attributes[ats]))
+                l = self.difference(other)
 
-                for ato in other.attributes.keys():
-                        oset.add("%s=%s" % (ato, other.attributes[ato]))
-
-                dset = sset.symmetric_difference(oset)
-
-                for att in dset:
-                        if att in sset:
-                                print "+ %s" % att
+                for src, dest in l:
+                        if not src:
+                                print "+", dest
+                        elif not dest:
+                                print "-", src
                         else:
-                                print "- %s" % att
+                                print "%s -> %s" % (src, dest)
 
         def set_fmri(self, fmri):
                 self.fmri = fmri
@@ -209,6 +186,13 @@
         def set_content(self, str):
                 """str is the text representation of the manifest"""
 
+                # So we could build up here the type/key_attr dictionaries like
+                # sdict and odict in difference() above, and have that be our
+                # main datastore, rather than the simple list we have now.  If
+                # we do that here, we can even assert that the "same" action
+                # can't be in a manifest twice.  (The problem of having the same
+                # action more than once in packages that can be installed
+                # together has to be solved somewhere else, though.)
                 for l in str.splitlines():
                         if re.match("^\s*(#.*)?$", l):
                                 continue
@@ -223,7 +207,7 @@
                                 action.data = \
                                     self.make_opener(self.fmri, action)
 
-                        if len(self.actions) == 0:
+                        if not self.actions:
                                 self.actions.append(action)
                         else:
                                 bisect.insort(self.actions, action)
@@ -247,7 +231,7 @@
         m2 = Manifest()
 
         y = """\
-set com.sun,test=true
+set com.sun,test=false
 set com.sun,data=true
 depend type=require fmri=pkg:/library/libc
 file fff555ff9 mode=0555 owner=sch group=staff path=/usr/bin/i386/sort isa=i386
@@ -267,3 +251,37 @@
         print null
 
         m2.display_differences(null)
+
+        print
+        m2.difference(m1)
+
+        m3 = Manifest()
+        t3 = """\
+dir mode=0755 owner=root group=sys path=/bin
+file 00000000 mode=0644 owner=root group=sys path=/bin/change
+file 00000001 mode=0644 owner=root group=sys path=/bin/nochange
+file 00000002 mode=0644 owner=root group=sys path=/bin/toberemoved
+link path=/bin/change-link target=change
+link path=/bin/nochange-link target=nochange
+link path=/bin/change-target target=target1
+link path=/bin/change-type target=random
+"""
+        m3.set_content(t3)
+
+        m4 = Manifest()
+        t4 = """\
+dir mode=0755 owner=root group=sys path=/bin
+file 0000000f mode=0644 owner=root group=sys path=/bin/change
+file 00000001 mode=0644 owner=root group=sys path=/bin/nochange
+file 00000003 mode=0644 owner=root group=sys path=/bin/wasadded
+link path=/bin/change-link target=change
+link path=/bin/nochange-link target=nochange
+link path=/bin/change-target target=target2
+dir mode=0755 owner=root group=sys path=/bin/change-type
+"""
+        m4.set_content(t4)
+
+        print "\n" + 50 * "=" + "\n"
+        m4.difference(m3)
+        print "\n" + 50 * "=" + "\n"
+        m4.difference(null)
--- a/src/modules/server/transaction.py	Mon Aug 13 14:13:33 2007 -0700
+++ b/src/modules/server/transaction.py	Mon Aug 13 14:48:07 2007 -0700
@@ -196,6 +196,9 @@
                 if "Content-Length" in hdrs and \
                     int(hdrs.getheader("Content-Length")) != 0:
                         rfile = request.rfile
+                # XXX Ugly special case to handle empty files.
+                elif type == "file":
+                        rfile = "/dev/null"
                 action = pkg.actions.types[type](rfile, **attrs)
 
                 # XXX Once actions are labelled with critical nature.
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/src/tests/simple-upgrade.ksh	Mon Aug 13 14:48:07 2007 -0700
@@ -0,0 +1,66 @@
+#!/bin/ksh -px
+#
+# CDDL HEADER START
+#
+# The contents of this file are subject to the terms of the
+# Common Development and Distribution License (the "License").
+# You may not use this file except in compliance with the License.
+#
+# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
+# or http://www.opensolaris.org/os/licensing.
+# See the License for the specific language governing permissions
+# and limitations under the License.
+#
+# When distributing Covered Code, include this CDDL HEADER in each
+# file and include the License file at usr/src/OPENSOLARIS.LICENSE.
+# If applicable, add the following below this CDDL HEADER, with the
+# fields enclosed by brackets "[]" replaced with your own identifying
+# information: Portions Copyright [yyyy] [name of copyright owner]
+#
+# CDDL HEADER END
+
+# Copyright 2007 Sun Microsystems, Inc.  All rights reserved.
+# Use is subject to license terms.
+
+eval `pkgsend open test/upgrade/[email protected]`
+if [ $? != 0 ]; then
+	echo \*\* script aborted:  couldn\'t open test/upgrade/A
+	exit 1
+fi
+
+echo "version 1 of /bin/change" > /tmp/change
+echo "version 1 of /bin/nochange" > /tmp/nochange
+
+echo $PKG_TRANS_ID
+pkgsend add dir  0755 root sys /bin
+pkgsend add file 0644 root sys /bin/change /tmp/change
+pkgsend add file 0644 root sys /bin/nochange /tmp/nochange
+pkgsend add file 0644 root sys /bin/toberemoved /dev/null
+pkgsend add file 0755 root sys /bin/attributechangeonly /dev/null
+pkgsend add link /bin/change-link change
+pkgsend add link /bin/nochange-link nochange
+pkgsend add link /bin/change-target target1
+pkgsend add link /bin/change-type random
+pkgsend close
+
+eval `pkgsend open test/upgrade/[email protected]`
+if [ $? != 0 ]; then
+	echo \*\* script aborted:  couldn\'t open test/upgrade/A
+	exit 1
+fi
+
+echo "version 2 of /bin/change" > /tmp/change
+
+echo $PKG_TRANS_ID
+pkgsend add dir  0755 root sys /bin
+pkgsend add file 0644 root sys /bin/change /tmp/change
+pkgsend add file 0644 root sys /bin/nochange /tmp/nochange
+pkgsend add file 0644 root sys /bin/wasadded /dev/null
+pkgsend add file 0444 root sys /bin/attributechangeonly /dev/null
+pkgsend add link /bin/change-link change
+pkgsend add link /bin/nochange-link nochange
+pkgsend add link /bin/change-target target2
+pkgsend add dir  0755 root sys /bin/change-type
+pkgsend close
+
+rm /tmp/change /tmp/nochange