On Tue, Jun 23, 2009 at 06:01:11PM -0700, Brock Pytlik wrote:
> http://cr.opensolaris.org/~bpytlik/ips-9290-v2/
You have a lot of code that deals with variants, but you ignore facets
entirely. Don't facets have some impact on dependency analysis, and hence
shouldn't many of the uses of "variant" in your code really be "varcet"? I
can understand that facets aren't fully implemented and testable, but I'd
hate to see them be introduced and have a bunch of variable and method
names in this wad either need to change or be misleading.
There appears to be no man page.
Just for comparison, the analyzer I'd had spat out actions like this:
file d4c3eb7039ba48253b69a86ed2663124e3f50832 group=bin mode=0755 \
owner=root path=usr/lib/amd64/libbrand.so.1
pkg.tmp.elf.dependencies=libc.so.1 \
pkg.tmp.elf.dependencies=libxml2.so.2
pkg.tmp.elf.dependencies.libc.so.1=SUNW_1.22 \
pkg.tmp.elf.dependencies.libc.so.1=SUNW_1.19
pkg.tmp.elf.dependencies.libc.so.1=SUNW_1.1 \
pkg.tmp.elf.dependencies.libc.so.1=SUNW_0.9
pkg.tmp.elf.dependencies.libc.so.1=SUNW_0.8 \
pkg.tmp.elf.dependencies.libc.so.1=SUNW_0.7
pkg.tmp.elf.dependencies.libxml2.so.2=SUNW_1.3 \
pkg.tmp.elf.dependencies.libxml2.so.2=SUNW_1.1
pkg.tmp.elf.info.arch=i386 \
pkg.tmp.elf.info.bits=64 pkg.tmp.elf.info.end=lsb
pkg.tmp.elf.info.osabi=none \
pkg.tmp.elf.info.type=so pkg.tmp.elf.versions=SUNWprivate
pkg.tmp.filetype=elf
and the resolver ended up tacking on
pkg.tmp.introduced_deps=pkg:/[email protected],5.11-0.101 \
pkg.tmp.introduced_deps=pkg:/[email protected],5.11-0.101
in addition to the actual depend actions. Storing all this info in depend
actions is fine, and you certainly don't need all the ELF versioning bits I
had (someday, maybe, but not now) nor all the other ELF data; I just threw
everything in. However, you might want to allow for a richer set of
"reason"s than just a single piece of information. I don't see anything
preventing extension of that in the future, but keep it in mind.
manifest.py:
- Given that so far this file doesn't have any client-specific imports,
it might be nice not to do that.
- This isn't your issue, but get_variants() is defined twice in the file.
- get_variants() returns a list. Perhaps get_all_variants() could return
a dictionary? Also, docstring please.
- Would you consider
return dict((
(name, self.attributes[name])
for name in self.attributes
if name.startswith("variant.")
))
variant.py:
- line 54: not your bug, but "__ketset" should be "__keyset"
- line 83: "two sets of variants" or "two variants" or perhaps "two
Variants objects"? Could you also make the docstrings correctly
punctuated sentences, and drop the spaces at the beginning and ends of
the strings? I don't know where that style came from, but it's wrong
wrong wrong.
- line 86: self[name] = list(set(var[name] + self[name]))? Or perhaps
replace the whole if statement with
self[name] = list(set(var[name] + self.get(name, [])))
which leads to
self.update(dict((
(name, list(set(var[name] + self.get(name, []))))
for name in var
)))
- line 91: sets have an issubset() method
- line 100: sets have a difference() method
- line 103: isn't set(self.keys()) equivalent to self.__keyset? Can you
access var.__keyset?
- line 104: why?
- line 111: fmris have a tuple() method; perhaps this should follow?
- line 112: Ah, but what exactly *is* the "tuple form of self"?
- line 113: I'm guessing "return tuple(self.items())" isn't quite what
you want. Perhaps
return sum(self.iteritems(), ())
that won't do the sorting for you, but you could enforce the sorting
elsewhere ...
os_sunos.py:
- You're not going to need the power of the full magic database, and I
don't think the output of the file command is Committed, so I think I'd
rather see you do the magic without forking off another process.
Should be faster, too.
- You're never removing t_path, are you?
base.py:
- line 36: what is "fp"? File pointer?
- lines 42, 56: shouldn't you have a base exception class and have these
guys defined in elf.py and python.py, respectively, inheriting from
that base class?
- line 84: does "if vs" work?
- line 109: why this and not a merge()?
- line 121: "Depedency" -> "Dependency"
- line 123: I might do
r = cmp(...) or \
cmp(...) or \
cmp(...)
- line 125: so dep_key() and action_path() return very very different
things -- the former (according to the docstring) is the target of the
dependency, while the latter is the source of the dependency. Is it
appropriate to sort dependencies by these two disparate things?
- line 145: as we discussed in the hallway yesterday, this should simply
be a "depend" action, and all the attributes attached to it should have
an appropriate prefix (and you might as well throw on type=require).
- line 153: "dep_path" is what, the path of the file that generated the
dependency, or the path of the file that resolves the dependency?
- line 161: "an index"?
- line 165: a KeyError waiting to happen? Or should this simply be a
named input variable?
- line 170: You might want to XXX-note that this is only useful when the
installed system matches delivered_files. Eventually, you'll have to
do the symlink resolution yourself.
- line 175: why not simply return a value on error, and None otherwise?
- line 186: "if p"?
elf.py:
- line 74: given that pkgsend is now growing multiple -d options, it
might make sense to support multiple proto areas. You might check with
Liane to see if she'd want this (not that it can't be changed later,
but ...).
- line 117: Shouldn't "p" be "tp" here? Or use "p" through the rest of
the loop?
- line 123-125, 150-151: presumably these are no longer relevant given
that you're now doing $PLATFORM and $ISALIST processing. But I'm not
entirely sure how that processing is supposed to work, so the questions
are still unanswered.
- Worth considering replacing the RuntimeErrors with something more
sophisticated later on.
check_deps.py
- You've got some terminology confusion here. The module is named
"check_deps" but on lines 44 and 127, you define functions talking
about "list"ing dependencies, and implicit ones at that. I think I
agree with Shawn here -- the module should be part of
pkg.publish.dependencies (which should perhaps be pkg.publish.depend),
and it should be about "analyzing" packages or manifests to determine
what its dependencies are. I don't understand what your objection is
to putting things in __init__.py or to using the word "analyze" in this
context. I would put a function in the "pkg.publish.depend" module
called "analyze", which would depend on two private functions called
"__remove_internal_deps()" and "__analyze()" (as the latter does most
of the real work of the front-end "analyze()")
- I also think "show_internal_deps" is poorly named. Perhaps
"remove_internal_deps" and switch the meaning of the flag?
- line 55, 58: how are plat and isalist supposed to work? A dependency
on $PLATFORM is evaluated at load-time, so if a module under $PLATFORM
is delivered in different packages depending on $PLATFORM (I can't
actually find an example), and something in /kernel/drv depends on it,
how do you resolve which package it's supposed to come from? If you
(or whoever is running your code) chooses one or the other, then you
end up with a dependency that is correct on one platform and not on
another (and indeed doesn't actually get that dependency resolved).
You could depend on both, which is probably the sanest answer, since
it's simple, and shouldn't be a big deal for minimization. Or you
could facet it. Similarly for $ISALIST.
- line 70: This will always return a string. Perhaps you mean to split
this on ":" or something?
- line 78: m.get_all_variants() already returns a Variants object.
- line 85: We've been using "mfst" for the four-letter abbreviation for
"manifest".
- line 89: "Depedency" -> "Dependency"
- line 149: four lines for this statement, please.
- line 166: I think this really needs to look more like a dispatcher.
Your internal file tasting mechanism should return something that can
be plugged into a table that maps file types to processing methods, and
you should be catching base-class exceptions.
- line 198: so ... what does "missing" mean in this context? That the
analyzer for the file type is missing?
pound_bang.py:
- I don't care too much, but I think "shebang" is a better term for this.
If that's too arcane, then perhaps "script".
- line 54: I think "!=" is clearer.
- line 69: drop the slash. It can be separated from the shebang by any
amount of whitespace, and needn't exist at all.
- line 70: usedlist? I know you were looking for solaris.py
compatibility, but that's going too far. :)
- line 71: single-line comments don't wrap. Either it fits on the line,
or put it on the line before.
- line 73: doing this right and dealing with the use of readlink() I
mentioned before may very well be the same thing. I'm not sure this is
worth doing wrong. It certainly doesn't hold on Linux. At the very
least, there should be a trailing slash to make sure it's not
/binsomething.
- line 85: shouldn't this be "return [], []", or is the tuple return on
line 84 incorrect? process_elf_dependencies() returns a single list,
but process_python_dependencies() returns a tuple of lists.
depthlimitedmf.py:
- line 11: not used?
- line 21: is there anything more to this method than adding the first
line to the one from the parent class? Similarly for load_module()?
pkgdep.py:
- My guess is that you'll usually want to emit the original manifest in
addition to the dependencies, rather than usually omitting it.
- Given that you're not treating "missing" files as error messages, they
should probably be emitted as actions, not bare strings. But if I
understand what's these actually are, I'd guess that a) they should be
sent to stderr, and b) they should only be emitted in a verbose mode,
because mostly you won't care.
- I would expect that the resolver is run from the same command as the
analyzer, which would suggest two subcommands for pkgdep: "analyze" and
"resolve".
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
