Hi Brock,
On Thu, 2011-02-03 at 19:14 -0800, Brock Pytlik wrote:
> General comment:
> It might be worth breaking out the code to insert the default search
> path into one function and using it in all the modules. I think it's
> used in elf.py, depthlimited.py (and the 24 version though it can't be
> imported there probably). Since it's only in two places, leaving it as a
> copy is also fine with me.
No problem - I'll move this to pkg.flavor.base.
> pkgdepend.1.txt:
> I think some examples of using pkg.depend.bypass-generate would be
> helpful for users.
Sure, I'll add this.
> flavor/elf.py:
> 160: should this line still refer to kernel modules or is it used for
> all elf files?
Good catch, it's all elf files.
> publish/dependencies.py:
> 166: If I understand what ignore_bypass is actually doing, I think the
> docstring might be a bit clearer as:
> "... determines whether to respect tags which state that generation of
> dependences against certain files or directories should be bypassed. ..."
Yes, that's the intent - I'm happy to change the wording.
> 319-320:
> This is pretty much personal preference, but I would like it better if
> __verify_run_path returned the list of errors, and that was added to the
> existing elist if there were any errors on the return on line 320. If
> your preference runs differently, no worries, just a suggestion.
It's a fatal enough error that I just wanted to bail out there & then.
Otherwise, we'd continue on to do work, and end up with a bizzare
dependency generated (one with multiple expansions of our
$PKGDEPEND_RUNPATH token)
> 426-431:
> I've reread this a few times, and I still can't wrap my head around it.
> Maybe an example illustrating the complex case would make things clearer?
Sure. (I'll abbreviate the depend attributes below) This is for cases
where have a dependency like:
depend pkg.d.depend.file=passwd pkg.d.depend.file=shadow \
pkg.d.depend.path=foo \
pkg.d.depend.path=bar
The following files would satisfy the dependency:
foo/passwd
bar/passwd
foo/shadow
bar/shadow
But, say we want to bypass only "foo/passwd", we need to split off a new
dependency so that we instead generate the two dependencies:
depend pkg.d.depend.file=passwd pkg.d.depend.file=shadow \
pkg.d.depend.path=bar
depend pkg.d.depend.file=shadow \
pkg.d.depend.path=foo \
pkg.d.depend.path=bar
thus allowing the following files to satisfy the dependencies:
bar/passwd
foo/shadow
bar/shadow
Does that make sense?
> 470-472:
> I can't quite figure out what this comment's trying to explain.
Ok, it's where a user has specified bypasses that overlap - that is, a
user wants to bypass foo/shadow and */shadow -- the wildcarded entry
already matches "foo/shadow", so there's no need to go to extra effort
dealing with the "foo/shadow" case now (splitting off a new dependency
as explained above) when we can simply drop the pkg.debug.depend.file
entry for "shadow"
> 519: How could the basename not be in dep.base_names? (other than it
> being '*', but I think 448 ensures it can't be '*').
>
> 522: Same as 519 for dir and dep.run_paths
Hmm, I'll have a think about that.
> 525: this could move outside the loop?
Yep, thanks - I'll change that.
> testing:
> Probably worth testing the cli when a MUltipleDefaultRunpaths exception
> is raised.
Sure.
> This looks greats, thanks for taking on this on. I'm looking forward to
> seeing this land.
No worries - thanks for the review! I'll send out a webrev after the
weekend with the changes, and address your comments on 519/522.
cheers,
tim
> On 01/27/11 07:39 PM, Tim Foster wrote:
> > Hi all,
> >
> > I've got a webrev here that allows pkgdepend users to bypass generation
> > of dependencies against certain files and allows them to modify the
> > runpath that pkgdepend produces. I'd appreciate a code review please?
> >
> > http://cr.opensolaris.org/~timf/pkgdep-python-webrev/
> >
> > My plan is to get this change putback to the pkg-gate once reviewed,
> > then work with the ON gatekeepers to find a good time to force the
> > gatelings to upgrade to these pkg bits, via flag day.
> >
> > I'll then push the corresponding ON changes (I've a bit more work to do
> > for sparc, i386 is looking good) that use this new functionality, but
> > will probably leave pkgdepend error-checking in its current state.
> >
> > Once that's done, I'll merge and push the ON pkglint changes and enable
> > pkgdepend error checking at the same time.
> >
> > cheers,
> > tim
> >
> >
> > _______________________________________________
> > pkg-discuss mailing list
> > [email protected]
> > http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
>
> _______________________________________________
> pkg-discuss mailing list
> [email protected]
> http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
