Danek Duvall wrote:
Make sure you update copyrights to 2008.
Will do for all changed files.
Makefile:
- line 140: This doesn't seem to have been updated with the latest set of
python files. Please make sure to test Makefile installation on
Solaris.
This was current as of Feb 7 when the code review request was submitted.
Before pushing anything, the changes need to be merged with the latest
gate and updated again. I'll be doing that this week.
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? For example, this could be broken up into:
- portable module (first to go in, but not referenced)
- changes in main code to use portable (including Makefile)
- setup.py file
- changes to unit test cases
- pylint related changes
file.py:
- line 176: I don't want for the try and the import mechanisms to get run
every time we come through here. Perhaps the import can be done
module-level, as before, but with in a try block where an ImportError
can simply set a boolean that's tested here. Would that work?
Yes. Will make that change.
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.
generic.py:
- line 139: 'closer' seems to actually be called 'cleanup'.
Right. Comment fixed.
image.py:
- line 146: no need to wrap
Fixed.
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
(where path is saved off at the beginning of the method call.)
elfextract.c:
- you've got a handful of blank lines you added tabs to. You've done
this (or adding spaces to blank lines) in a bunch of other places, too.
I'll scan for these and eliminate them for the next review.
- line 377: why not the same sort of ifdef for verneed that you had for
verdef?
They should both be the same. Fixed.
server/transaction.py:
- line 220: same on the try/import here as in file.py.
Will fix.
- line 226: I think you should have a more specific message here ("ELF
support not available" or something).
This has been changed to:
request.send_response(501, "ELF files are not supported on this server")
(Once the merge is done with the gate, the 501 will be replaced with a
symbolic code.)
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. Do you mean not use classes here at all? Even if a
class method is decorated with @staticmethod, it still has to be called
using either the classname or an instance.
One alternative would be to replace the method definitions in
portable/__init.py__ with, for example:
get_group_by_name = impl.get_group_by_name
after the "impl" variable has been set.
This would eliminate the double function call and some program text. It
would remove the code-based documentation for the method signature. One
would need to dig into the implementation classes to see 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.
- line 243: unused variable "e".
Fixed.
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. Another option is
to catch the KeyError at the point where the method is called as it may
be easier to choose a reasonable value then.
If we do catch the KeyError in the portable classes, would it be
appropriate to return the uid and gid of the current user?
- line 128: "if passwd_stamp <= self.users_lastupdate.get(dir, 0):".
Note that you need to put "self." before all the "users_lastupdate"
mentions. Indeed, the code can't possibly work as-is.
Fixed.
portable/util.py:
- please use eight-space indents. (here and other places)
Fixed.
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.
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. With the unit test case
redesign done by Dan, this will be better because we don't need the ksh
versions - it can all be python. However, I'd like to refactor this
somehow so that the logic could just be in one place. But I wasn't sure
where to put it. Any ideas? I'm not aware of any common utility code
that is shared between setup.py and the test case logic. I suppose such
a module could be created.
harness_lib.ksh:
- Vile indentation.
Fixed. There were some tabs in there.
multiplatform.py:
- Is this used by pylint somehow? If so, should / can it be mentioned in
pylintrc?
Yes, this is used by pylint. I'll check into that.
I haven't reviewed setup.py or pylintrc in any detail. I'd like for Dan to
give his okay on the test bits before this comes back.
There have been significant changes in the test area since Feb 7, so
that merge will be a pretty major redo. I'll run that part by Dan for sure.
Thanks.
Tom
Danek
begin:vcard
fn:Tom Mueller
n:Mueller;Tom
org:Sun Microsystems, Inc.;Update Center/OpenInstaller Software
adr:;;21915 Hillandale Dr;Elkhorn;NE;68022;USA
email;internet:[EMAIL PROTECTED]
title:Senior Staff Engineer
tel;work:877-250-4011
tel;fax:877-250-4011
tel;home:402-916-9943
x-mozilla-html:TRUE
version:2.1
end:vcard
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
