18172 p.d.d.file is misleading when links are involved
authorBrock Pytlik <brock.pytlik@oracle.com>
Tue, 19 Apr 2011 02:46:00 -0700
changeset 2298 c15c3c1a5590
parent 2297 9c53302b94fd
child 2299 04f16b1bc15c
18172 p.d.d.file is misleading when links are involved 18173 pkgdepend resolve doesn't handle links moving correctly
src/modules/flavor/base.py
src/modules/publish/dependencies.py
src/tests/cli/t_pkgdep_resolve.py
--- a/src/modules/flavor/base.py	Mon Apr 18 12:55:39 2011 -0700
+++ b/src/modules/flavor/base.py	Tue Apr 19 02:46:00 2011 -0700
@@ -298,7 +298,8 @@
                         res_pths, res_links = resolve_links(
                             path_to_check, delivered_files, links,
                             orig_dep_vars, attrs)
-                        for res_pth, res_pfmri, res_vc in res_pths:
+                        for res_pth, res_pfmri, res_vc, res_via_links in \
+                            res_pths:
                                 p = self._check_path(res_pth, delivered_files)
                                 if p:
                                         res.append((p, res_vc))
--- a/src/modules/publish/dependencies.py	Mon Apr 18 12:55:39 2011 -0700
+++ b/src/modules/publish/dependencies.py	Tue Apr 19 02:46:00 2011 -0700
@@ -50,6 +50,7 @@
 type_prefix = "%s.type" % base.Dependency.DEPEND_DEBUG_PREFIX
 target_prefix = "%s.target" % base.Dependency.DEPEND_DEBUG_PREFIX
 bypassed_prefix = "%s.bypassed" % base.Dependency.DEPEND_DEBUG_PREFIX
+via_links_prefix = "%s.via-links" % base.Dependency.DEPEND_DEBUG_PREFIX
 
 # A tag used to hold the product of the paths_prefix and files_prefix
 # contents, used when bypassing dependency generation
@@ -539,19 +540,26 @@
         return m, missing_files
 
 def choose_name(fp, mfst):
-        """Find the package name for this manifest. If it's defined in a set
-        action in the manifest, use that. Otherwise use the basename of the
-        path to the manifest as the name.
+        """Find the package name for this manifest.  If it's defined in a set
+        action in the manifest, use that.  Otherwise use the basename of the
+        path to the manifest as the name.  If a proper package fmri is found,
+        then also return a PkgFmri object so that we can track which packages
+        are being processed.
+
         'fp' is the path to the file for the manifest.
 
         'mfst' is the Manifest object."""
 
         if mfst is None:
-                return urllib.unquote(os.path.basename(fp))
+                return urllib.unquote(os.path.basename(fp)), None
         name = mfst.get("pkg.fmri", mfst.get("fmri", None))
         if name is not None:
-                return name
-        return urllib.unquote(os.path.basename(fp))
+                try:
+                        pfmri = fmri.PkgFmri(name)
+                except fmri.IllegalFmri:
+                        pfmri = None
+                return name, pfmri
+        return urllib.unquote(os.path.basename(fp)), None
 
 def helper(lst, file_dep, dep_vars, orig_dep_vars):
         """Creates the depend actions from lst for the dependency and determines
@@ -572,7 +580,7 @@
         res = []
         vars = []
         errs = set()
