Merge lp:~milo/linaro-ci-dashboard/correct-chain-build into lp:linaro-ci-dashboard

[Milo Casagrande]

This branch fixes the problem with chaining into AndroidLoop: when a build was scheduled, the real AndroidLoop was not really created nor built. The part missing was the overriding of the 'preconfigure' method.

I had also to change it into an instance method instead of a classmethod since that caused some problem calling it correctly.

[Stevan Radaković (stevanr)]

This needs some more work here in my opinion.
You also have a wrong description here. The build was scheduled for the chained loop, it's just that the results of the textfield loop build were not transferred to the android loop build.
Now the questions remain: shouldn't the dict parameters be mapped into the real android loop fields now? What is "user_defined_values"? Do they override the other fields in the loop?

review: Needs Information

[Milo Casagrande (milo)]

On Fri, Sep 14, 2012 at 1:51 PM, Stevan Radaković
<email address hidden> wrote:
> Review: Needs Information
>
> This needs some more work here in my opinion.
> You also have a wrong description here. The build was scheduled for the chained loop, it's just that the results of the textfield loop build were not transferred to the android loop build.

Yes, the point is that the values are not used at all at this stage.
Even if it is scheduled, it is not building anything since we are not
doing anything with those results.

> Now the questions remain: shouldn't the dict parameters be mapped into the real android loop fields now?

That was my understanding of the chainability of loops, at least WRT
android textfield loops and android loop.
They write everything in the Android textfield, we transfer that
inside the AndroidLoop and runs from there.

Even if we do not want to map parameters into the field, we need to do
something with the dict that is passed, otherwise it is not used. The
same could be said if we do not want to map things, we can easily drop
AndroidLoop and we do not need the chainability with it (we just move
the logic into the textfield loop).

> What is "user_defined_values"?

The AndroidLoop model is exactly the same model as is in the wiki
description. The point of "user_defined_values" text field is that
when they create a build, they write variables that are not documented
anywhere.

Take a look here:
https://android-build.linaro.org/builds/~linaro-android/snowball-ics-gcc46-igloo-stable-blob/

There are variables that are not part of the Android build service as
we have now (an example MONKEY_RUNNER):
https://wiki.linaro.org/Platform/Android/LinaroAndroidBuildService
Those should be environment variable used at some stage of the build.
Where: I have no idea.

The idea behind AndroidLoop was to have all the necessary fields (as
described in the wiki) as first class fields in the model, but still
giving the user a way to define the other kind of variables.

> Do they override the other fields in the loop?

They actually can, because there is no check that prevents that. The
same could happen if somebody writes the same key<>value pair twice in
the text field.

What I do here is basically setting up the AndroidLoop model with the
values from the dict: if something in there does not match an
attribute of AndroidLoop, store it in 'user_defined_values'. After
that, the configuration passed into Jenkins is the base64 encoded
string of all the values, also of those in 'user_defined_values' (at
the moment this last part is not happening though).

--
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

[Stevan Radaković (stevanr)]

On 09/14/2012 02:40 PM, Milo Casagrande wrote:
> On Fri, Sep 14, 2012 at 1:51 PM, Stevan Radaković
> <email address hidden> wrote:
>> Review: Needs Information
>>
>> This needs some more work here in my opinion.
>> You also have a wrong description here. The build was scheduled for the chained loop, it's just that the results of the textfield loop build were not transferred to the android loop build.
> Yes, the point is that the values are not used at all at this stage.
> Even if it is scheduled, it is not building anything since we are not
> doing anything with those results.

Second part is still not correct. It actually builds the existing
AndroidLoop which is chained to this Textfield one.
Since you need to have the AndroidLoop in the system in order to chain
it, it has already some default set of options, and it can be build. The
problem still remains with not updating the chained loop with the
results of text field.

Regarding everything else, I didn't look at the code correctly.. I
somehow missed the lines

18 + for key, value in configuration.iteritems():
19 + if hasattr(self, key.lower()):
20 + setattr(self, key.lower(), value)
21 + else:

