Merge lp:~didrocks/autopilot/remove-circular-dep into lp:autopilot

[Didier Roche-Tolomelli]

don't create circular dependency. python-autopilot should recommends -desktop or -touch autopilot, but don't dep on them

don't create circular dependency. python-autopilot should recommends -desktop or -touch autopilot, but don't dep on them

[Łukasz Zemczak (sil2100)]

Yes, this makes more sense now. +1

review: Approve

[Thomi Richards (thomir-deactivatedaccount)]

Guys,

As I'm sure you're aware, developers can become a little too posessive over their code. Perhaps this is one such instance, but please don't push MPs through to land without consulting the relevant people for a review first.

MTerry and I discussed this in Oakland. The python-autopilot package is useless without *either* autopilot-touch *or* autopilot-desktop installed. Literally useless. You won't be able to create any input device, or get any display information, or use any of the other autopilot drivers.

We (mterry and I) did some research regarding circular dependencies, and (if my memory serves me right) we found that in the case of circular dependencies, apt will break the chain at an arbitrary point. This is only a problem if you have (post|pre)(inst|rm) scripts that require your dependency to be installed at the time the script is run. We do not fit this category. The only script we run is to make the uinput-autopilot device node, which does not depend on the autopilot-touch or autopilot-desktop code.

So, if you'd like this merged, please explain why this change is a good thing. What problem does this MP solve? How will you solve the problem of people installing python-autopilot only, and then getting a broken autopilot setup?

I hope this doesn't come across too harsh - I really don't mean it to, but it's an unpleasant surprise to see things like this being merged when I was not made aware of any issue that needed to be solved.

Cheers,

review: Needs Information

[Didier Roche-Tolomelli (didrocks)]

Hey Thomi,

Well, you should know that packages are pushed by people having upload rights, knowing exactly how the package system work, earning that through years.

We are not touching or approving upstream code change without upstream ack. We would like the same for the packaging side (seeing in particular the number of missing replaces/conflicts we had to fix).

You are creating a circular dependency, as explained in the commit message. This is considered harmful as you can't bootstrap the distribution anymore. If you have to rebuild everything, how would you do it? If you have this breakdown of A being useless without B and B being useless without A, maybe your split doesn't make sense?

Telling python-autopilot is useless without autopilot-desktop or autopilot-touch doesn't qualify to add that dependency (it should even maybe be just a suggest). For instance, I can argue installing libunity-core-6.0-5 without unity is useless, should we have it depending on unity itself? I don't think so. I see the same rationale with python-autopilot

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:223
http://jenkins.qa.ubuntu.com/job/autopilot-ci/106/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-saucy-amd64-ci/34
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-saucy-armhf-ci/34

Click here to trigger a rebuild:
http://s-jenkins:8080/job/autopilot-ci/106/rebuild

review: Approve (continuous-integration)

[Thomi Richards (thomir-deactivatedaccount)]

Hi,

On Tue, May 28, 2013 at 7:07 PM, Didier Roche <email address hidden> wrote:

> Well, you should know that packages are pushed by people having upload
> rights, knowing exactly how the package system work, earning that through
> years.
>
> We are not touching or approving upstream code change without upstream
> ack.

Except that's exactly what you're doing. This is a merge proposal against
lp:autopilot - that is upstream code. We maintain our own packaging code
because it is convenient for us to do so. We fully expect the distro team
to want/need to tweak that code, in which case you have two options: either
maintain separate packaging information in a branch somewhere, or work with
us to alter our own packaging information.

