On Fri, Jan 04, 2008 at 05:02:08PM -0500, James Falkner wrote:
>> - 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().
Okay. We should probably write some unit tests for this code.
>> 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
>
> ?
Yeah, something like that. But I'd rather concentrate on the "if we need
this at all" bit. The only purpose I see this file serving is as
documentation for the methods that implementors need to, er, implement.
But that's pretty much already done, unless you're planning a VMS port of
IPS as well.
>> __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)?
I believe so. Again, take a look at some of the standard Python modules
that have different modules for different platforms, but masquerade in a
single namespace. Remember, when you import names from another module,
they become part of your own namespace, and -- at the module level -- are
available for re-export.
>> 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().
Ah, okay.
>> 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.
Enums or plain ints, or something? On my gentoo box, they're #defines.
>> - 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..
Same value? SHT_SUNW_verdef and SHT_SUNW_verneed have the same value, or
the SUNW versions have the same value as the GNU versions?
I'd protect these with #ifdefs of the actual values, though:
#ifdef SHT_SUNW_verdef
case SHT_SUNW_verdef:
#endif
#ifdef SHT_GNU_verdef
case SHT_GNU_verdef:
#endif
blah blah blah
which is what I was getting at for the EM_ values, too. Again, they're
macros on my gentoo box; can't speak for ubuntu.
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
