=== jtv1 is now known as jtv [10:07] henninge: could you please review this branch: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-class-udev-device-8/+merge/13554 ? [10:22] intellectronica: ^^^? === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com [10:28] adeuring: yes, looking at it now [10:28] intellectronica: thanks! === henninge changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com [10:33] I am here now ... [10:33] HI intellectronica! ;) [10:33] hi henninge, happy new week! === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: adeuring, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com [11:06] adeuring: wouldn't it be better to use path expressions to find the nodes you want? less likely to make this sort of mistake [11:07] intellectronica: I'm doing this now, I believe. Or do I miss something? [11:08] adeuring: i'm looking at parent.parent.parent.parent and thinking out loud how we can avoid it [11:09] intellectronica: that's hard... We simply need the grandparent of a device, in UdevDevice.scsi_controller [11:09] adeuring: the code in general looks fine, but i find it hard to follow, so i'm wondering if there's anything that can be done to improve it [11:09] yes [11:10] intellectronica: yes, I know that it is not that simple. That's the reason for the sometimes long-winded comments ;) [11:33] adeuring: r=me. i find the code quite hard to wrap my head around, but i guess that's mostly just because it's a complex document that's being parsed. i couldn't think of any way to improve on that with small changes. and yes, the commentary does help a lot === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com [11:34] henninge: would you like me to review your branch? [11:34] intellectronica: thanks! [11:36] henninge: your fix looks good. The comment for the test is lacking the word "entry" after "an," and the test method is confusingly named; I'd suggest test_approve_only_if_needs_review. Ceterum censeo r=me. [11:38] intellectronica: thanks but I was impatient and asked jtv. He is familiar with the problem, anyway. [11:38] henninge: also, w.r.t. your test plan, not that it matters much but I don't think the date shown in the UI will be updated. AFAIK we have a bug open for that. [11:38] cool bananas [11:42] jtv: thank you [11:46] al-maisan, I'll look at that branch now. [11:58] jml: thanks [11:59] jtv: http://paste.ubuntu.com/296752/ [12:00] henninge: fine === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: edwin, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com [12:31] intellectronica, henninge: fancy to reveiw a sequel of my previous branch: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-class-udev-device-9/+merge/13562 (~650 lines, but ca 40%...50% is the definition of test data) [12:32] adeuring: i'm reviewing a branch of edwin's i picked off the reviews queue, but can pick your branch when i finish, it won't be long [12:32] intellectronica: thanks! === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: edwin, - || queue: [abel] || This channel is logged: http://irclogs.ubuntu.com === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: abel, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com [12:47] maaan, those hwdb branches are starting to feel like soyuz to me. i have to work so much out just in order to be able to understand what they do [12:49] lies === mrevell is now known as mrevell-lunch [13:33] adeuring: r=me === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com [13:42] intellectronica: thanks! === mrevell-lunch is now known as mrevell === abentley changed the topic of #launchpad-reviews to: on call: intellectronica, henninge,abentley || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com === ursula is now known as Ursinha [15:06] intellectronica: fancy another sequel of this HWDB submissions stuff (a bit shorter this time: 177 lines diff)? [15:06] adeuring: sure [15:06] intellectronica: thanks! [15:06] https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-class-udev-device-10/+merge/13567 [16:28] abentley: Can you review a short lpbuildbot branch for me please? There is some bzrlib stuff in there so I'd like you to check my assumptions. https://code.edge.launchpad.net/~allenap/lpbuildbot/explicitly-disconnect-transports-bug-419421/+merge/13575 [16:28] allenap: sure. [16:28] Thanks. === abentley changed the topic of #launchpad-reviews to: on call: intellectronica, henninge,abentley || reviewing: -, -, allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com [16:36] allenap: The transports variable isn't doing a lot for you. I'm not sure what disconnecting two transports with the same underlying connection does. [16:37] abentley: I don't follow. [16:38] allenap: transports is an optional parameter to Branch.open_containing. [16:38] allenap: It's only valuable if you pass it into another call later. [16:39] abentley: I was relying on the fact that reusable transports get appended to it, so I could disconnect them. [16:40] allenap: I don't think we provide a strong guarantee that passing it into open_containing will append all transports that were opened in the process of opening the branch. [16:40] abentley: Ah, okay. [16:41] allenap: Are you sure you need to disconnect all the transports? [16:43] abentley: Well, no, but from the bug report it seems that ssh processes are not getting reaped all the time. I'm assuming that they are related to bzr operations. If that's a correct assumption, then I want to try explicitly reaping them. [16:43] abentley: It may be that the assumption is wrong, but I want to try this because I don't have any better ideas :) [16:44] allenap: Right, but are you sure you need to disconnection all the *transports*? Because I would have thought you only needed to disconnect all the *connections*. [16:45] abentley: I didn't understand the distinction before. All the connections I would say. Calling transport.disconnect() closes the connection, so I'm relying on that. [16:47] allenap: Right, so I think you can just do branch.bzrdir.root_transport.disconnect() [16:49] abentley: Okay. I assume that will basically disconnect everything that bzrlib has done from everything. I think that'll be fine for our buildbot configuration as it stands, but it's bad behaviour if we ever add more pollers to the configuration. [16:50] allenap: No, that will just disconnect the ssh connection related to the branch you just opened. [16:52] abentley: Ah, I read that wrong! I read bzrlib.bzrdir.root_transport.disconnect, and I assumed root_transport was a module global. [16:52] abentley: Okay, thanks for that, I'll use that approach. [16:52] allenap: Cool. [16:54] allenap: Also, you should pull the "branch =" statement out of the try block. If that were to fail, you wouldn't want to go to the finally block. [16:54] abentley: Yeah, agreed. [16:56] I can't say whether this approach will actually solve the zombie problem, but it looks fine otherwise. [16:56] al-maisan, hello [16:56] hello jml , just read your email [16:57] thanks very much for clarifying things [16:57] al-maisan, my pleasure. [16:57] so, the branch-sourcepackage relation is 1-on-1 .. but [16:58] al-maisan, there are two kinds of relationships between a branch and a sourcepackage [16:58] aha [16:58] intellectronica: henninge, abentley: Can one of you take https://code.edge.launchpad.net/~sinzui/launchpad/package-link-validation-1/+merge/13578 ? [16:58] al-maisan, a branch can *belong* to a source package === sinzui changed the topic of #launchpad-reviews to: on call: intellectronica, henninge,abentley || reviewing: -, -, allenap || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com [16:58] al-maisan, (or a product, or be a junk branch) [16:58] yes... [16:58] al-maisan, alternatively, a branch can be the *official branch* for a (sourcepackage, pocket) [16:59] sinzui: sure, i'll review it [16:59] the two concepts are independent. === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge,abentley || reviewing: sinzui, -, allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com [16:59] al-maisan, furthermore, a branch can be the official branch for more than one (sourcepackage, pocket) ISTR [16:59] jml: can be the can be the *official branch* for a number of (sourcepackage, pocket) items? [17:00] ah [17:00] allenap: AIUI, this method is called in a thread from a long-running process. Correct? [17:00] abentley: Yes. [17:01] allenap: Have you considered passing a transports parameter into it from the long-running process? [17:01] al-maisan, this is deliberate. I'm beginning to think it's also misguided. [17:01] jml: is it correct to assume that in such a (sourcepackage, pocket) collection a sourcepackage.distroseries only appears once? [17:01] sinzui: i don't understand the way you structured that conditional around line 32 of the diff [17:01] allenap: That would presumably limit you to one SSH connection per long-running process. [17:01] al-maisan, I'm afraid not. [17:01] sinzui: why do you need an extra if? [17:02] * sinzui should have annotated the cover letter [17:02] jml: how's that beneficial? [17:02] abentley: That's certainly a possibility, quite an easy one to do. The transports will go for no more than 10 minutes without being used. Is that likely to cause problems? What happens if bazaar.launchpad.net goes down? Will bzrlib cope? [17:02] al-maisan, that's all you have to work with in terms of assumptions: http://pastebin.ubuntu.com/296938/ [17:02] ah [17:02] al-maisan, I'm not sure it _is_ beneficial :) [17:02] aha :) [17:03] al-maisan, but the code you write here has to work with our current model [17:03] jml: that's understod. [17:03] allenap: I'm not sure how bzrlib reacts if the host goes away while a connection is open. [17:03] intellectronica: There are two kinds of errors the user can get when setting a sourcepackage for a series: the first rule that exists is that you cannot reuse the the exact combination of distroseries, sourcepackage, and productseries. [17:04] jml: hmm .. this looks a bit funny: UNIQUE, btree (distroseries, sourcepackagename, pocket) [17:04] allenap: I'm more worried about the fact that transports weren't designed to be thread-safe, so you'd have to take care of that yourself. [17:04] intellectronica: The error gets its own message because the problem is locale to the series being edits [17:04] al-maisan, that means... [17:05] sinzui: but why `message="..."\n if message:...`? you don't need that if [17:05] al-maisan, I'm on a call I want to keep short. you'll have my full attention in a minute [17:05] jml: sure [17:05] intellectronica:sorry. I have the plague and I and thinking is a challenge [17:06] abentley: I think gary_poster must have had that problem because the custom BzrPoller we use overrides buildbot's to avoid concurrency already. [17:06] jml: well, if I am to check whether we can upload to a pocket and a number of (distroseries, pocket) combination can appear in that storm result set, the question is which one should I check.. [17:06] intellectronica: That is crack I should have removed when I refacteded the branch between commits [17:06] abentley: Meaning: gary_poster wrote the custom poller afaict. [17:06] riht [17:06] right [17:06] maybe all of them and be satisfied when I hit the first one that "passes" [17:07] sinzui: also, when you have a multiline conditional (like around line 38), it's nice if you can add a comment immediately before the then clause, so that it's clearer where the condition stops and the statements to execute begin [17:08] sinzui: ditto near line 46 [17:08] jml: I guess I am a bit confused since there can be one "current" source package release in a distro series and I assumed that an official branch would be linked to that .. [17:08] intellectronica: okay [17:08] I can add comments [17:08] cool [17:09] allenap: I don't think Launchpad PQM Bot should be subscribed to code review email. [17:09] abentley: No, that seems odd. [17:09] sinzui: it's a bit strange when you set next_url to the value of cancel_url, becuase even though it's the correct url, it implies that you're canceling [17:09] indeed [17:10] sinzui: i would add another property, even though it's inefficient, just for readability [17:10] abentley: I don't have the power to fix that. You? [17:10] I can define next_url and and set cancel_url from it. [17:11] allenap: Yes. I've left it subscribed, so it can see the branch, but with no email options set. [17:11] sinzui: yeah, i think that's the best solution [17:12] abentley: The code that calls getRawChanges catches all exceptions, so wouldn't crash if bzrlib went awry due to a failed transport being passed in. But I wonder if (a) keeping those transports around would cause problems on all subsequent runs (so requiring a restart of buildbot), or (b) if we emptied the list of transports after an exception would we leak processes again? [17:13] sinzui: i'm not sure about the improved description for the packaging field. it's a lot nicer, but i wonder if users will find the concrete examples helpful. i, for example, have no idea what cadaver and libneon do, so mentioning them only serves as a distraction [17:13] i know what apache is, though sometimes i wish i didn't [17:13] intellectronica: yes. I struggled with that example [17:14] intellectronica: INCLUDE is a rare condition. upstreams rarely need to know about that [17:14] I don't think a is true. Transports are meant to be reused. b might be true. [17:14] sinzui: i would cut right after the question. i think it's formed clearly enough that users will be able to rely on it alone to understand [17:15] al-maisan, back. really sorry about that. team standup. [17:15] oh, good. I was tempted to do that, but thought saying more would help [17:15] al-maisan, UNIQUE, btree (distroseries, sourcepackagename, pocket) just means that each SuiteSourcePackage has exactly one official branch. [17:16] al-maisan, the goal is for there to be official branches for every SuiteSourcePackage === abentley changed the topic of #launchpad-reviews to: on call: intellectronica, henninge,abentley || reviewing: sinzui, -, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com [17:16] sinzui: i find the way you format the call to dict on line 222 a bit strange. i think that if you use the function call, you should avoid the trailing coma [17:17] al-maisan, as for which one to check [17:17] al-maisan, I think you should check all of them and the real question becomes whether the results are ANDed or ORed. [17:18] intellectronica: I agree with you. I should not have use a trailing comma. [17:20] al-maisan, Really, you're right and the model is wonky here. I'd recommend ANDing them (since it's more restrictive) and we can chase up a more restrictive model w/ james_w and cjwatson. === salgado is now known as salgado-lunch [17:21] jml: I am trying to think whether OR'ing them would be OK as well [17:22] sinzui: the rest looks great, so r=me with these changes [17:23] intellectronica: Here is the incremental: http://pastebin.ubuntu.com/296953/ === beuno is now known as beuno-lunch [17:23] al-maisan, if a distroseries is closed for a pocket then it should be impossible to upload to the associated branch, I think. [17:24] jml: I agree, however, there's that one-branch-to-many (sourcepackage, pocket) relationship [17:24] sinzui: perfect === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge,abentley || reviewing: -, -, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com [17:25] al-maisan, fwiw, with the real data, the relationship is in fact 1:1 [17:25] so, if any of the (sourcepackage, pocket) combinations become non-valid i.e. karmic/release after Nov. 1st [17:25] jml: that's good news [17:25] in which case we should reconsider the following constraint: [17:25] "seriessourcepackagebranch__ds__spn__pocket__key" UNIQUE, btree (distroseries, sourcepackagename, pocket) [17:26] umm, don't you mean, "add a uniqueness constraint to branch"? [17:27] I thought a seriessourcepackagebranch contraint like (distroseries, sourcepackagename) probably makes more sense [17:28] no, because that would make it impossible to have an lp:ubuntu/karmic-security/openssh branch and an lp:ubuntu/karmic/openssh branch [17:28] but then it depends how seriessourcepackagebranch will be used [17:28] jml: maybe we could have brief voice call [17:28] al-maisan, sure. [17:28] * al-maisan fires up skype [17:31] al-maisan, ? [17:31] jml: just a second [17:32] bac: need a reivew? [17:33] intellectronica: yes, please. i was waiting for the MP to show up before asking but got distracted. [17:33] intellectronica: it'll be your easiest of the day [17:33] bac: so that's just an old test reinstated, right? [17:33] indeedy [17:33] intellectronica: and a smidgen of drive-by === deryck is now known as deryck[lunch] [17:35] bac: looks fine, r=me. thanks for remembering to reactivate that test [17:35] intellectronica: thanks === danilos is now known as danilos-afk === salgado-lunch is now known as salgado [18:06] intellectronica, henninge, or abentley: a very simple branch for one of you [18:07] https://code.edge.launchpad.net/~leonardr/lazr.authentication/correct-scheme/+merge/13586 [18:07] leonardr: I am on it === intellectronica changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: -, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com === henninge changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: leonadr, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com === henninge changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: leonardr, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com === deryck[lunch] is now known as deryck === beuno-lunch is now known as beuno === matsubara is now known as matsubara-lunch === abentley is now known as abentley-lunch [18:14] leonardr: looks nice! [18:15] leonardr: shouldn't the base class do some check if self.AUITH_SCHEME actually exists or provide a default value? [18:15] Or am I being over-paranoid? [18:15] hmm [18:15] there's no sensible default value [18:16] if you forget it with a new authentication class, the crash will happen at the appropriate time (when you try to use the authentication for the first time, before you commit anything) [18:16] so i think it's ok [18:17] leonardr: ok, but please add a comment to the base class saying that derived classes need to implement this. [18:17] to the doc string [18:20] leonardr: r=me [18:31] henninge, can I hop into your queue? [18:32] rockstar: no queue, just forgot to update the topic [18:32] ;) === henninge changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: rockstar, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com [18:32] henninge, okay, I'll send it directly to you then. [18:32] rockstar: show me the mp [18:32] henninge, sending now. [18:34] henninge, abentley: even more trivial than my last branch: https://code.edge.launchpad.net/~leonardr/lazr.restful/0.9.13/+merge/13588 [18:38] leonardr: r=me [18:41] henninge, https://code.edge.launchpad.net/~rockstar/launchpad/code-import-cvs-oops/+merge/13589 [18:48] henninge, how's it going? [18:48] rockstar: done, r=me [18:49] henninge, thanks! === henninge changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: -, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com [18:58] henninge, abentley-lunch ping. i have a very simple branch [18:58] barry: bring it on [18:58] henninge: thanks! mp'ing it now === henninge changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: barry, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com [19:18] henninge: it's almost ridiculously trivial :) https://code.launchpad.net/~barry/launchpad/mm2112/+merge/13591 [19:20] barry: the trivality is soothing at this hour ... ;-) [19:20] barry: r=me === abentley-lunch is now known as abentley [19:21] henninge: thanks! :) === henninge changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: -, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com === matsubara-lunch is now known as matsubara === henninge changed the topic of #launchpad-reviews to: on call: abentley || reviewing: -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com [20:53] sinzui: can you do a UI review of this MP (screenshot links in the last comment). https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-426899-ppa-links/+merge/13505 [20:56] EdwinGrubbs: I really think that the link to "PPA packages related to Curtis Hovey" should be (i) Related PPA packages [20:56] sinzui: sounds good [20:57] hmm. My page is wrong too [20:57] Has my hate for these pages evolved to apathy. I am not even surprised that they are wrong [20:57] Why would motus trust these page? [21:04] sinzui: which piece of information is wrong on your page? [21:05] EdwinGrubbs: That package in my ppa is superseded. I expected to see the one that users can download. [21:06] EdwinGrubbs: have you looked at my related projects. I weep every time I see that page [21:06] * sinzui thinks team participation has no place in related projects or packages [21:08] sinzui: I don't think that is as big of a problem for people that are not in the Registry Administrators team. [21:09] rockstar: can you do a secondary ui review of https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-426899-ppa-links/+merge/13505 [21:10] I found a bug relating to it. jml had agreed with the issue. And scottk was concerned too. All 3 of us are concerned. rarely can one issue unite three disparate personalities like this one [21:11] EdwinGrubbs, sure. [21:13] EdwinGrubbs, looks good. === flacoste is now known as flacoste_afk === matsubara is now known as matsubara-afk [23:41] sinzui, huh what?