Skip to main content

[pkg-discuss] Re: small code review: 16992741 New publication tool, which reverts pkgs in repo if content hasn't changed

  • From: Bart Smaalders < >
  • To: Erik Trauschke < >
  • Cc:
  • Subject: [pkg-discuss] Re: small code review: 16992741 New publication tool, which reverts pkgs in repo if content hasn't changed
  • Date: Mon, 05 Aug 2013 13:39:50 -0700

On 07/22/13 07:45 AM, Erik Trauschke wrote:
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

fixed

          line 101: since -r and -s options are required,
                     they should not be enclosed in '[' ']'.
                        Alternatively, these could be positional
                arguments.

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
                pkg.

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 ?

fixed

          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
unneeded.

all fixed


              line 298: we discussed having a way of picking a surface
out of
                     the reference repository.  Is that a later
enhancement?

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?)
                        comment

              lin3 305-307:

                    why not:

                        from itertools import repeat
                        ...
                    ref_pkgs = zip(latest_ref_pkg, repeat(intent))

                        if prefetch_manifests takes an iterable, you
could
                also do itertools.izip() instead of zip.

all fixed

          line 337:
                        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
handled correctly.

              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
                clause.

fixed

              line 352: what happens if someone adds name=fred to a file
                    action? E.g. is this supposed to be for set actions
                only?

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
                        not

                def use_refs(acts, deps):
                       for a in acts:
                    if not use_ref(a, deps):
                          return False
                return True

                        and you get instead:

                if not use_refs(ta, tdeps) or not use_refs(ra, rpeps):
                    continue

              line 476 - 482: return True out of loop, False at end.

              line 551: is error msg correct?

              line 572: where is C read (used)?

all fixed.

t_pkghorst.py:
          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
publisher configurations?


Erik


OK; I'm happy enough with this wad at this point; we prob. need to
get some more user feedback.

- Bart

--
Bart Smaalders                  Solaris Core OS

  http://blogs.oracle.com/barts
"You will contribute more with Mercurial than with Thunderbird."
"Civilization advances by extending the number of important
 operations which we can perform without thinking about them."


[pkg-discuss] Re: small code review: 16992741 New publication tool, which reverts pkgs in repo if content hasn't changed

Bart Smaalders 08/05/2013

[pkg-discuss] Re: small code review: 16992741 New publication tool, which reverts pkgs in repo if content hasn't changed

Erik Trauschke 08/06/2013

[pkg-discuss] Re: small code review: 16992741 New publication tool, which reverts pkgs in repo if content hasn't changed

Shawn Walker 08/08/2013

[pkg-discuss] Re: small code review: 16992741 New publication tool, which reverts pkgs in repo if content hasn't changed

Erik Trauschke 08/09/2013
 
 
Close
loading
Please Confirm
Close