On Mon 30 Nov 2009 at 11:39PM, Brock Pytlik wrote:
> Webrev:
> http://cr.opensolaris.org/~bpytlik/ips-12697-v1/
>
> Bugs:
> 12697 pkgdep throws an exception for $PLATFORM or $ISALIST in runpath
> 11306 pkgdep can traceback on versionexception
> 12790 pkgdep doesn't do the same conversion for 64 bit kernel paths that
> the importer does
> 12816 pkgdep gives misleading error message when payload file is missing
> 12896 pkgdep resolve should complain if two packages provide a depended
> on file
>
> This allows the user to provide the values that should replace dynamic
> tokens in a run path. It also addresses some other bugs in pkg deps
> logic and error handling.
elf.py: Would it be easier to use str.replace()?
That is to say (roughly):
for p in paths:
if "$" in p:
for vars in dyn_tok_conv:
p = p.replace(var, val)
if "$" in p:
error msg.
Also, minimally, seems like the "blah in blah" idiom could be coalesced
with the call to find() since find returns -1 if the value isn't found.
Perhaps clean_paths could be named something more expressive. You're
not cleaning them, really, you're doing variable substitution.
So you unconditionally overwrite $ORIGIN, then delete it. What if
the caller had specified it in their call to process_elf_dependencies?
Might it be better to respect it, or throw an error or something?
Or to take a copy of the caller's dyn_tok_conv[] and modify that?
Even just [[ assert "$ORIGIN" not in dyn_tok_conv" ]] would be nice.
> Note: I'm aware that three blocks of code are commented out in
> t_pkgdep.test_output. Instead of marking the test as failing, it seemed
> better to remove the three bits that are currently troublesome (those
> which search the image to resolve dependencies) so that the rest of the
> output can be validated. If this goes back as is, I'll file a bug to fix
> those three bits of the test cases.
Brock and I talked this over and he agreed to rework the test cases.
I looked at the rest, but I'm not sure how well I understand it all.
It looks OK to me. Thanks for the thorough test cases.
Something for you to do on a rainy day: dependencies.py is non-trivial
at this point. A "big theory" comment at the top to orient maintainers
and reviewers would be very helpful.
Has Liane applied this and given it a try? That would probably be
worth checking before putback.
I did try this on the build 127 version of pkgdep:
$ uname -a
SunOS xanadu 5.11 snv_127 i86pc i386 i86pc
$ pkgdep generate
/var/pkg/pkg/SUNWcs/0.5.11%2C5.11-0.127%3A291111T043632Z/manifest /
It output some stuff about sparc things which weren't found on
my x86 box. I then applied your patch, and got some good looking
deps, but also a lot of:
Couldn't find /436c1dd11526f76624f40c837081d082d2614fa8
I wasn't really sure what to make of these new msgs, or what I
might do about them.
-dp
--
Daniel Price, Solaris Kernel Engineering http://blogs.sun.com/dp
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
