On Sun, Nov 15, 2009 at 3:24 PM, Jonathan Lange <email address hidden> wrote:

> On Sun, Nov 15, 2009 at 2:35 PM, Michael Nelson
> <email address hidden> wrote:
> > Michael Nelson has proposed merging
> lp:~michael.nelson/launchpad/443353-api-builds-from-private into
> lp:launchpad.
> >
> >    Requested reviews:
> >    Canonical Launchpad Engineering (launchpad)
> > Related bugs:
> >  #443353 API does not include Builds for sources that were sync'd from
> private PPAs
> >  https://bugs.launchpad.net/bugs/443353
> >
> >
> > Overview
> > ========
> >
> > This branch fixes bug 443353 by ensuring that getBuildsForSources() will
> include builds that were originally built in a different archive context,
> but have since had binaries copied into the source archive context.
> >
> > Issues
> > ======
> >
> > There are two main issues with this branch IMO.
> >
> > 1. Ensuring that the result of the union was ordered correctly is
> hackish, and dependent on a storm implementation detail (that columns in SQL
> queries for a certain table are ordered alphabetically).
> >
> > 2. Testing the correct ordering in the doctest isn't so readable. It
> could be better to print out the results instead, but on the other hand, I
> didn't want the test to be dependent on database ids. Previously the test
> simply ensured the last item was the one expected - I've updated this to
> instead assure that the complete result is sorted as expected.
> >
> > Any suggestions welcome!
> >
>
> Hi Michael,
>
> Thanks for fixing this -- on a Sunday even!
>
>
And thanks for reviewing even on a Sunday :)



> I'm generally OK with this branch, but would like another opportunity
> to have a look at it and maybe to talk with you face-to-face.
>
>
Yes, I was keen for pointers as I wasn't happy with the hack... after
talking with both Jamu and Gustavo, I think we've got a much better solution
(although it still feels it should not be necessary).



> jml
>
> > Testing/QA
> > ==========
> >
> > To test, run:
> > bin/test -vvt doc/publishing.txt
> >
> > To QA:
> >
> > Visit:
> > https://edge.launchpad.net/ubuntu/+source/openjdk-6
> >
> > and expand the intrepid 6b12-0ubuntu6.6 security release. Currently this
> only displays two builds, where as it should display all the builds listed
> at:
> >
> > https://edge.launchpad.net/ubuntu/+source/openjdk-6/6b12-0ubuntu6.6
> >
> >
> >
> >
> > --
> >
> https://code.launchpad.net/~michael.nelson/launchpad/443353-api-builds-from-private/+merge/14896<https://code.launchpad.net/%7Emichael.nelson/launchpad/443353-api-builds-from-private/+merge/14896>
> > Your team Launchpad code reviewers from Canonical is subscribed to branch
> lp:launchpad.
> >
> > === modified file 'lib/lp/soyuz/doc/publishing.txt'
> > --- lib/lp/soyuz/doc/publishing.txt     2009-11-05 10:51:36 +0000
> > +++ lib/lp/soyuz/doc/publishing.txt     2009-11-15 20:35:29 +0000
> > @@ -1055,22 +1055,53 @@
> >  each build found.
> >
> >     >>> cprov_builds.count()
> > -    7
> > +    8
> >
> >  The `ResultSet` is ordered by ascending
> >  `SourcePackagePublishingHistory.id` and ascending
> >  `DistroArchseries.architecturetag` in this order.
> >
> > -    >>> source_pub, build, arch = cprov_builds.last()
> > -
> > -    >>> print source_pub.displayname
> > -    foo 666 in breezy-autotest
> > -
> > -    >>> print build.title
> > -    i386 build of foo 666 in ubuntutest breezy-autotest RELEASE
> > -
> > -    >>> print arch.displayname
> > -    ubuntutest Breezy Badger Autotest i386
> > +    # The easiest thing we can do here (without printing ids)
> > +    # is to show that sorting a list of the resulting ids+tags does not
> > +    # modify the list.
> > +    >>> from copy import copy
> > +    >>> ids_and_tags = [(pub.id, arch.architecturetag)
> > +    ...     for pub, build, arch in cprov_builds]
> > +    >>> ids_and_tags_sorted = copy(ids_and_tags)
> > +    >>> ids_and_tags_sorted.sort()
> > +    >>> ids_and_tags == ids_and_tags_sorted
> > +    True
> > +
>
> You don't have to use copy() and .sort(), you can use sorted()
> instead. That would be slightly nicer.
>
>
Done.



