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)
src/modules/flavor/python.py
line 72, typo "raised when a python to be analyzed" - a python file,
right?
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)
src/modules/publish/dependencies.py
line 992, nit, the comment describes pfmri, but we're using pkg_fmri
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?
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
--------------
src/modules/variant.py
line 366, typo "needs to be associated other information" should be
"needs to be associated with other..."
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
