Danek Duvall wrote:
[snip]
base.py:
- line 109: why this and not a merge()?
I don't understand how merge would be used here... If you mean
instead of the iteration over pkg_vars to add more things into vars,
it's because vars.merge(pkg_vars) will always equal pkg_vars.
Because vars.keys() is always a subset of pkg_vars.keys()?
Partially. More that the values for each key in vars is always a subset
of the matching key in pkg_vars.
- 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.
Yep, I was aware of this issue, I'll add the XXX note.
[ ... ] Actually, I don't think I will need to do the symlink resolution,
in this context. These are reference real files in the proto area(s) and
on disk. It would seem to me that we can actually rely on realpath and
normpath here. For doing the resolution against packages that actively
being published, I agree and know that eventually we'll need to deal with
that.
Okay; it just looked like you were operating on a thing that was in memory,
not on disk. And there's nothing in the code here that would prevent it
from doing that, except for the call to realpath(). I'd put a comment here
that you're making this assumption, and that it's okay.
Comment added :)
Note, though, that if you've got a (relative) link in a proto area, the
path you'll get back will be in the proto area, too, not something you'd
want to stick in a manifest. Not sure if this matters to you in this
method. Another assumption to document?
Sorry, this bit I didn't quite follow.
- line 175: why not simply return a value on error, and None otherwise?
Assuming you're talking about resolve, the reason is that other
functions return something other than self.ERROR paired with a
value.
What other functions are called in the same context, and where is that
context?
The resolve function of the elf module is the current example of
returning something other than self.ERROR. It can return self.WARNING
paired with a value.
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'm fine with moving this to modules/publish/depend/analyze.py. It was
called check_deps because, eventually, that's what it would up doing, but
I'll bow to the crowd on this one. However, it was my intention to have
the modules for each type of dependencies live in their own subdirectory
like the actions and bundle types do. Would you like that directory to be
modules/publish/depend/types/? And have analyze in the depend directory?
How would modules/publish/analyze_dependencies.py and
modules/publish/depend/* work?
There's another place we're going to need per-filetype code: content
hashes. Thinking ahead to that, I'd probably pull this out of publish
entirely. We've been talking about "tasting" files to determine what they
are, so perhaps pkg.flavor.{elf,script,smf,jar,etc}?
Ok, works for me.
- 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.
I don't have a good solution here. I thought this was better than
just punting on them, I'm happy to do that if it's preferred and
file a bug to try to get it more perfect in the future.
Punt for now, I think. The impact is limited. We can revisit this again
once we've decided how to deal with platforms more generally.
Ok, so just leave the $PLATFORM and $ISALIST literals in the paths and
refuse to find dependencies for them? Will do.
pound_bang.py:
- 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.
I've missed where you talked about the use of readlink() before. In
general, I'm confused by this comment so I'll find you next week to
get a better handle on what the problem is.
The previous reference was my comment for line 170 of base.py -- realpath
and readlink are closely related, and I must have forgotten what you'd used
there. At any rate, my point is that resolving through a symlink probably
should be done "right" rather than this hacky way which isn't going to work
on all platforms (because /bin is a separate directory on Linux). If Tom's
okay with this, though, then it's probably okay for now.
Well, it's what solaris.py does today. I realize that we want to be
compatible across platforms, but given the choice between having this
cause problems or missed/unresolved dependencies for the WOS or having
it play nice with linux, I think it makes more sense to focus on the
Solaris case for now. That said, this seems like something that's easy
to test for, so we could probably get it working in both places without
too much trouble.
- 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".
Yeah, I ended up doing that in my resolver wad. I went with generate
and resolve. I prefer generate over analyze, though if you care,
I'll change it.
I do care -- "generate" sounds like you're creating stuff out of whole
cloth; analyze sounds like you're figuring out what's already there. Maybe
others have a different feel for it.
I guess to me, analyze seems much more to be the equivalent of resolve,
or "generate + resolve". Analysis implies a coherent and useful result,
something I wouldn't really ascribe to the current production. Other
suggestions: produce, find, find-dependencies, dissect, inspect, index,
tabulate, develop?
Brock
Thanks,
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