Everything else is clear.
Approving +1.

[Stevan Radaković (stevanr)]

review: Approve

[Milo Casagrande (milo)]

On Fri, Sep 14, 2012 at 3:01 PM, Stevan Radaković
<email address hidden> wrote:
>
> Second part is still not correct. It actually builds the existing
> AndroidLoop which is chained to this Textfield one.
> Since you need to have the AndroidLoop in the system in order to chain
> it, it has already some default set of options, and it can be build.

True. But does it build something with the default values? I was never
able to... :-/
Thanks for the review!

--
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

[Paul Sokolovsky (pfalcon)]

> There are variables that are not part of the Android build service as
> we have now (an example MONKEY_RUNNER):
> https://wiki.linaro.org/Platform/Android/LinaroAndroidBuildService
> Those should be environment variable used at some stage of the build.
> Where: I have no idea.

If you look at https://wiki.linaro.org/Platform/Android/LinaroAndroidBuildService it has

"As explained above, you may use other options/variables as supported by Android makefiles (they will be exported to the environment)."

I.e. it's explicitly documented that *arbitrary* variables can be used in android-build.linaro.org , it's just the case that some of variables have well-defined semantics (as documented in https://wiki.linaro.org/Platform/Android/LinaroAndroidBuildService), while the rest is just "allowed". So yes, presence of "user_defined_values" is understood, and it's not that easy to map legacy build into new structured build (for that, you should know all strictly defined fields, and parse them out).

=== modified file 'dashboard/frontend/android_build/models/android_loop.py'
--- dashboard/frontend/android_build/models/android_loop.py	2012-09-12 12:48:46 +0000
+++ dashboard/frontend/android_build/models/android_loop.py	2012-09-14 11:40:23 +0000
@@ -140,6 +140,30 @@
                                                       capitalize=capitalize,
                                                       upper=True)
 
+    def preconfigure(self, configuration=None):
+        """Makes a build parameter dict from build result configuration.
+
+        This method should be overridden by subclasses.
+        """
+        if isinstance(configuration, dict):
+            # The name is the name of the loop that is calling the chained one,
+            # not the one we want to actually configure, we drop it.
+            configuration.pop('name')
+            unmatched_values = u''
+            for key, value in configuration.iteritems():
+                if hasattr(self, key.lower()):
+                    setattr(self, key.lower(), value)
+                else:
+                    # If the attr cannot be found, we throw everything in
+                    # user_defined_values since the text field is free form.
+                    unmatched_values = u'%s=%s\n' % (key.upper(), str(value))
+
+            self.user_defined_values = unmatched_values
+            self.save()
+        else:
+            self.log.error("Parameter was not a 'dict' instance.")
+        return {}
+
     @models.permalink
     def get_detail_url(self):
         return 'AndroidLoopDetail', (), {'slug': self.name}

=== modified file 'dashboard/frontend/models/loop.py'
--- dashboard/frontend/models/loop.py	2012-09-12 13:01:02 +0000
+++ dashboard/frontend/models/loop.py	2012-09-14 11:40:23 +0000
@@ -247,19 +247,21 @@
         """Invokes the build of the next loop in chain.
 
         This method will be called when the build is finished.
+
+        :param configuration: The configuration for creating the new loop.
+        :type configuration dict
         """
         if self.next_loop:
-            next_loop = self.next_loop.get_child_object()
-            parameters = next_loop.__class__.preconfigure(configuration)
-            next_loop.schedule_build(parameters)
+            chained_loop = self.next_loop.get_child_object()
+            parameters = chained_loop.preconfigure(configuration)
+            chained_loop.schedule_build(parameters)
 
-    @classmethod
-    def preconfigure(cls, configuration=None):
+    def preconfigure(self, configuration=None):
         """Makes a build parameter dict from build result configuration.
 
         This method should be overridden by subclasses.
         """
-        return configuration
+        return {}
 
     @models.permalink
     def get_detail_url(self):