Thanks for the detailed review. Comments below. Any of your comments that I do not comment on are considered 'accepted' as-is.
Danek Duvall wrote: >> http://cr.opensolaris.org/~jhf/ips-multi-2/index.html > > image.py: > > - 548,593: Now that these aren't returning the tuples their namesakes do, > they probably ought to be renamed so no one gets confused. > > > generic.py: > > - 298: "absolute" > - 309: I know this was incredibly difficult for me to get right, so I'm > wary of the change here, especially since it doesn't appear to result > in identical code. Was the "/" I had there never actually necessary? It didn't appear so. Previously, you were setting pathlist[0] to "/", and then constructing a path using "/" plus a subset of the elements of pathlist starting at 0. So, the "/" appeared to be redundant in the call to join(). > - 312ff: There doesn't seem to be any change here. Did you correct the > tab indents to spaces? Yep, but I didn't do a very good job - there are still some tabs in there. I'll fix them. > os_base.py: > > - if we need this at all, perhaps use a decorator method? Hmm.. what would the decorator do? Get rid of all the NotImplementedErrors so we'd just have something like @abstract get_platform(self): pass @abstract get_release(self): pass ? > __init__.py, interface.py: > > - should make everything available through pkg.portable. "import > pkg.portable as portable" and then "portable.foo()". Ok. Is that possible, while still maintaining a class hierarchy (so that, for example, os_sunos can share most of the os_unix implementation)? > - iface.py, 64-65, 72: use join() or %s > > - 70: "if not impl:" > > - 73: no trailing blank line. > > os_unix.py: > > - 35: "of" -> "if" > os_windows.py: > > - 72: Why would this call result in an ImportError? Wouldn't that have > happened back when you imported getpass on line 26? No, the getpass module doesn't actually do the import of pwd until you call getuser(). > - 79: Aren't you missing "()"? > > - 84ff (and other places): Use 8-space indents. > > - 85: More appropriate as a comment. > > - 99: Capitalize "windows" > > util.py: > > - 31,43: "standardized" > > - 71: "representing" > > config.py: > > - the changes here look okay, but I'm now not able to reconstruct why > Stephen decided to use vfsstat here. Perhaps it was just the mode we > were thinking in when we designed the behavior, or maybe there's a good > reason that's not coming to mind at the moment. Either way, be sure to > get his okay on this change first. Ok. Stephen? :-) (I'll try and get his OK separately) > face.py: > > - 28: why? Left over from previous attempt. Will remove. > - 37: I'd do this with a try/except clause instead of an if/else clause. > I've been leaning towards the "forgiveness" end of the spectrum when it > doesn't result in less readable code. > > transaction.py: > > - 220: You're putting too much inside the try. Only the import statement > should be there. And given that you're doing a return if the import > fails, there's no need to use an else clause to put the rest of the elf > bits into. > > elf.c: > > - 32: Is __sun defined by gcc, too? Yep. > - 37: If the method signatures were different, this wouldn't work. I > think you mean simply method names. > > - 55: Why the extra blank line? (You have some trailing blank lines in > other files, too -- remove those.) A few files were missing trailing newlines, and webrev complained. I must have added too much when I fixed the complaint. I'll remove them. > elfextract.c: > > - 84: Can't you just check to see if EM_486 is #defined, or are these > not macros on all platforms? It's not on Ubuntu 7.04. > - 187,222: ? > > - 365,376: Perhaps we should be checking for both section names on all > platforms? I don't know how much sense that makes. SHT_SUNW_verdef and SHT_SUNW_verneed seem to only exist on Solaris. Since they are defined to be the same value there, I would expect that to be the case everywhere.. > I've not yet reviewed your scripts, batchfiles, tests, or setup.py, though > I'd like to make sure that the shell-script frontends to the pkg commands > aren't delivered in situations where they're not necessary. They seem > pretty gross to me, particularly use of LD_LIBRARY_PATH. The shell script frontends are currently only produced when using setup.py. When using Makefiles, they are ignored. Ultimately it should probably be a build-time option to produce the wrappers or not, if/when we retire the Makefiles. The use of LD_LIBRARY_PATH is needed when we bundle private copies of libraries (like OpenSSL) for platforms that don't have them. This won't likely be needed once we start producing multi-platform IPS packages that contain IPS, as they could express a dependency on the needed libraries. -jhf- _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
