On Jun 18, 2009, at 7:52 PM, Brock Pytlik wrote:
Here's phase one of the dependency work. It takes a manifest and a
proto area and produces a list of the files the package depends on
that it doesn't deliver. Most of the focus was on establishing an
api for use, but a simple command line interface has been added as
well.
Webrev:
http://cr.opensolaris.org/~bpytlik/ips-9290-v1/
Bug:
9290 need a way to find the file dependencies of a package
http://defect.opensolaris.org/bz/show_bug.cgi?id=9290
modules/actions/generic.py:
line 395: > 80 characters
modules/client/variant.py:
* brief docstrings for the new functions appreciated
* unit tests under t_api would be nice
line 101: ancient nordic runtime error?
modules/manifest.py:
line 488: I can't remember if startswith() was slower than just
using an array slice and string comparison
modules/publish/check_deps.py:
It seems like these functions could be in modules/publish/
dependencies/__init__.py. check_deps is sort of an odd name for a
module. Maybe module/publish/analyze.py ?
lines 66, 72: it seems like this should be in pkg.portable or
something. Is there no way to get this information via the os or
portable modules? If not, can you put a common method into
pkg.portable for this so that other platforms can have their own
abstract method?
line 74: Why not use CachedManifest here?
line 75-77: you need some try/except handling here in case the
open or close fails
line 92: applies to all 'manf' parameters; i think it should be
clear that a Manifest object is expected and not the manifest file, or
string, etc.
line 113: I almost thought this was a copy/paste error because of
the subtle one-letter difference, a comment above each of these might
help those reading. Perhaps just capitalise 'manifest' or say
'Manifest object'?
line 116: a blank line inserted before this would help readability
line 117: need spaces around '=' after 'vars'
line 125: drop, or add one after line 85
line 149, 153: try/except needed to handle failure?
line 154: i'd like to see this split out into a pkg.portable method
so it can be abstracted per-platform
line 158: is this used?
line 174, 176: combine cases since they have the same handling?
lines 210-217: leftover debugging code? (notes the bpytlik path on
212)
line 219ff: looks like a lot of leftover testing/debugging code?
modules/publish/dependencies/base.py:
line 39: missing _()?
line 40, 53, 66, 142, 188: I think we generally put two blank lines
between class definitions
line 54: maybe 'PythonModulePathMissing' would be clearer?
line 52: missing _()?
line 64: missing _()?
line 68: drop the second ','
line 75: drop the ','
line 78, 79, 81, 84: should these be private properties?
line 122: missing r = ?
line 131: why not if self.dep_vars: return ... return
"" (simplifies it a tiny bit)?
line 149: seems like this should be private since you provide the
function on line 151 to access it, but at that point, you might as
well just rename the property dep_key? I'm probably missing something
bigger.
line 162: why not just:
norm_path = os.path.normpath(os.path.join("/", self.dep_path))
if norm_path in delivered_files:
return norm_path
real_path = os.path.realpath(norm_path)
if real_path in delivered_files:
return real_path
The above avoids some extra stats() I believe realpath() does in
some cases and is slightly more compact?
lines 172, 174: nit: I believe we generally place two spaces
between '.' in text
line 186: it doesn't seem as though the else: is necessary given
the explicit returns above
lines 196-197: are these really intended to be public properties?
modules/publish/dependencies/elf.py:
* The various calls to the elf module in here probably need try/
except wrapping. Some of them already have it.
modules/publish/dependencies/pound_bang.py:
What? Not shebang? j/k [1]
line 69: according to wikipedia, there could be an unknown amount
of optional whitespace before the '/'. Also, perhaps we should warn
about files that declare #! something but without a leading '/' to
make it absolute?
modules/publish/dependencies/python.py:
line 50: python module instead of python file?
modules/publish/depthlimitedmf.py:
Maybe this should be modules/publish/dependencies/python/mf.py?
src/pkgdep.py:
line 72: we tend to put this last
lines 138, 141: try/except needed?
line 163: emsg() instead of print?
tests/api/t_check_deps.py:
line 26: can you put this after datetime since all the rest are in
order?
line 155: s/""" /""""/
line 328: s/and/an/ ?
Looking good.
Cheers,
--
Shawn Walker
[1] http://en.wikipedia.org/wiki/Shebang_(Unix)
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