-        for pfmri, delivered_vars in lst:
+        for path, pfmri, delivered_vars, via_links in lst:
                 # If the pfmri package isn't present under any of the variants
                 # where the dependency is, skip it.
                 if not orig_dep_vars.intersects(delivered_vars,
@@ -596,6 +604,15 @@
                 dep_vars.mark_as_satisfied(delivered_vars)
                 attrs = file_dep.attrs.copy()
                 attrs.update({"fmri":pfmri.get_short_fmri()})
+                # We know which path satisfies this dependency, so remove the
+                # list of files and paths, and replace them with the single path
+                # that works.
+                attrs.pop(paths_prefix, None)
+                attrs.pop(fullpaths_prefix, None)
+                attrs[files_prefix] = [path]
+                if via_links:
+                        via_links.reverse()
+                        attrs[via_links_prefix] = ":".join(via_links)
                 # Add this package as satisfying the dependency.
                 res.append((actions.depend.DependencyAction(**attrs),
                     action_vars))
@@ -670,7 +687,7 @@
                         inter = path_vars.intersection(p_vc)
                         inter.mark_all_as_satisfied()
                         tmp_vars.mark_as_satisfied(p_vc)
-                        res_paths.append((path, pfmri, inter))
+                        res_paths.append((path, pfmri, inter, []))
                 # If the path was resolved under all relevant variants, then
                 # we're done.
                 if res_paths and tmp_vars.is_satisfied():
@@ -684,18 +701,9 @@
 
         # Create the path to check for links.
         cur_path = os.path.join(*lst[0:index])
-        # The links in delivered packages (ie, those current being resolved)
-        # should be preferred over links from the installed system.
+        # Find the links which match the path being checked.
         rel_links = links.delivered.get(cur_path, [])
-        tmp = set()
-        for pfmri, vc, rel_target in rel_links:
-                tmp.add(pfmri.get_name())
-        # Only considered links from installed packages which are not also being
-        # resolved.
-        for pfmri, vc, rel_target in links.installed.get(cur_path, []):
-                if pfmri.get_name() in tmp:
-                        continue
-                rel_links.append((pfmri, vc, rel_target))
+        rel_links.extend(links.installed.get(cur_path, []))
         # If there weren't any relevant links, then add the next path component
         # to the path being considered and try again.
         if not rel_links:
@@ -729,15 +737,17 @@
                 # add the paths and the dependencies from links found to the
                 # results.
                 res_links.extend(link_deps)
+                for rec_path, rec_pfmri, rec_vc, via_links in rec_paths:
+                        via_links.append(path)
                 res_paths.extend(rec_paths)
                 # Now add in the dependencies for the current link.
-                for rec_path, rec_pfmri, rec_vc in rec_paths:
+                for rec_path, rec_pfmri, rec_vc, via_links in rec_paths:
                         attrs = file_dep_attrs.copy()
                         attrs.update({
                             "fmri": link_pfmri.get_short_fmri(),
                             type_prefix: "link",
                             target_prefix: next_path,
-                            files_prefix: path
+                            files_prefix: [path]
                         })
                         attrs.pop(paths_prefix, None)
                         attrs.pop(fullpaths_prefix, None)
@@ -790,22 +800,12 @@
                 paths_info, path_deps = resolve_links(os.path.normpath(p),
                     files_dict, links, orig_dep_vars, file_dep.attrs.copy())
                 link_deps.extend(path_deps)
-                delivered_list = \
-                    [(pfmri, vc) for (path, pfmri, vc) in paths_info]
                 try:
-                        new_res, dep_vars = helper(delivered_list, file_dep,
+                        new_res, dep_vars = helper(paths_info, file_dep,
                             dep_vars, orig_dep_vars)
                 except AmbiguousPathError, e:
                         errs.append(e)
                 else:
-                        # We know which path satisfies this dependency, so
-                        # remove the list of files and paths, and replace them
-                        # with the single path that works.
-                        for na, nv in new_res:
-                                na.attrs.pop(paths_prefix, None)
-                                na.attrs.pop(fullpaths_prefix, None)
-                                na.attrs[files_prefix] = [p]
-
                         if not res:
                                 res = new_res
                                 continue
@@ -1066,8 +1066,12 @@
         # purposes of dependency resolution.
         distro_vars = variants.VariantCombinationTemplate()
 
-        for mp, name, mfst, pkg_vars, miss_files in manifests:
+        resolving_pkgs = set()
+
+        for mp, (name, pfmri), mfst, pkg_vars, miss_files in manifests:
                 distro_vars.merge_values(pkg_vars)
+                if pfmri:
+                        resolving_pkgs.add(pfmri.pkg_name)
 
         pkg_list = None
         if use_system:
@@ -1078,12 +1082,15 @@
                     api.ImageInterface.LIST_INSTALLED))
                 for (pub, stem, ver), summ, cats, states in pkg_list:
                         pfmri = fmri.PkgFmri("pkg:/%s@%s" % (stem, ver))
+                        if pfmri.pkg_name in resolving_pkgs:
+                                continue
                         mfst = api_inst.get_manifest(pfmri, all_variants=True)
                         distro_vars.merge_values(mfst.get_all_variants())
 
         # Build a list of all files delivered in the manifests being resolved.
-        for mp, name, mfst, pkg_vars, miss_files in manifests:
-                pfmri = fmri.PkgFmri(name)
+        for mp, (name, pfmri), mfst, pkg_vars, miss_files in manifests:
+                if pfmri is None:
+                        pfmri = fmri.PkgFmri(name)
                 add_fmri_path_mapping(files.delivered, links.delivered, pfmri,
                     mfst, distro_vars)
 
@@ -1092,6 +1099,8 @@
         if use_system:
                 for (pub, stem, ver), summ, cats, states in pkg_list:
                         pfmri = fmri.PkgFmri("pkg:/%s@%s" % (stem, ver))
