Thanks a lot. See my comments below.
On 07/19/13 04:06 PM, Bart Smaalders wrote:
* Pkghorst will need to become something else...
sure, do we just want to use Shawn's suggestion of 'pkgresurface' or go
into another round of bike-shedding ;)
Imho, the above has to many syllables and takes too long to type. Maybe
'pkgresurf' then. Or, since we are in California, 'pkgsurf'.
* Command prob needs a strategy comment in pkghorst.py
explaining the algorithm.
Just at the head of the file or how is this usually done?
progress.py: You use the word "revert" here; perhaps
we should use a better word that isn't
confusable w/ pkg revert?
Open to suggestions here. Going through the dictionary, candidates would
be 'rescind', 'retract, 'back-out', 'reverse'. If I see this right
'revoke' and 'withdraw' could mean the same thing but I guess these are
usually associated with something different.
Another idea is 're-version', which isn't even in the dictionary.
For me 'revert' sounded the best but I see that there might be some
concern about confusion.
pkghorst.py: line 79 missing comment
line 101: since -r and -s options are required,
they should not be enclosed in '[' ']'.
Alternatively, these could be positional
I think positional arguments are less clear than then current way. I'll
removed the  though.
line 124: help should comment that target repo
should only contain 1 version of each
That would be an interesting add-on in the future, where all packages in
between get removed as well. This shouldn't even be that difficult if
you just run the algorithm so many times until nothing changes anymore.
But I think this requires some more thought.
For now I edited the help text.
line 132: pkgdefs.EXIT_OOPS rather than 1 ?
line 165: Error message needs to be more solution
oriented; e.g. "repo must contain only
one version of each package" or ...
That should probably get changed to an assert, since *@latest should
never yield any unmatched packages, should it? This was in there just
for debugging and I could also remove it. Bottom line, nobody should
ever see this message.
line 188: perhaps suggest pkgrepo verify ?
line 215: please sort options
line 238: call to usage should have retcode=pkgdefs.EXIT_OK
line 277 - 290: code would be simpler if you used else clause
with for loop on 278; 'found' variable then
line 298: we discussed having a way of picking a surface
the reference repository. Is that a later
Not sure how you would do that. The problem is how do you specify a
surface that is complete except for *@latest? If you specify *@1.0 what
happens to all the packages which don't have a 1.0 version. Should they
be ignored? But maybe this could be used for something like: Only
resurface the X packages, for instance. Need to think about this some more.
line 303: code doesn't match (copied?)
from itertools import repeat
ref_pkgs = zip(latest_ref_pkg, repeat(intent))
if prefetch_manifests takes an iterable, you
also do itertools.izip() instead of zip.
In what case do we have exactly matching fmris?
That happens with all obsolete packages, for instance. They will have
exactly the same version in source and reference repo. Sure, they might
not be in a freshly-built consolidation repo, however, they need to be
line 348: this function would have better logic if it
explictly tested for things that were True
and returned False by default like the first
line 352: what happens if someone adds name=fred to a file
action? E.g. is this supposed to be for set actions
That is still up for debate. I just implemented one way which allows us
to ignore certain actions. We still have to define the scope of this
feature. But yes, at the moment, you could have a name attribute in a
file action and it would make pkghorst ignore the action.
line 379 - 395: The use_ref_pkg flag is kind of ugly. Why
def use_refs(acts, deps):
for a in acts:
if not use_ref(a, deps):
and you get instead:
if not use_refs(ta, tdeps) or not use_refs(ra, rpeps):
line 476 - 482: return True out of loop, False at end.
line 551: is error msg correct?
line 572: where is C read (used)?
line 527: Consider running w/ su_wrap=false and make
sure no files get opened writable.
Not sure I can follow. I guess you mean su_wrap=True but I still don't
understand how I can test if files get opened only in read mode. Are we
doing this in some other test case for an example?
how about an additional test? what happens when you run
pkghorst again on the same repo? Nothing should happen,
right? Is that actually the case?
Yes it is. I added the test case and also changed the handling in the
tool slightly. I didn't check for nothing-to-do and did unnecessary work.
Should we test -p option?
I added a test to check that an unspecified publisher isn't touched. Do
you think it makes sense to test the whole set of combinations of
[pkg-discuss] Re: small code review: 16992741 New publication tool, which reverts pkgs in repo if content hasn't changed