Updated webrev:
http://cr.opensolaris.org/~bpytlik/ips-9290-v2/
Responses inline below.
Shawn Walker wrote:
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/manifest.py:
line 488: I can't remember if startswith() was slower than just
using an array slice and string comparison
I'm not sure. I think the startswith construction is easier to
understand. If this becomes a performance bottleneck, I'll explore other
options.
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 ?
Well, it's a functional module, so I like check_deps. I also in general
don't like putting things in __init__.py for several reasons. analyze
seems far to general for either its current or intended purposes. If you
prefer check_dependencies, I could live with that.
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?
Thanks for thinking of this, I forgot about pkg.portable. I've made that
change.
line 74: Why not use CachedManifest here?
Um, not sure why I would. This is a tool for developers. I would suspect
that each time the tool was run, the caches would be out of date since
they would have changed their manifest. Further, using the
cachedmanifest would deposit (from their persecptive) random extra files
into the directory they're working in. If/when this gets integrated into
server logic, I'm happy to use a CachedManifest for that code's entry point.
line 75-77: you need some try/except handling here in case the open
or close fails
It's caught in pkgdep.py. Well, the open is at least. Under what
conditions could closing a file I've opened for reading fail?
line 149, 153: try/except needed to handle failure?
Seems unnecessary to me for now. If making a tmp file fails, I'd like
the full trace back at least once so I can try to understand what's
happened.
line 154: i'd like to see this split out into a pkg.portable method
so it can be abstracted per-platform
Ok
line 54: maybe 'PythonModulePathMissing' would be clearer?
Sure
line 78, 79, 81, 84: should these be private properties?
Nope, if they could be protected I'd use that, but subclass
do/will/could need these variables.
line 122: missing r = ?
Yeah, thanks.
line 131: why not if self.dep_vars: return ... return ""
(simplifies it a tiny bit)?
sure
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.
It can't be private because subclasses modify it. Dep_key is different
from dep_path. dep_key is hashable, dep_path may not be.
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?
sure
lines 196-197: are these really intended to be public properties?
See above comments about lack of the notion of protected in python.
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.
If you have specific examples, please let me know. This was lifted
straight from solaris.py, so I would suspect the common cases are being
handled correctly.
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?
For the first pass, I'd like to keep this logic as similar to solaris.py
as possible. I'll happy to take these as bugs/rfes
modules/publish/dependencies/python.py:
line 50: python module instead of python file?
Well, it's the path to the file. I'm can use python module file, though
I prefer python file. To me, module implies some kind of encapsulation,
indexer, dependencies, etc are modules. Intuitively, I wouldn't call
client.py a "module" (setting aside that it probably is given python's
technical definitions). So, I'd prefer just python file.
modules/publish/depthlimitedmf.py:
Maybe this should be modules/publish/dependencies/python/mf.py?
I don't believe it belongs in the dependencies directory. That directory
is for the modules which generate dependencies. I also would like to
keep the longer name as it describes why we bothered to subclass module
finder.
src/pkgdep.py:
line 72: we tend to put this last
k
lines 138, 141: try/except needed?
Nope, it's already been opened and closed by the previous function call.
line 163: emsg() instead of print?
Sure
tests/api/t_check_deps.py:
line 26: can you put this after datetime since all the rest are in
order?
Thanks for reminding me, I need to clean this list out.
Looking good.
Cheers,
Thanks for taking a look.
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
