On 05/23/11 15:52, Tim Foster wrote:
Hi Brock,
On Fri, 2011-05-20 at 16:19 -0700, Brock Pytlik wrote:
Webrev:
http://cr.opensolaris.org/~bpytlik/ips-16271-v1/
Looks good! Here's some comments:
src/modules/flavor/depthlimitedmf.py
line 215, probably worth a comment explaining why we're doing this
(similar for depthlimitedmf24.py)
Sure
src/modules/flavor/python.py
line 72, typo "raised when a python to be analyzed" - a python file,
right?
Well, the module had hopes of being a herpetologist, but had to settle...
line 84: perhaps "but contains a syntax error that prevents it from
being analyzed."
(I keep re-reading your wording and my suggestion, and I'm honestly not
sure which is better)
Sure, i like yours better :)
src/modules/publish/dependencies.py
line 992, nit, the comment describes pfmri, but we're using pkg_fmri
whoops
line 1041, hardcoding 5.11 looks weird, maybe include a comment
explaining why this is ok (PkgFmri disregards build_release if it's
already present, but needs it if it's not)
Other modules have # XXX comments saying that we need a better way to
default the build_release string, which might be worthwhile mentioning
here too?
sure, I'll add the XXX that's sprinkled around.
line 1042, 1077, this leaves us open to crashing on broken manifests,
eg.
---
file /usr/bin/ls path=/bin/ls mode=0644 owner=root group=sys
depend fmri=pkg://///// type=require
---
with your patch applied gets:
--------------
timf@linn[983] pkgdepend resolve dep
Traceback (most recent call last):
File "/usr/bin/pkgdepend", line 548, in<module>
__ret = main_func()
File "/usr/bin/pkgdepend", line 534, in main_func
return resolve(pargs, img_dir)
File "/usr/bin/pkgdepend", line 322, in resolve
prune_attrs=not verbose, use_system=use_system_to_resolve)
File
"/home/timf/projects/ips/tmp-pkg.hg/proto/root_i386/usr/lib/python2.6/vendor-packages/pkg/publish/dependencies.py",
line 1280, in resolve_deps
deps = combine(deps, pkg_vars, pfmri)
File
"/home/timf/projects/ips/tmp-pkg.hg/proto/root_i386/usr/lib/python2.6/vendor-packages/pkg/publish/dependencies.py",
line 1042, in combine
for f in cur_dep.attrlist("fmri")
File
"/home/timf/projects/ips/tmp-pkg.hg/proto/root_i386/usr/lib/python2.6/vendor-packages/pkg/fmri.py",
line 155, in __init__
detail=self.pkg_name)
IllegalFmri: Illegal FMRI 'pkg:///////': Invalid Package Name: ////
pkgdepend: This is an internal error in pkg(5) version a688f3d5cab6+. Please
let the
developers know about this problem by including the information above (and
this message) when filing a bug at:
https://defect.opensolaris.org/bz/enter_bug.cgi?product=pkg&component=cli
--------------
Good point. I'll catch the exceptions and include them in the error output.
src/modules/variant.py
line 366, typo "needs to be associated other information" should be
"needs to be associated with other..."
Cool.
Thanks for the review! I'll send out another webrev for the new error
i'll be introducing once I get that set up.
Brock
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
