[Yum-devel] Patch: Hard-link packages that have already been downloaded.

[Ryan Lynch]

Hi, James,

Thanks for the feedback.  I understood most of your comments, and I
agree with everything I understood.  I need a couple of
clarifications, for the parts I didn't grasp.

I also had a chance to reflect on the design, and I made some changes.
 I started out with a "quick and dirty" approach, and I think the new
patch will show a little more thought.


On Fri, Sep 18, 2009 at 00:15, James Antill <james at fedoraproject.org> wrote:
>  I'm still not crazy about using -k (package kache dir?:), and now -o
> too, small option(s).

I've dropped the short options, entirely.


> 2. Why do you have pkgcache as a copy of cache?

I apologize, I have no idea what you're talking about, here--could you
illustrate the issue a little, for me?


> 4. I'm not sure you can just use 'path' for symlink's argument, might
> need to do realpath() ... or at least normalize it.

My original 'os.symlink()' usage seems to work OK under Python 2.4.3
(RHEL 5) and 2.6 (F11).  As far as I know, 'os.symlink()', it takes
the same parameters as 'os.link()', and uses them the same way.  For
comparison:

 * http://docs.python.org/library/os.html#os.link
 * http://docs.python.org/library/os.html#os.symlink

But think I may not understand your concern--can you clarify?  (Note
that in the


> 6. Might as well add the pkgs you download to the cache.

Again, I'm not quite sure what this means, since I'm not that familiar
with the YUM/RPM idioms.  Can you clarify?


Here are the details of the design changes, as they differ from the
previous patch:

First of all, In the 'if opts.downloadcomps or opts.downloadmd' block,
I removed the entire 'if not os.path.exists(local_repo_path)' block,
which repeats the exact check-and-create sequence that already takes
two blocks prior.  If the first block ends without the directory
existing, the program exits-on-error, so the 2nd block seems
irrelevant.

Second, I renamed 'checkPkg()' to 'VerifyPackage()', which is more
specific.  I also modified it to raise exceptions rather than return
tuples.

Third, I extracted most of the package-checking-and-downloading logic
into three functions, AlreadyInRepo()', 'LinkIntoRepo()',
'DownloadIntoRepo()', and converted the 'if-then-else' structures into
'try-except-else' blocks.

Now, I know that some people really hate using exceptions as control
structures, but I think that 'try-except' structures work better,
here.  Daniel's original idiom (capturing the returned values from
'checkPkg()' in variables, and checking the Boolean in an 'if' clause)
gets the job done.  But I couldn't figure out a way to use it to
implement the loop body elegantly.  The two best 'if-then'
arrangements I came up with were:

 - extra redundant 'os.path.exists()' checks (not very efficient), or
 - extra Boolean state flags (complicated the flow of control),

neither of which appealed to me as much as the current
'try-except-else usage.  And as much as I hate to fall back on this
justification, the 'try-except-else' logic just feels more Pythonic.
But I understand that reasonable people might disagree, so let me know
your preferences and I can change it, if required.

Finally, in the 'for pkg in download_list' loop, I made a few other
changes, mostly to simplify and remove redundancies:

 * Generate the 'n' counter variable via an 'enumate()' call wrapping
'download_list', in the 'for' expression, instead of explicitly
zeroing and incrementing 'n'.
 * Move the 'if opts.urls' block and the two variable assignments it
depends on ('repo' and 'remote') to the beginning of the loop body, so
we're not doing any extra work when we don't have to.
 * Remove the package size check that happens right before the
'repo.cache = 0' line, since we already thoroughly check any existing
package in the 'AlreadyInRepo()' call.


I realize that I might need to break this down into smaller patches.
Can you let me know whether I'm on the right track, first?

At any rate, thank you very much for your help, so far, too.

-Ryan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: yum-utils_reposync.py_hardlink_bravo.diff
Type: text/x-patch
Size: 12394 bytes
Desc: not available
URL: <http://lists.baseurl.org/pipermail/yum-devel/attachments/20090919/4575e232/attachment-0001.bin>