On Thu, Jul 10, 2008 at 12:57:41AM -0500, Shawn Walker wrote: > http://cr.opensolaris.org/~dduvall/pkg-newlist/src/client.py.wdiff.html > ========== > 821 +pkg: no packages matching the following patterns you specified are > 822 +installed on the system. Specify -r to retrieve requested > information > 823 +from the repository:""")) > > Won't -r retrieve information from more than one possible repository > if they more than one authority (in the future, if not now)?
Yup. But there are more problems with that message. You might still get it when you already specified -r. I'll tailor that a bit more. > http://cr.opensolaris.org/~dduvall/pkg-newlist/src/modules/catalog.py.wdiff.html > ========== > 193 + does, it will be ignored. > 194 + """ > > Shouldn't line 194 be moved up to line 193 to match line 216, etc.? Sure. > 204 + else: > 205 + if version not in d[fmri.pkg_name]: > 206 + d[fmri.pkg_name][version] = \ > 207 + (fmri, [ auth ]) > 208 + bisect.insort( > 209 + > d[fmri.pkg_name]["versions"], fmri.version) > 210 + else: > 211 + > d[fmri.pkg_name][version][1].append(auth) > > Couldn't 204 be changed to elif merging 204 and 205 and bringing 210 > back an indent level? I don't see why not. Unfortunately, the bisect line still comes out to 81 characters. :) > http://cr.opensolaris.org/~dduvall/pkg-newlist/src/modules/client/image.py.wdiff.html > ========== > > 654 + os.symlink("../../pkg/%s/installed" % > fmri.get_dir_path(), > 655 + "%s/state/installed/%s" % (self.imgdir, > fmri.get_link_path())) > > 666 + if not os.path.isdir("%s/state/installed" % > self.imgdir): > > 670 + os.unlink("%s/state/installed/%s" % (self.imgdir, > 671 + fmri.get_link_path())) > > > Why not os.path.join here for the various paths and store installed > path and installed.builed path in variables? Are we not supposed to > always use it? A variable wouldn't save you anything, since each path is used exactly once in each method -- one absolute and one relative path to the symlink, and one absolute path to its target. As for using os.path.join(), it doesn't really buy you anything, either -- it takes up more space in these cases, and doesn't get you any cross-platform compatibility goodies. > 920 + self.cache_catalogs() > 921 + > 894 922 if failed: > 895 923 raise RuntimeError, (failed, total, > succeeded) > > Is 920 and 921 intentionally before 922/923? Is the thought that we > should cache what we can? Is there any concern that if failed was true > that we would be caching an inconsistent or bad catalog state? That was the thought, but it's possible to get a bad catalog. I'll take another look at this. > 1067 + self.pkg_states[fmristr] = > ("installed", f) > 1085 + self.pkg_states[fmristr] = > ("installed", f) > 1288 + "state": inst_state, > 1289 + "upgradable": ver != newest, > 1290 + "frozen": False, > 1291 + "incorporated": False, > 1292 + "excludes": False > > It would be nice to have constants for the pkg states instead of > hardcoding "installed", etc. in various places. Okay. > 1192 + patterns = [ ] > > Space between []. Yup. > http://cr.opensolaris.org/~dduvall/pkg-newlist/src/modules/fmri.py.wdiff.html > ========== > 275 + return "%s" % > (urllib.quote(self.pkg_name, "")) > > Why not just return "urllib.quote(self.pkg_name, "")" ? Ask Stephen. He copied his own code, which you can see in get_dir_path() and get_url_path(). I thought about combining these three methods, because they're almost identical, but didn't feel it was worthwhile when I was still trying to get everything working. I can take another look at this now, though. > General Comments > ========== > I tried out your patch. list is *much* better than it used to be for > the -s and -a case. > > *However*, I noticed that "list -sa" is still atrociously slow. Yeah, well, there's precious little I can do about that -- it has to download every manifest of every package version and parse it. Algorithmic improvements aren't going to help that travesty out much. > Still, on my local system, after the first run, list -a completes in > seconds instead of minutes as it does now. Excellent. I'll get another webrev out tomorrow. Thanks, Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
