On 11/30/10 07:58 PM, Tim Foster wrote:
Hi Brock,
On Fri, 2010-11-19 at 19:15 -0800, Brock Pytlik wrote:
Webrev:
http://cr.opensolaris.org/~bpytlik/ips-16013-v1/
Bugs:
16013 ON build noise from pkgdepend about perl's complicated symlinks
17412 pkgdepend should not be able to produce dependencies with build
versions set
17413 MultiplePackagesPathError should be variant aware
This wad allows pkgdepend to start taking symlinks into account when
computing dependencies. The logic turned out to be surprisingly hairy
(hence the nearly 700 lines of tests), so I'd appreciate efforts to try
and figure out if there are corner cases I missed.
Looks great. Aside from my pulling down the patch and playing with a
few example files (which we'd talked about off-list) I've included some
code review comments here. They're mostly nits really, what the code is
doing seems to be right.
Thanks for taking a look!
src/modules/flavor/base.py
line 244, 289 nit - "... finds the real paths that a path ..."
line 245, 290 nit - ", given a set of known links."
line 247, typo - "under which"
All fixed.
src/modules/publish/dependencies.py
line 52, perhaps a comment explaining what the Entries namedtuple is
would be good here? (I know it's also documented at line 462 and 591)
Ok, I've added a comment there.
line 60, nit - should be "two or more"?
line 453, nit - perhaps instead "This method maps a path to one or more
target paths, and the variants under which each target path can exist."
Sounds good :)
line 470, ".. for the file dependency for the original path", does that
mean the attributes of the file containing the path being resolved or
does it mean something else?
I think I'm a bit lost with the terms used in this docstring. We have
"real path", "original path", "path being evaluated" and "path which
needs to be resolved" (I'm throwing in "target path" above for good
measure :-) Now, I think some of those terms may refer to the same
things, but it'd be nice if they used the same words in that case.
Ok, I've changed some of this around, and generally converged on real path.
line 483: "... doesn't have broader ramifications" is a bit obscure,
(nit: should be "don't") - could you expand on what these are in the
comments to make the maintainers life easier? Perhaps just "changes made
here while checking variants are not reflected in actual variants on
each file's variant set"
Ok, I've updated the comment a litt.e
line 930: is there any reason to use itertools.chain() here given as
there's only one generator now in use?
Nope, thanks for catching that.
src/tests/cli/t_pkgdep.py
line 1471, this test is a wee bit long, breaking it up into sub-tests
(perhaps some with a shared set of setup methods) would make life a lot
easier for maintainers when trying to narrow down potential breakages
down the line.
The 17 block comments you have here look like good method boundaries.
[ I got a bit lost reviewing this, so I'd like to have another go if
you get a chance to break it up a bit? ]
Heh, ok, I'll break it up.
line 1502 and elsewhere, nit - could we have a few more blank lines?
I'll try.
line 1734, typo - "due to links"
line 1808, the block comment suggests this is the same as 2116. One's
testing internal dependency resolution, the other's testing external
resolution, and both are using two dimensions of variants, right?
Yep, I've adjusted the comments appropriately.
line 1968, typo - "delivered"
line 2116, typo - "dimensions", "expected"
line 2156, 2165, extra spaces at the end of the line, new line inserted
Thanks again for taking a look at this.
I'll break up the test suite and send it out again with the small fix I
ran into while looking at ON's dependencies resolution issues.
Brock
cheers,
tim
_______________________________________________
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