> Nicer still would be an assertSorted() method called from a unit test :P
>
>
We're in the process of moving lots of soyuz non-story doc-test into
unit-tests :)


> > +If a source package is copied from another archive (including the
> > +binaries), then these builds will also be included in the result (even
> > +though they were build in a different archive context).
> > +
>
> Saying something about _why_ would greatly improve this as a document.
> Can you please add something like that?
>
>
I added an example which demonstrates the main situation requiring this
change.



> > +    # Create a new PPA and publish a source with some builds
> > +    # and binaries.
> > +    >>> other_ppa = factory.makeArchive()
> > +    >>> binaries = test_publisher.getPubBinaries(archive=other_ppa)
> > +    >>> build = binaries[0].binarypackagerelease.build
> > +    >>> source_pub = build.sourcepackagerelease.publishings[0]
> > +
>
> Can you please add some comments here about how the context of the
> build is _not_ Celso's PPA? This can be easily lost in skimming the
> docs.
>
>
Done - added a comment and demo'd by printing the archive name associated
with the build.


> > +    # Copy the source into Celso's PPA, ensuring that the binaries
> > +    # are alse published there.
> > +    >>> source_pub_cprov = source_pub.copyTo(
> > +    ...     source_pub.distroseries, source_pub.pocket,
> > +    ...     cprov.archive)
> > +    >>> binaries_cprov = test_publisher.publishBinaryInArchive(
> > +    ...     binaries[0].binarypackagerelease, cprov.archive)
> > +
> > +Now we will see an extra source in Celso's PPA as well as an extra
> > +build - even though the build's context is not Celso's PPA.
> > +
> > +    >>> cprov_sources_new = cprov.archive.getPublishedSources()
> > +    >>> cprov_sources_new.count()
> > +    9
> > +
> > +    >>> cprov_builds_new = publishing_set.getBuildsForSources(
> > +    ...     cprov_sources_new)
> > +    >>> cprov_builds_new.count()
> > +    9
> >
>
> Maybe it's worth adding a note in the text saying "(It was 8 befgore
> we started)".
>
>
Done.