I'm quite happy to approve merges like this, because, as you point out: you
guys are the packaging experts here. All it would take is to drop me an
email (since we're rarely on IRC at the same time), saying "Hey Thomi,
there's a problem in the packaging for autopilot, it's <description>, and
there's a branch that fixes it here <branch_location>. Could you review it
please?"

> We would like the same for the packaging side (seeing in particular the
> number of missing replaces/conflicts we had to fix).
>
> You are creating a circular dependency, as explained in the commit
> message. This is considered harmful as you can't bootstrap the distribution
> anymore. If you have to rebuild everything, how would you do it? If you
> have this breakdown of A being useless without B and B being useless
> without A, maybe your split doesn't make sense?
>
> but.. if we're talking about building something, surely you'd care about
Build-Depends, nor Depends? There is no circular dependency in autopilot
when it comes to building the packages. It's all pretty straightforward
there. The only circular dependency is at run-time, but since you can
trivially build all the packages we need, I don't see how this is a
problem. Perhaps I'm missing something, and you can clarify this point for
me?

> Telling python-autopilot is useless without autopilot-desktop or
> autopilot-touch doesn't qualify to add that dependency (it should even
> maybe be just a suggest). For instance, I can argue installing
> libunity-core-6.0-5 without unity is useless, should we have it depending
> on unity itself? I don't think so. I see the same rationale with
> python-autopilot
>

libunity is a library, I guess you could conceivably use it without unity.
However, Unity will never work without libunity.

Similarly, Autopilot can never work without either autopilot-desktop or
autopilot-touch. If you need to break the dependency chain (and I'm not
convinced of that yet - see above), then I'm not sure this is the correct
place to break it.

If you'd rather discuss this in real-time, feel free to put an event on my
calendar (make it at a sensible time please - I guess late in my day and
early in yours is going to be most sensible).

Cheers,

--
Thomi Richards
<email address hidden>

[Didier Roche-Tolomelli (didrocks)]

Indeed this is in the upstream code, but I think this would be silly to start forking our own upstream packaging and I think upstream should be a little bit more trustful on people making packaging for years. Also, they should request if they need any help on the packaging, especially for transition (that would have avoided all the Replaces: mismatch).

We shouldn't have to use mail, just merge request. However, do you remember how long some trivial branches sit idling last cycle before getting reviewed after a ping? I thought that for a trivial change only involving packaging changes, upstream can just take packagers into consideration and trust them to do the right thing… (or upstream need to be quite reactive on their MP).

We care about build-deps as well as deps, especially as autopilot is used as a release pre-requisite right now. Conceptually python-autopilot is a library for autopilot as it can never work without -desktop or -touch. So let me reiterate:
- it's either that the breakage isn't good because those 2 packages
- or, as we did on other packages, in both Debian and Ubuntu, downgrade as a recommends, as we install recommends by default.

Mind reconsidering this one and moving on?

[Didier Roche-Tolomelli (didrocks)]

From http://www.debian.org/doc/debian-policy/ch-relationships.html (7.2 Binary Dependencies):

"If there is a circular dependency among packages being installed or removed, installation or removal order honoring the dependency order is impossible, requiring the dependency loop be broken at some point and the dependency requirements violated for at least one package. Packages involved in circular dependencies may not be able to rely on their dependencies being configured before they themselves are configured, depending on which side of the break of the circular dependency loop they happen to be on. If one of the packages in the loop has no postinst script, then the cycle will be broken at that package; this ensures that all postinst scripts are run with their dependencies properly configured if this is possible. Otherwise the breaking point is arbitrary. Packages should therefore avoid circular dependencies where possible, particularly if they have postinst scripts."

Yeah, as you can see, it's not an issue as of today, BUT, maybe one day, you will have subtile configuration issue and we'll have forgotten about the circular dependency. That's why following the debian policy: we should therefore avoid them

[Thomi Richards (thomir-deactivatedaccount)]

review: Approve

=== modified file 'debian/control'
--- debian/control	2013-05-15 10:25:55 +0000
+++ debian/control	2013-05-27 12:42:28 +0000
@@ -27,13 +27,13 @@
 Architecture: all
 Depends: ${misc:Depends},
          ${python:Depends},
-         autopilot-desktop | autopilot-touch,
          python-dbus,
          python-junitxml,
          python-testscenarios,
          python-testtools,
          udev,
-Recommends: libautopilot-gtk (>= 1.3),
+Recommends: autopilot-desktop | autopilot-touch,
+            libautopilot-gtk (>= 1.3),
             libautopilot-qt (>= 1.3),
             python-autopilot-trace,
             recordmydesktop,