Dan Price wrote:
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.
Well, remember that val may be a list for which all expansions are
needed. That is if PLATFORM -> [foo, bar, baz] in dyn_tok_conv then the
run path a/$PLATFORM/b becomes 3 paths, a/foo/b, a/bar/b, and a/baz/b.
It was this phenomena, combined with having the potential for multiple
tokens in a runpath that led me to the structure I used. I think I'd
lean towards keeping it as is because the above approach will walk a
string O(number of tokens defined * average number of values per token)
if there's a $ in it, while the other approach walks the string O(the
number of times a dynamic token is in the string). I doubt that either
takes long enough to make a substantial difference, but I'd wager that
most runpaths have 0 or 1 dynamic tokens.
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.
Mhmm, J offered a similar suggestion.
Perhaps clean_paths could be named something more expressive. You're
not cleaning them, really, you're doing variable substitution.
ok, does sub_dyn_toks work for you? If not, feel free to pick a name and
I'm likely to be fine with it.
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.
Yeah, I'll check if its been set and only set it if it hasn't been.
Alternatively, I'm happy checking for that when the CLI is parsed and
erroring out if ORIGIN is set using -D.
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 pulled the 3 commented blocks (and the supporting code needed to make
them work) out into a different test case which will be set to fail for now.
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.
Sure, I'll file a bug to remind me to do that.
Has Liane applied this and given it a try? That would probably be
worth checking before putback.
I'll see if she has the cycles to try it.
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
So, I was assuming that pkgdep was being run on unpublished manifests.
Specifically, on manifests whose payload was either NOHASH or a path to
the file since that's when I think it will actually be used,
before/during publishing.
pkgdep sees an action like:
file 46d582feff77d6155e4ee2346b852bc3b2829673
chash=5d01a515a26b1b352dc53000d321b60520625a55 elfarch=i386 elfbits=32
elfhash=ed68896d44ae9f653bf668f9c9354bb55e1c1e33 group=bin mode=0555
owner=root path=etc/fs/dev/mount pkg.csize=7642 pkg.size=18416
variant.arch=i386
And it goes, "OK, that's a file action with a path payload, let me go
find the file at 46d582feff77d6155e4ee2346b852bc3b2829673" because it
has no way of knowing that the hash isn't a valid path.
A few options seem obvious:
1) Say don't do that, only run pkgdep on manifests which haven't been
published.
2) If the payload path can't be found, check if the path set in the
action can be, if so use that and drive on.
3) Use heuristics to guess whether the payload path is a hash and use
the path if we think it is.
4) A combination of 2 and 3, first try the payload path. If it can't be
found, see whether the thing looks like a hex hash, if it does, try the
path in the action.
I'm leaning towards option 4, but could be convinced of the others.
Thanks for the review,
Brock
I wasn't really sure what to make of these new msgs, or what I
might do about them.
-dp
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
