Danek Duvall wrote: > On Fri, Apr 04, 2008 at 07:08:04PM -0700, Bart Smaalders wrote: > >> http://cr.opensolaris.org/~barts/optional-dependencies/ > > In general, you seem to have lost whatever settings you were using to > expand tab indentations to spaces.
Yup; fixed. All comments not noted were addressed in the obvious manner. > - line 115: If you're being careful about KeyErrors here, then why not in > parse() and verify()? > I had a broken depend action in the repo; this code found the problem. We need to implement more error checking on publish. I've removed this checking for now as it's not needed for production code. > > generic.py: ... > - line 430: will we ever return more than one? > Not here unless there are multiple path attributes on an action, in which case we'll be tossing tracebacks all over :-). the Directory action already returns more than one.. > legacy.py: > - line 64: get_name() may return a name with a slash in it, which likely > won't work when it's time to create the link. Should this be down > closer to where it's used? I used fmri.get_url_path() here to resolve this - good catch. > - line 1017: what kind of clever thing did you have in mind? > fixed comment to be more specific re error passback. > - line 1021: I'm wondering if perhaps we should have multiple lost+found > directories, each with a timestamp, rather than timestamping individual > files (fix the timestamp for the duration of the operation)? This > would make it easier to match pkg failures with places to look. Not > sure how this would interact with the gui. > TBD - deferred > > imageplan.py: > - line 175: "var/sadm/install" is special for the legacy action, right? > yes > fmri.py: > > - line 183: you're not using dump_fmri() anywhere, and get_fmri() is > probably better anyway. > debugging method - removed. > - line 211: I still think my hashing function is better, even if it's no > safer. > ok. I'll defer this until we rewhack fmri impl. for performance. b > publish.py: > > - line 197, 198: line.strip() > > - line 205: I'm confused here. What's the purpose of putting the > localpath attribute in here just to rip it out again, if the action > constructor doesn't do anything with it? If it's just junk, why not > use "NOHASH"? ah... that works too. fine. > > - line 224: "if "path" in action.attrs:" > > - line 228: do you really want a new Transaction for each file, rather > than one to cover the whole include? good point... fixed. Code review updated. I've included updated test for upgrade, which also test implicit directory removal, and entire incorporations for builds 75, 79. 85 and 86, plus changes to Makefile in src/util/distro_import to build incorporations. I still want to build incorporations file from distro automatically, but I need to be able to retrieve manifest w/o install first to be able to do that. - Bart -- Bart Smaalders Solaris Kernel Performance [EMAIL PROTECTED] http://blogs.sun.com/barts "You will contribute more with mercurial than with thunderbird." _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