+                        if pfmri.pkg_name in resolving_pkgs:
+                                continue
                         mfst = api_inst.get_manifest(pfmri, all_variants=True)
                         add_fmri_path_mapping(files.installed, links.installed,
                             pfmri, mfst, distro_vars)
@@ -1101,7 +1110,7 @@
 
         pkg_deps = {}
         errs = []
-        for mp, name, mfst, pkg_vars, miss_files in manifests:
+        for mp, (name, pfmri), mfst, pkg_vars, miss_files in manifests:
                 # The add_fmri_path_mapping function moved the actions it found
                 # into the distro_vars universe of variants, so we need to move
                 # pkg_vars (and by extension the variants on depend actions)
--- a/src/tests/cli/t_pkgdep_resolve.py	Mon Apr 18 12:55:39 2011 -0700
+++ b/src/tests/cli/t_pkgdep_resolve.py	Tue Apr 19 02:46:00 2011 -0700
@@ -483,6 +483,53 @@
 file NOHASH group=sys mode=0600 owner=root path=var/log/authlog variant.arch=foo variant.opensolaris.zone=global variant.debug=True
 file NOHASH group=sys mode=0600 owner=root path=var/log/authlog variant.arch=foo variant.opensolaris.zone=nonglobal variant.debug=False
 """
+        bug_18172_top = """\
+set name=pkg.fmri [email protected],5.11-1
+depend fmri=__TBD pkg.debug.depend.file=ksh pkg.debug.depend.path=usr/bin pkg.debug.depend.reason=usr/lib/brand/shared/dsconvert pkg.debug.depend.type=script type=require
+"""
+        bug_18172_l1 = """\
+set name=pkg.fmri [email protected],5.11-1
+link path=usr/bin/ksh target=../../usr/lib/l1
+"""
+        bug_18172_l2 = """\
+set name=pkg.fmri [email protected],5.11-1
+link path=usr/lib/l1 target=l2
+"""
+        bug_18172_l3 = """\
+set name=pkg.fmri [email protected],5.11-1
+link path=usr/lib/l2 target=isaexec
+"""
+        bug_18172_dest = """\
+set name=pkg.fmri [email protected],5.11-1
+file tmp/foo path=usr/lib/isaexec
+"""
+
+        bug_18173_cs_1 = """\
+open [email protected],5.11-1
+add set name=pkg.fmri [email protected],5.11-1
+add file NOHASH path=usr/lib/isaexec mode=0755 owner=root group=sys
+
+add link path=usr/sbin/sh target=../bin/i86/ksh93
+add hardlink path=usr/bin/ksh target=../../usr/lib/isaexec
+close
+"""
+
+        bug_18173_cs_2 = """\
+set name=pkg.fmri [email protected],5.11-2
+file tmp/foo path=usr/lib/isaexec
+"""
+
+        bug_18173_ksh = """\
+set name=pkg.fmri [email protected],5.11-2
+hardlink path=usr/bin/ksh target=../../usr/lib/isaexec
+depend fmri=__TBD pkg.debug.depend.file=isaexec pkg.debug.depend.path=usr/lib pkg.debug.depend.reason=usr/bin/ksh pkg.debug.depend.type=hardlink type=require
+"""
+
+        bug_18173_zones = """\
+set name=pkg.fmri [email protected],5.11-2
+file NOHASH path=usr/lib/brand/shared/dsconvert mode=0755
+depend fmri=__TBD pkg.debug.depend.file=ksh pkg.debug.depend.path=usr/bin pkg.debug.depend.reason=usr/lib/brand/shared/dsconvert pkg.debug.depend.type=script type=require
+"""
 
         misc_files = ["tmp/foo"]
 
@@ -1386,5 +1433,113 @@
                     "of pkgdeps for the dependent package. Deps were:\n%s" %
                     "\n".join([str(d) for d in pkg_deps[d_path]]))
 
+        def test_bug_18172(self):
+                """Test that the via-links attribute is set correctly when
+                multiple packages deliver a chain of links."""
+
+                top_path = self.make_manifest(self.bug_18172_top)
+                l1_path = self.make_manifest(self.bug_18172_l1)
+                l2_path = self.make_manifest(self.bug_18172_l2)
+                l3_path = self.make_manifest(self.bug_18172_l3)
+                dest_path = self.make_manifest(self.bug_18172_dest)
+
+                pkg_deps, errs = dependencies.resolve_deps(
+                    [top_path, l1_path, l2_path, l3_path, dest_path],
+                    self.api_obj, use_system=False)
+                if errs:
+                        raise RuntimeError("Got the following unexpected "
+                            "errors:\n%s" % "\n".join(["%s" % e for e in errs]))
+                self.assertEqual(len(pkg_deps), 5)
+                self.assertEqual(len(pkg_deps[dest_path]), 0)
+                self.assertEqual(len(pkg_deps[l1_path]), 0)
+                self.assertEqual(len(pkg_deps[l2_path]), 0)
+                self.assertEqual(len(pkg_deps[l3_path]), 0)
+                self.assertEqual(len(pkg_deps[top_path]), 4,
+                    "Got wrong number of pkgdeps for the dependent package. "
+                    "Deps were:\n%s" %
+                    "\n".join([str(d) for d in pkg_deps[top_path]]))
+                got_top = False
+                got_l1 = False
+                got_l2 = False
+                got_l3 = False
+                for d in pkg_deps[top_path]:
+                        if d.attrs["fmri"].startswith("pkg:/dest"):
+                                self.assertEqual(
+                                    d.attrs[dependencies.files_prefix],
+                                    ["usr/lib/isaexec"])
+                                self.assertEqual(
+                                    d.attrs[dependencies.via_links_prefix],
+                                    "usr/bin/ksh:usr/lib/l1:usr/lib/l2")
+                                self.assertEqual(
+                                    d.attrs[dependencies.type_prefix], "script")
+                                got_top = True
+                        elif d.attrs["fmri"].startswith("pkg:/ksh"):
+                                got_l1 = True
+                        elif d.attrs["fmri"].startswith("pkg:/l2"):
+                                got_l2 = True
+                        else:
+                                self.assert_(d.attrs["fmri"].startswith(
+                                    "pkg:/l3"))
+                                got_l3 = True
+                self.assert_(got_top and got_l1 and got_l2 and got_l3, "Got "
+                    "the right number of dependencies but missed one of the "
+                    "expected ones. Deps were:\n%s" %
+                    "\n".join([str(d) for d in pkg_deps[top_path]]))
+
+        def test_bug_18173(self):
+                """Test that if a link moves in a new version of a package, the
+                link in the installed version of the package doesn't interfere
+                with dependency resolution."""
+
+                self.make_misc_files("usr/lib/isaexec")
+                self.pkgsend_bulk(self.rurl, self.bug_18173_cs_1)
+                cs2_path = self.make_manifest(self.bug_18173_cs_2)
+                ksh_path = self.make_manifest(self.bug_18173_ksh)
+                zones_path = self.make_manifest(self.bug_18173_zones)
+
+                self.api_obj.refresh(immediate=True)
+                self._api_install(self.api_obj, ["cs"])
+
+                pkg_deps, errs = dependencies.resolve_deps(
+                    [cs2_path, ksh_path, zones_path], self.api_obj,
+                    use_system=True)
+                if errs:
+                        raise RuntimeError("Got the following unexpected "
+                            "errors:\n%s" % "\n".join(["%s" % e for e in errs]))
+                self.assertEqual(len(pkg_deps), 3)
+                self.assertEqual(len(pkg_deps[cs2_path]), 0)
+                self.assertEqual(len(pkg_deps[ksh_path]), 1)
+                self.assertEqual(len(pkg_deps[zones_path]), 2,
+                    "Got wrong number of pkgdeps for the dependent package. "
+                    "Deps were:\n%s" %
+                    "\n".join([str(d) for d in pkg_deps[zones_path]]))
+                got_cs2 = False
+                got_ksh = False
+                for d in pkg_deps[zones_path]:
+                        if d.attrs["fmri"].startswith("pkg:/cs"):
+                                self.assert_(d.attrs["fmri"].endswith("-2"))
+                                self.assertEqual(
+                                    d.attrs[dependencies.files_prefix],
+                                    ["usr/lib/isaexec"])
+                                self.assertEqual(
+                                    d.attrs[dependencies.via_links_prefix],
+                                    "usr/bin/ksh")
+                                self.assertEqual(
+                                    d.attrs[dependencies.type_prefix], "script")
+                                got_cs2 = True
+                        else:
+                                self.assert_(d.attrs["fmri"].startswith(
+                                    "pkg:/ksh"))
+                                self.assertEqual(
+                                    d.attrs[dependencies.files_prefix],
+                                    ["usr/bin/ksh"])
+                                self.assertEqual(
+                                    d.attrs[dependencies.type_prefix], "link")
+                                got_ksh = True
+                self.assert_(got_cs2 and got_ksh, "Got the right number "
+                    "of dependencies but missed one of the expected ones. "
+                    "Deps were:\n%s" %
+                    "\n".join([str(d) for d in pkg_deps[zones_path]]))
+
 if __name__ == "__main__":
         unittest.main()