On Mon, Mar 10, 2008 at 03:56:59PM -0500, Tom Mueller wrote: > This was current as of Feb 7 when the code review request was submitted.
Ah, okay, of course. > Would it be better to break up these porting changes into a set of about > 5 or so deltas rather than one big delta to make things easier for code > review? No, I don't think so, though the setup.py bits are more interesting on the Solaris side of the camp, so if you want to get those bits in first, that'd work. In any case, it'd be nice to have a webrev between the 2/7 version and your next. >> I'm also not sure that it's an error not to be able to check the elf >> hash; it'll just make your downloads less efficient. >> > We're not expecting a case where the file action would be checking files > that have elf hashes, but the elf module would not be available. That's why > this is flagged as an error. Hm, okay. It can be fixed pretty easily if we run into a situation like that. >> image.py: >> >> - line 146: no need to wrap > > A different change is actually needed here. On non-Unix systems, where > there is no /var/pkg directory, this assertion is triggered if an attempt > is made to do a find_root on a directory that is not contained within an > image. We need a nice exception to be raised in this case so that the GUI > code can catch this. > > I was thinking of raising a ValueError exception, i.e., > > if d == oldpath: > raise ValueError, "directory %s not contained within an image" > % path In general, we need tighter exception raising. Either more specific classes, or at least make the data sent back a clear, parseable part of the API. Something like this is a start, though I'd probably have the path as a separate argument to the exception constructor. A different wad, though. >> portable/__init.py__: >> >> - line 240/241: Did you try doing something like: >> >> from os_<implname> import * >> >> That way you could avoid the double function call thing you've got >> going on here. (I realize you'd have to define the class methods at >> the top-level, too. And those that can be probably should be >> decorated with @staticmethod.) >> > I'm not sure I'm following what you mean by defining the class methods at > the top-level. "from foo import *" means to take all the names left at the top level of foo by the time the module is read and put them in the namespace where the import is done. If the only thing in foo's top level is the class definition, then you don't get the class methods imported, just the class, which is not the effect you're going for. > Do you mean not use classes here at all? No, you have to do that in order to inherit properly, which you want so you don't have code duplication. I pointed out to James several times that os.path seems to be a pretty good model for this sort of thing, and that you should try to copy that, if possible. I haven't heard that it's either impossible or ridiculously difficult, but I'd like to hear the reasoning if you're not going to do it that way. Stephen's also suggested a couple of times that the dynamic loading is actually unnecessary, since we're only going to be running any particular installation of IPS on a single OS, so the switching could actually be done at (package) build time. Given that this should be exceedingly simple, is there a reason you're not doing that? >> I think you'd also be better off making the various class instances >> singletons, so that caches are shared process-wide. Or can you >> confirm that's already the case? >> > The class is only instantiated with the impl variable is initialized, so > the class instances are singletons. But don't you get a new instance every time you "import pkg.portable"? It'd be easy enough to check with some debugging printfs. >> portable/os_unix.py: >> >> - line 76 (et al): don't you want to catch KeyError here? >> > Good question. If KeyError is caught here, then we would need to choose > some appropriate value to return for the uid and gid. ?? You don't return anything in your blanket except statement; why would you need to if you only caught KeyError? My point here is that I think that your except statement is too wide a net. Only catch the exceptions you actually think might happen. Every time you do this, you make it more difficult to hit ^C, etc. >> scripts/*.sh: >> >> - what are these for? >> > On Linux, older Solaris systems (.e.g, 9) and Mac OS, python is not always > found at /usr/bin. When IPS is shipped in a user-installable image, it may > need to be accompanied by a python that will be available in the user > image. These scripts provide for a "pkg" command that can find the python > at various different places. Okay. As long as these scripts aren't used on the current release of Solaris, I don't particularly care what you do. Though we could fix that particular problem on S10, too. >> api-complete.py: >> >> - Why did you pull the platform, etc, computation out of the "main" >> area? >> > The intent is to use the same platform computation logic for all of the > places in the build where it is needed: setup.py, api-complete.py, > cli-complete.ksh, etc. This block came from setup.py so I made > api-complete.py consistent with setup.py. Huh. You should discuss this with Dan, see if you can find a place to put this. At the very least, I'd put it in a function which you then call from main. Then moving where the function is defined is a bit more straightforward. Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
