Merge lp:~mvo/pkgme-devportal/i386-only-for-now into lp:pkgme-devportal

[Michael Vogt]

Makes supported architectures configurable, and default to i386 for now.

This is a branch that make the architectures configurable and defaults to "i386" for now. It does affect the database as the previous default for the architectures included "all". AFAICT this should cause any regressions as in the database it was overriden.

Reference is bug #832128.

This will eventually lead to lp:~mvo/pkgme-devportal/more-architecture-when-scanning-binaries where the architecture configuration will actually mean that the database is filled with the additional architectures.

[James Westby (james-w)]

Hi Michael,

I'm not really sure what "supported" architectures mean here, could you help me
out?

37 + # XXX: mvo: it seems we don't need "all" here as we are interessted
38 + # in binary symbols only?

I'm not sure what that means, but it sounds like something we should have an
answer for before we land this?

53 +ARCHITECTURES = load_configuration().options.architectures_supported

Setting a module-level variable based on reading a config file is bad form,
because it will read the config file at import, which means it's hard
to write tests for example. Querying the config at use would be better.

Thanks,

James

review: Needs Information

[lp:~mvo/pkgme-devportal/i386-only-for-now]

61. By Michael Vogt on 2012-07-19

do not load ARCHITECTURES configuration on a module level but instead in each function that needs it, thanks to james_w for this suggestion

[Michael Vogt (mvo)]

On Thu, Jul 19, 2012 at 05:15:33PM -0000, James Westby wrote:
> Review: Needs Information
>
> Hi Michael,
Hi James,

> I'm not really sure what "supported" architectures mean here, could you help me
> out?

Just to clarify, do you mean that the string for the configuration
option in the configglue part is not clear? Or that its overall not
clear?

What I mean with "supported" for the architectures is that those are
the architectures we have symbols for in the database and expect to
generate packages for. Do you think a different name would make it
clearer? Maybe "known" or something like this?

> 37 + # XXX: mvo: it seems we don't need "all" here as we are interessted
> 38 + # in binary symbols only?
>
> I'm not sure what that means, but it sounds like something we should have an
> answer for before we land this?

Indeed, its based on the previous code that remove "all" from the list
of ARCHITECTURES for the launchpad querry. And it made me wonder if we
need "all" at all. I think not, but maybe I'm overlooking something.

> 53 +ARCHITECTURES = load_configuration().options.architectures_supported
>
> Setting a module-level variable based on reading a config file is bad form,
> because it will read the config file at import, which means it's hard
> to write tests for example. Querying the config at use would be better.

This is fixed now in my branch.

Thinking a bit more about the branch it may actually not be that
useful, I mainly did it in preparation for the
lp:~mvo/pkgme-devportal/raise-on-unsupported-architectures

But given that the database currently does not have a architecture
field afaik it may well be a bit pointless to make it configurable
before it is actually configurable. So I'm fine putting that branch on
hold for now until the rest of the code is updated to work with
multiple architectures too.

Cheers,
 Michael

[James Westby (james-w)]

On Fri, 20 Jul 2012 06:47:26 -0000, Michael Vogt <email address hidden> wrote:
> On Thu, Jul 19, 2012 at 05:15:33PM -0000, James Westby wrote:
> > Review: Needs Information
> >
> > Hi Michael,
> Hi James,
>
> > I'm not really sure what "supported" architectures mean here, could you help me
> > out?
>
> Just to clarify, do you mean that the string for the configuration
> option in the configglue part is not clear? Or that its overall not
> clear?

I'm not sure what it means for an architecture to be "supported".

> What I mean with "supported" for the architectures is that those are
> the architectures we have symbols for in the database and expect to
> generate packages for. Do you think a different name would make it
> clearer? Maybe "known" or something like this?

Maybe. If the info is in the database, then could it be inferred from
that?

> > 37 + # XXX: mvo: it seems we don't need "all" here as we are interessted
> > 38 + # in binary symbols only?
> >
> > I'm not sure what that means, but it sounds like something we should have an
> > answer for before we land this?
>
> Indeed, its based on the previous code that remove "all" from the list
> of ARCHITECTURES for the launchpad querry. And it made me wonder if we
> need "all" at all. I think not, but maybe I'm overlooking something.

Right, if this is the architectures we are looking in packages for
symbols for then all probably isn't needed.

> > 53 +ARCHITECTURES = load_configuration().options.architectures_supported
> >
> > Setting a module-level variable based on reading a config file is bad form,
> > because it will read the config file at import, which means it's hard
> > to write tests for example. Querying the config at use would be better.
>
> This is fixed now in my branch.

Great, thanks.

> Thinking a bit more about the branch it may actually not be that
> useful, I mainly did it in preparation for the
> lp:~mvo/pkgme-devportal/raise-on-unsupported-architectures
>
> But given that the database currently does not have a architecture
> field afaik it may well be a bit pointless to make it configurable
> before it is actually configurable. So I'm fine putting that branch on
> hold for now until the rest of the code is updated to work with
> multiple architectures too.

Ok.

If this is just about giving better error messages until we have
architectures figured then I'm fine with it, but we might as well skip
the configuration overhead and just hardcode the list in that case.

Thanks,

James

[Michael Vogt (mvo)]

Thanks James, I agree that in itself this is probably not useful to have this as a config option. Maybe it makes more sense in the context that this will eventually lead up to lp:~mvo/pkgme-devportal/more-architectures which should make it easy to add additional architecture via this config option.

