Merge lp:~pieq/checkbox/fix-1561821-new-input_id-parameter-for-input-jobs into lp:checkbox

[Pierre Equoy]

[Paul Larson (pwlars)]

Looks ok to me, though I'm not sure how to test it. It looks like LP is messed up trying to generate the diff.

review: Approve

[Sylvain Pineau (sylvain-pineau)]

The missing diff:

$ bzr diff -c4288
=== modified file 'checkbox-support/checkbox_support/parsers/udevadm.py'
--- checkbox-support/checkbox_support/parsers/udevadm.py 2016-02-17 16:40:50 +0000
+++ checkbox-support/checkbox_support/parsers/udevadm.py 2016-03-28 10:29:59 +0000
@@ -122,7 +122,8 @@
         "_vendor",
         "_vendor_id",
         "_subvendor_id",
- "_vendor_slug",)
+ "_vendor_slug",
+ "_input_id",)

     def __init__(self, environment, name, lsblk=None, bits=None, stack=[]):
         self._environment = environment
@@ -140,6 +141,7 @@
         self._vendor_id = None
         self._subvendor_id = None
         self._vendor_slug = None
+ self._input_id = None

     def __repr__(self):
         vid = int(self.vendor_id) if self.vendor_id else 0
@@ -503,6 +505,17 @@
         return None

     @property
+ def input_id(self):
+ if self._input_id is not None:
+ return self._input_id
+ # e.g.:
+ # '/devices/pci0000:00/0000:00:14.0/usb3/3-1/3-1:1.1/0003:046D:C534.0002/input/input8'
+ # we want the last thing ('input8')
+ if 'input' in self.path:
+ return self.path.split('/')[-1]
+ return None
+
+ @property
     def product_id(self):
         if self._product_id is not None:
             return self._product_id
@@ -773,7 +786,8 @@
     def as_json(self):
         attributes = ("path", "bus", "category", "driver", "product_id",
                       "vendor_id", "subproduct_id", "subvendor_id", "product",
- "vendor", "interface", "name", "product_slug", "vendor_slug")
+ "vendor", "interface", "name", "product_slug",
+ "vendor_slug", "path")

         return {a: getattr(self, a) for a in attributes if getattr(self, a)}

=== modified file 'providers/plainbox-provider-checkbox/jobs/input.txt.in'
--- providers/plainbox-provider-checkbox/jobs/input.txt.in 2016-03-02 14:01:49 +0000
+++ providers/plainbox-provider-checkbox/jobs/input.txt.in 2016-03-28 10:29:59 +0000
@@ -3,9 +3,8 @@
 template-filter: device.category == 'MOUSE' or device.category == 'TOUCHPAD' or device.category == 'TOUCHSCREEN'
 plugin: manual
 category_id: 2013.com.canonical.plainbox::input
-id: input/pointing_{product_slug}_{category}_{__index__}
+id: input/pointing_{product_slug}_{category}_{input_id}_{__index__}
 estimated_duration: 30.0
-requires: device.path == '{path}'
 _summary: Check pointing functionality for {product}
 _purpose:
     This will test your {product} device
@@ -76,9 +75,8 @@
 template-filter: device.category == 'MOUSE' or device.category == 'TOUCHPAD'
 plugin: manual
 category_id: 2013.com.canonical.plainbox::input
-id: input/clicking_{product_slug}_{category}_{__index__}
+id: input/clicking_{product_slug}_{category}_{input_id}_{__index__}
 estimated_duration: 30.0
-requires: device.path == '{path}'
 _summary: Check button functionality for {product}
 _purpose:
     This will test the buttons of your {product} device

=== modified file 'providers/plainbox-provider-resource-generic/bin/udev_resource'
--- providers/plainbox-provider-resource-generic/bin/udev_resource 2016-02-02 12:54:35 +000...

[Sylvain Pineau (sylvain-pineau)]

The idea was good but...

I see two problems:

1 - It increases the amount of data generated from the udev resource job as every input-like devices will get this additional field and we really need a fix for two input jobs.

2 - Adding the input kernel id to each job will give us two distinct jobs, one before unplugging the mouse and a second after re-plug and resuming/bootstrapping the session.

I diff'ed the two input jobs and what differs between both is indeed located in the job.parameters (the path property). To mitigate/work around the problem I'd would keep the change to the job definitions (in all cases they have to be updated) but rather modify how templates work to instantiate new units with only the parameters they need (i.e without the changing udev path).

See https://code.launchpad.net/~sylvain-pineau/checkbox/fix-1561821

review: Disapprove