germinate should not examine all components in PPAs

I don't intend to add special cases for any archive, PPAs or not. However, clearly this is awkward since you want universe from the first archive in the list but want to disregard the fact that it doesn't exist in the second, and there's no way to configure that explicitly in germinate (it would probably be excessively verbose anyway). Perhaps a reasonable solution would be to raise an exception only if one of the specified components doesn't exist in *any* of the specified archives.

If by awkward you mean to say that PPAs can no longer be used with germinate when the seed uses more than just the main component.

For linaro, the impact that I see is we'll have to either fork germinate (yuck!) or setup our own archive for content that is now found in PPAs.

It would seem the usefulness of PPAs as a storage place for function that is not quite upstream yet something we want to pull into a reference build is gone.

I can't imagine that the Linaro project will be the only on affected by this but perhaps so.

Thanks for the consideration.

Um, you're overreacting a bit there - I'll fix this bug. I just meant
to say that special-casing PPAs is not the correct fix, and describe my
thinking on the correct solution.

With the genius of Steve Langasek, we've (well he) authored a proposed fix. I'm still testing it at the moment, but none the less I'll link the code here to the bug. You might find it acceptable.

This patch has been running well over the past couple of days. I haven't run into any issues for the variety of linaro-seed work that I do.

Hi Tom, thanks for the patch. I'll have to let Colin make the final determination, both because he knows the code much better than I do and because he has upload rights where I do not. I do have some questions after looking at your patch from 2011-02-15.

First: since you have a branch that contains your change, why not add a merge proposal for that? It would make it easier to review your patch, comment on it, and apply it. Also, they're easy to do! :)

Why do you add the empty string to the suffix list in the for loop? I see that the original code unconditionally adds the unsuffixed value of open_tag_file() if both .bz2 and .gz fail. Is that the real source of the bug (iow, open_tag_file(mirror, '') may not produce a non-None value)?

Nit: it's better to spell it "if tag_file is not None"

Nit: some of the indentation is a bit inconsistent. E.g. the "raise IOError" is too far to the right, although it doesn't break anything.

Nit: unless tag_files could be any false value, I generally like to use a more specific test. In this case, tag_files will always be a list, so testing for len() == 0 is better since it more closely matches your intention.

I know you're modifying the existing logic, but it seems like the only way to break out of the for-loop with tag_file not None is when open_tag_file() returns some non-None value. In that case, I wonder if this code makes more sense (completely untested of course ;):

        tag_files = []
        for mirror in mirrors:
            for suffix in (".bz2", ".gz", ""):
                tag_file = None
                try:
                    tag_file = open_tag_file(mirror, suffix)
                except (IOError, OSError):
                    pass
                else:
                    if tag_file is not None:
                        tag_files.append(tag_file)
                    break
        if len(tag_files) == 0:
            raise IOError
        return tag_files

Notice that I moved tag_files=None to inside the inner loop. I think the way the old code was written, it was possible to add a tag_file more than once inadvertently. Let's say mirrors=('A', 'B'). open_tag_file('A', '.gz') returns something non-None and it gets added to tag_files. Now mirror 'B' is processed, but all its suffixes raise IOError. When the inner loop for mirror='B' completes, tag_file is still set to the value it had during the mirror='A' iteration and it gets added twice.

I can't tell whether that's intended, but I kind of doubt it, thus the rewrite moving tag_file=None to the inner loop.

Or I could be totally wrong. :) As I said though, I'll let Colin make the final determination. Cheers, and thanks for your contribution!

On Thu, Mar 31, 2011 at 08:42:41PM -0000, Barry Warsaw wrote:
> Why do you add the empty string to the suffix list in the for loop? I
> see that the original code unconditionally adds the unsuffixed value of
> open_tag_file() if both .bz2 and .gz fail. Is that the real source of
> the bug (iow, open_tag_file(mirror, '') may not produce a non-None
> value)?

The bug is that open_tag_file raises an exception when the file is not
found, and we were only passing the exception for the ".bz2" and ".gz"
cases - which means that when we fall through to the uncompressed case, if
the file isn't there, we get an immediate exception where what we want is to
only raise an exception if we don't find a file for that component on *any*
mirror.

> Notice that I moved tag_files=None to inside the inner loop. I think
> the way the old code was written, it was possible to add a tag_file more
> than once inadvertently. Let's say mirrors=('A', 'B').
> open_tag_file('A', '.gz') returns something non-None and it gets added
> to tag_files. Now mirror 'B' is processed, but all its suffixes raise
> IOError. When the inner loop for mirror='B' completes, tag_file is
> still set to the value it had during the mirror='A' iteration and it
> gets added twice.

I'm failing to see how this would happen. tag_file is initialized to None
for each mirror, which is still the value it should have at the end of the
loop if open_tag_file() fails for all B, right?

Cheers,
--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
<email address hidden> <email address hidden>

I think it's clearer to move tag_files.append(tag_file) into the inner loop anyway; tag_file_open always either raises an exception or returns non-None, and this makes that clear.

I'll deal with this and Barry's other nits. Thanks!

This bug was fixed in the package germinate - 1.25

---------------
germinate (1.25) unstable; urgency=low

  * Only raise an exception from open_tag_files if an appropriate file is
    not found on any mirror (thanks, Tom Gall, Steve Langasek, and Barry
    Warsaw; LP: #717879).
 -- Colin Watson <email address hidden> Tue, 05 Apr 2011 14:40:15 +0000