[lp:~mvo/pkgme-devportal/i386-only-for-now]

62. By Michael Vogt on 2012-07-24

merged trunk and resolved conflicts

=== modified file 'devportalbinary/binary.py'
--- devportalbinary/binary.py	2012-07-24 14:16:18 +0000
+++ devportalbinary/binary.py	2012-07-24 14:43:20 +0000
@@ -283,8 +283,11 @@
     DEPENDS = '${shlibs:Depends}, ${misc:Depends}'
 
     def get_architecture(self, metadata):
-        # XXX: No idea about architecture
-        return None
+        # XXX: once we scan more than one architecture we need to actually
+        #      pick the ones that we build for (e.g. set to "i386 amd64")
+        #      But currently our database only does i386 so its ok for now
+        conf = load_configuration()
+        return "".join(conf.options.architectures_supported)
 
     def get_build_depends(self, metadata):
         return ', '.join(guess_dependencies(self.path))
 
=== modified file 'devportalbinary/configuration.py'
--- devportalbinary/configuration.py	2012-07-17 13:26:31 +0000
+++ devportalbinary/configuration.py	2012-07-24 14:43:20 +0000
@@ -11,6 +11,7 @@
     Section,
     DictOption,
     StringOption,
+    TupleOption,
     )
 
 
@@ -57,6 +58,20 @@
         help='mapping of library name to pkgname to force picking selected '
              'dependencies')
 
+    # The architectures that we fetch binary packages for.
+    #
+    # XXX: Really, we should be fetching the binary packages for _all_
+    # architectures and storing their symbols in the database keyed by
+    # architecture, and then using the architecture of the incoming binary to
+    # figure out which libraries we need (XXX - not sure how multiarch affects
+    # this.  Instead, for the moment we are assuming that all binaries are
+    # i386.
+    architectures = Section()
+    architectures.supported = TupleOption(
+        # XXX: mvo: it seems we don't need "all" here as we are interessted
+        #           in binary symbols only?
+        default=("i386",),
+        help='The supported architectures')
 
 def get_config_file_path():
     """Return the path to the configuration file."""
 
=== modified file 'devportalbinary/database.py'
--- devportalbinary/database.py	2012-07-17 13:51:11 +0000
+++ devportalbinary/database.py	2012-07-24 14:43:20 +0000
@@ -34,16 +34,6 @@
 SERVICE_ROOT = uris.LPNET_SERVICE_ROOT
 
 
-# The architectures that we fetch binary packages for.
-#
-# XXX: Really, we should be fetching the binary packages for _all_
-# architectures and storing their symbols in the database keyed by
-# architecture, and then using the architecture of the incoming binary to
-# figure out which libraries we need (XXX - not sure how multiarch affects
-# this.  Instead, for the moment we are assuming that all binaries are i386.
-ARCHITECTURES = ('i386', 'all')
-
-
 class LaunchpadFixture(Fixture):
 
     def __init__(self, application_name, service_root):
@@ -78,16 +68,15 @@
 
 
 def iter_published_binaries(lp, since=None, name=None, exact_match=False):
+    architectures = load_configuration().options.architectures_supported
     ubuntu = lp.distributions['ubuntu']
     archive = ubuntu.main_archive
     # XXX: oneiric is a puppy that is just for christmas. Specifically, it's a
     # bug that this is looking in oneiric, should instead be looking in
     # ... well, we don't know.
     oneiric = ubuntu.getSeries(name_or_version='oneiric')
-    # XXX: 'all' doesn't work?
-    ARCHITECTURES = ['i386']
     our_series = (
-        oneiric.getDistroArchSeries(archtag=tag) for tag in ARCHITECTURES)
+        oneiric.getDistroArchSeries(archtag=tag) for tag in architectures)
     filters = dict(status='Published')
     if since:
         filters['created_since_date'] = since
@@ -174,6 +163,7 @@
         """
         directory = tempfile.mkdtemp()
         binary_file_urls = self.spph.binaryFileUrls()
+        architectures = load_configuration().options.architectures_supported
         for url in binary_file_urls:
             # XXX: This is a bit of a hack to work around the fact that Launchpad
             # has no way of filtering binary packages for a source package by
@@ -185,7 +175,7 @@
                 # binary packages that export symbols and *don't* start with lib.
                 continue
             architecture = filename.split('_')[-1]
-            if architecture not in ARCHITECTURES:
+            if architecture not in architectures:
                 continue
             path = download_file(url, directory)
             symbols_contents = extract_symbols_file(path)
 
=== modified file 'devportalbinary/tests/test_binary.py'
--- devportalbinary/tests/test_binary.py	2012-07-24 14:16:18 +0000
+++ devportalbinary/tests/test_binary.py	2012-07-24 14:43:20 +0000
@@ -344,6 +344,13 @@
             get_packages_for_libraries(["libasound.so.2"], "i386"),
             set(["libasound2"]))
 
+    def test_architecture(self):
+        backend = self.make_backend()
+        conf = load_configuration()
+        self.assertEqual(
+            backend.get_architecture(metadata={}), 
+            "".join(conf.options.architectures_supported))
+
     def test_get_extra_targets_no_bundled_libs(self):
         package_name = self.getUniqueString()
         path = self.getUniqueString()