> >  Next we'll create two sources with two builds each (the
> SoyuzTestPublisher
> >  default) and show that the number of unpublished builds for these
> sources
> >
> > === modified file 'lib/lp/soyuz/model/publishing.py'
> > --- lib/lp/soyuz/model/publishing.py    2009-10-29 11:55:35 +0000
> > +++ lib/lp/soyuz/model/publishing.py    2009-11-15 20:35:29 +0000
> > @@ -40,6 +40,7 @@
> >  from canonical.database.enumcol import EnumCol
> >  from lp.registry.interfaces.pocket import PackagePublishingPocket
> >  from lp.soyuz.model.binarypackagename import BinaryPackageName
> > +from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
> >  from lp.soyuz.model.files import (
> >     BinaryPackageFile, SourcePackageReleaseFile)
> >  from canonical.launchpad.database.librarian import (
> > @@ -1233,20 +1234,59 @@
> >                 Build.buildstate.is_in(build_states))
> >
> >         store = getUtility(IStoreSelector).get(MAIN_STORE,
> DEFAULT_FLAVOR)
> > -        result_set = store.find(
> > -            (SourcePackagePublishingHistory, Build, DistroArchSeries),
> > +        find_spec = (
> > +            SourcePackagePublishingHistory, Build, DistroArchSeries)
> > +
> > +        # We'll be looking for builds in the same distroseries as the
> > +        # SPPH for the same release.
> > +        builds_for_distroseries_expr = (
> >             Build.distroarchseriesID == DistroArchSeries.id,
> > +            SourcePackagePublishingHistory.distroseriesID ==
> > +                DistroArchSeries.distroseriesID,
> > +            SourcePackagePublishingHistory.sourcepackagereleaseID ==
> > +                Build.sourcepackagereleaseID,
> > +            In(SourcePackagePublishingHistory.id,
> source_publication_ids)
> > +            )
> > +
>
> I think it's better style to close parens on the same line. PEP 8
> agrees with me, IIRC.
>


As we discussed, I think you mistook the above for function arguments rather
than a tuple. Our style guide uses the above (closing paren on following
line inline with the contained items).



>
> > +        # First, we'll find the builds that were built in the same
> > +        # archive context as the published sources.
> > +        builds_in_same_archive = store.find(
> > +            find_spec,
> > +            builds_for_distroseries_expr,
> >             SourcePackagePublishingHistory.archiveID == Build.archiveID,
> > -            SourcePackagePublishingHistory.distroseriesID ==
> > -                DistroArchSeries.distroseriesID,
> > -            SourcePackagePublishingHistory.sourcepackagereleaseID ==
> > -                Build.sourcepackagereleaseID,
> > -            In(SourcePackagePublishingHistory.id,
> source_publication_ids),
> > -            *extra_exprs)
> > -
> > +            *extra_exprs)
> > +
> > +        # Next get all the builds that have a binary published in the
> > +        # same archive... even though the build was not built in
> > +        # the same context archive.
> > +        builds_copied_into_archive = store.find(
> > +            find_spec,
> > +            builds_for_distroseries_expr,
> > +            SourcePackagePublishingHistory.archiveID != Build.archiveID,
> > +            BinaryPackagePublishingHistory.archive == Build.archiveID,
> > +            BinaryPackagePublishingHistory.binarypackagerelease ==
> > +                BinaryPackageRelease.id,
> > +            BinaryPackageRelease.build == Build.id,
> > +            *extra_exprs)
> > +
> > +        result_set = builds_copied_into_archive.union(
> > +            builds_in_same_archive).config(distinct=True)
> > +
> > +        # XXX 2009-11-15 Michael Nelson bug=366043. It is not possible
> > +        # to sort by the name `SourcePackagePublishingHistory.id` as
> after
> > +        # the union there are no tables. Nor can we sort by ambiguous
> `id`
> > +        # as in this case there are 3 id columns in the result.
> Specifying
> > +        # the column index is the only option that I can find. So we're
> > +        # relying on an implementation detail of Storm that it lists
> > +        # columns in alphabetical order in sql queries.
>
> I don't get what you mean by "ambiguous `id`". Surely the ID columns
> can be referred to by their table name as before?
>

Not after the union - it just returns a result set of columns (as in the
general case, the corresponding items in the result set do not even need to
be from the same table (ie. select Product.id from products UNION select
Item.id from Items).


>
> It might be a good idea to grab Jamu and see if he can solve your problem.
>
> > +        sql_columns =
> SourcePackagePublishingHistory._storm_columns.values()
> > +        sql_column_names = [column.name for column in sql_columns]
> > +        sql_column_names.sort()
> > +
>
> That's pretty terrible, but if that's what you've got to do, then I'm OK.
>

Agreed (about it being terrible). So Jamu mentioned ClassAlias, but I
couldn't see how that could help, and then Gustavo took a look, he agreed
that the ClassAlias wouldn't help here and that there isn't anything other
than a workaround - but he provided a better workaround - yay.

Incremental attached. Thanks Jonathan.


> You can use sorted() here to make things a little better.
>
> > +        # SQL order by uses 1-based column numbers.
> > +        source_pub_id_col_number = sql_column_names.index('id') + 1
>
> Given that all of this is a workaround for a storm bug, maybe it's a
> good idea to put it into a small helper function?
>
> >         result_set.order_by(
> > -            SourcePackagePublishingHistory.id,
> > -            DistroArchSeries.architecturetag)
> > +            source_pub_id_col_number, DistroArchSeries.architecturetag)
> >
> >         return result_set
> >
> --
>
> https://code.launchpad.net/~michael.nelson/launchpad/443353-api-builds-from-private/+merge/14896<https://code.launchpad.net/%7Emichael.nelson/launchpad/443353-api-builds-from-private/+merge/14896>
> You are the owner of
> lp:~michael.nelson/launchpad/443353-api-builds-from-private.
>