/srv/irclogs.ubuntu.com/2009/10/19/#launchpad-reviews.txt

=== jtv1 is now known as jtv
adeuringhenninge: could you please review this branch: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-class-udev-device-8/+merge/13554 ?10:07
adeuringintellectronica: ^^^?10:22
=== intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
intellectronicaadeuring: yes, looking at it now10:28
adeuringintellectronica: thanks!10:28
=== henninge changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
henningeI am here now ...10:33
henningeHI intellectronica! ;)10:33
intellectronicahi henninge, happy new week!10:33
=== intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: adeuring, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
intellectronicaadeuring: wouldn't it be better to use path expressions to find the nodes you want? less likely to make this sort of mistake11:06
adeuringintellectronica: I'm doing this now, I believe. Or do I miss something?11:07
intellectronicaadeuring: i'm looking at parent.parent.parent.parent and thinking out loud how we can avoid it11:08
adeuringintellectronica: that's hard... We simply need the grandparent of a device, in UdevDevice.scsi_controller11:09
intellectronicaadeuring: 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 it11:09
intellectronicayes11:09
adeuringintellectronica: yes, I know that it is not that simple. That's the reason for the sometimes long-winded comments ;)11:10
intellectronicaadeuring: 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 lot11:33
=== intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
intellectronicahenninge: would you like me to review your branch?11:34
adeuringintellectronica: thanks!11:34
jtvhenninge: 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:36
henningeintellectronica: thanks but I was impatient and asked jtv. He is familiar with the problem, anyway.11:38
jtvhenninge: 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
intellectronicacool bananas11:38
henningejtv: thank you11:42
jmlal-maisan, I'll look at that branch now.11:46
al-maisanjml: thanks11:58
henningejtv: http://paste.ubuntu.com/296752/11:59
jtvhenninge: fine12:00
=== intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: edwin, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
adeuringintellectronica, 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:31
intellectronicaadeuring: 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 long12:32
adeuringintellectronica: thanks!12:32
=== 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
intellectronicamaaan, 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 do12:47
bigjoolslies12:49
=== mrevell is now known as mrevell-lunch
intellectronicaadeuring: r=me13:33
=== intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
adeuringintellectronica: thanks!13:42
=== 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
adeuringintellectronica: fancy another sequel of this HWDB submissions stuff (a bit shorter this time: 177 lines diff)? 15:06
intellectronicaadeuring: sure15:06
adeuringintellectronica: thanks!15:06
adeuringhttps://code.edge.launchpad.net/~adeuring/launchpad/hwdb-class-udev-device-10/+merge/1356715:06
allenapabentley: 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/1357516:28
abentleyallenap: sure.16:28
allenapThanks.16:28
=== abentley changed the topic of #launchpad-reviews to: on call: intellectronica, henninge,abentley || reviewing: -, -, allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com
abentleyallenap: 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:36
allenapabentley: I don't follow.16:37
abentleyallenap: transports is an optional parameter to Branch.open_containing.16:38
abentleyallenap: It's only valuable if you pass it into another call later.16:38
allenapabentley: I was relying on the fact that reusable transports get appended to it, so I could disconnect them.16:39
abentleyallenap: 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
allenapabentley: Ah, okay.16:40
abentleyallenap: Are you sure you need to disconnect all the transports?16:41
allenapabentley: 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
allenapabentley: It may be that the assumption is wrong, but I want to try this because I don't have any better ideas :)16:43
abentleyallenap: 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:44
allenapabentley: 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:45
abentleyallenap: Right, so I think you can just do branch.bzrdir.root_transport.disconnect()16:47
allenapabentley: 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:49
abentleyallenap: No, that will just disconnect the ssh connection related to the branch you just opened.16:50
allenapabentley: Ah, I read that wrong! I read bzrlib.bzrdir.root_transport.disconnect, and I assumed root_transport was a module global.16:52
allenapabentley: Okay, thanks for that, I'll use that approach.16:52
abentleyallenap: Cool.16:52
abentleyallenap: 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
allenapabentley: Yeah, agreed.16:54
abentleyI can't say whether this approach will actually solve the zombie problem, but it looks fine otherwise.16:56
jmlal-maisan, hello16:56
al-maisanhello jml , just read your email16:56
al-maisanthanks very much for clarifying things16:57
jmlal-maisan, my pleasure.16:57
al-maisanso, the branch-sourcepackage relation is 1-on-1 .. but16:57
jmlal-maisan, there are two kinds of relationships between a branch and a sourcepackage16:58
al-maisanaha16:58
sinzuiintellectronica: henninge, abentley: Can one of you take https://code.edge.launchpad.net/~sinzui/launchpad/package-link-validation-1/+merge/13578 ?16:58
jmlal-maisan, a branch can *belong* to a source package16:58
=== 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
jmlal-maisan, (or a product, or be a junk branch)16:58
al-maisanyes...16:58
jmlal-maisan, alternatively, a branch can be the *official branch* for a (sourcepackage, pocket)16:58
intellectronicasinzui: sure, i'll review it16:59
jmlthe two concepts are independent.16:59
=== 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
jmlal-maisan, furthermore, a branch can be the official branch for more than one (sourcepackage, pocket) ISTR16:59
al-maisanjml: can be the can be the *official branch* for a number of (sourcepackage, pocket) items?16:59
al-maisanah17:00
abentleyallenap: AIUI, this method is called in a thread from a long-running process.  Correct?17:00
allenapabentley: Yes.17:00
abentleyallenap: Have you considered passing a transports parameter into it from the long-running process?17:01
jmlal-maisan, this is deliberate. I'm beginning to think it's also misguided.17:01
al-maisanjml: is it correct to assume that in such a (sourcepackage, pocket) collection a sourcepackage.distroseries only appears once?17:01
intellectronicasinzui: i don't understand the way you structured that conditional around line 32 of the diff17:01
abentleyallenap: That would presumably limit you to one SSH connection per long-running process.17:01
jmlal-maisan, I'm afraid not.17:01
intellectronicasinzui: why do you need an extra if?17:01
* sinzui should have annotated the cover letter17:02
al-maisanjml: how's that beneficial?17:02
allenapabentley: 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
jmlal-maisan, that's all you have to work with in terms of assumptions: http://pastebin.ubuntu.com/296938/17:02
al-maisanah17:02
jmlal-maisan, I'm not sure it _is_ beneficial :)17:02
al-maisanaha :)17:02
jmlal-maisan, but the code you write here has to work with our current model17:03
al-maisanjml: that's understod.17:03
abentleyallenap: I'm not sure how bzrlib reacts if the host goes away while a connection is open.17:03
sinzuiintellectronica: 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:03
al-maisanjml: hmm .. this looks a bit funny:  UNIQUE, btree (distroseries, sourcepackagename, pocket)17:04
abentleyallenap: 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
sinzuiintellectronica: The error gets its own message because the problem is locale to the series being edits17:04
jmlal-maisan, that means...17:04
intellectronicasinzui: but why `message="..."\n if message:...`? you don't need that if17:05
jmlal-maisan, I'm on a call I want to keep short. you'll have my full attention in a minute17:05
al-maisanjml: sure17:05
sinzuiintellectronica:sorry. I have the plague and I and thinking is a challenge17:05
allenapabentley: I think gary_poster must have had that problem because the custom BzrPoller we use overrides buildbot's to avoid concurrency already.17:06
al-maisanjml: 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
sinzuiintellectronica: That is crack I should have removed when I refacteded the branch between commits17:06
allenapabentley: Meaning: gary_poster wrote the custom poller afaict.17:06
gary_posterriht17:06
gary_posterright17:06
al-maisanmaybe all of them and be satisfied when I hit the first one that "passes"17:06
intellectronicasinzui: 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 begin17:07
intellectronicasinzui: ditto near line 4617:08
al-maisanjml: 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
sinzuiintellectronica: okay17:08
sinzuiI can add comments17:08
intellectronicacool17:08
abentleyallenap: I don't think Launchpad PQM Bot should be subscribed to code review email.17:09
allenapabentley: No, that seems odd.17:09
intellectronicasinzui: 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 canceling17:09
sinzuiindeed17:09
intellectronicasinzui: i would add another property, even though it's inefficient, just for readability17:10
allenapabentley: I don't have the power to fix that. You?17:10
sinzuiI can define next_url and and set cancel_url from it.17:10
abentleyallenap: Yes.  I've left it subscribed, so it can see the branch, but with no email options set.17:11
intellectronicasinzui: yeah, i think that's the best solution17:11
allenapabentley: 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:12
intellectronicasinzui: 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 distraction17:13
intellectronicai know what apache is, though sometimes i wish i didn't17:13
sinzuiintellectronica: yes. I struggled with that example17:13
sinzuiintellectronica: INCLUDE is a rare condition. upstreams rarely need to know about that17:14
abentleyI don't think a is true.  Transports are meant to be reused.  b might be true.17:14
intellectronicasinzui: 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 understand17:14
jmlal-maisan, back. really sorry about that. team standup.17:15
sinzuioh, good. I was tempted to do that, but thought saying more would help17:15
jmlal-maisan,  UNIQUE, btree (distroseries, sourcepackagename, pocket) just means that each SuiteSourcePackage has exactly one official branch.17:15
jmlal-maisan, the goal is for there to be official branches for every SuiteSourcePackage17:16
=== abentley changed the topic of #launchpad-reviews to: on call: intellectronica, henninge,abentley || reviewing: sinzui, -, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
intellectronicasinzui: 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 coma17:16
jmlal-maisan, as for which one to check17:17
jmlal-maisan, I think you should check all of them and the real question becomes whether the results are ANDed or ORed.17:17
sinzuiintellectronica: I agree with you. I should not have use a trailing comma.17:18
jmlal-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.17:20
=== salgado is now known as salgado-lunch
al-maisanjml: I am trying to think whether OR'ing them would be OK as well17:21
intellectronicasinzui: the rest looks great, so r=me with these changes17:22
sinzuiintellectronica: Here is the incremental: http://pastebin.ubuntu.com/296953/17:23
=== beuno is now known as beuno-lunch
jmlal-maisan, if a distroseries is closed for a pocket then it should be impossible to upload to the associated branch, I think.17:23
al-maisanjml: I agree, however, there's that one-branch-to-many (sourcepackage, pocket) relationship17:24
intellectronicasinzui: perfect17:24
=== intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, henninge,abentley || reviewing: -, -, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
jmlal-maisan, fwiw, with the real data, the relationship is in fact 1:117:25
al-maisanso, if any of the (sourcepackage, pocket) combinations become non-valid i.e. karmic/release after Nov. 1st17:25
al-maisanjml: that's good news17:25
al-maisanin which case we should reconsider the following constraint:17:25
al-maisan    "seriessourcepackagebranch__ds__spn__pocket__key" UNIQUE, btree (distroseries, sourcepackagename, pocket)17:25
jmlumm, don't you mean, "add a uniqueness constraint to branch"?17:26
al-maisanI thought a seriessourcepackagebranch contraint like (distroseries, sourcepackagename) probably makes more sense 17:27
jmlno, because that would make it impossible to have an lp:ubuntu/karmic-security/openssh branch and an lp:ubuntu/karmic/openssh branch17:28
al-maisanbut then it depends how seriessourcepackagebranch will be used17:28
al-maisanjml: maybe we could have brief voice call17:28
jmlal-maisan, sure.17:28
* al-maisan fires up skype17:28
jmlal-maisan, ?17:31
al-maisanjml: just a second17:31
intellectronicabac: need a reivew?17:32
bacintellectronica: yes, please.  i was waiting for the MP to show up before asking but got distracted.17:33
bacintellectronica: it'll be your easiest of the day17:33
intellectronicabac: so that's just an old test reinstated, right?17:33
bacindeedy17:33
bacintellectronica: and a smidgen of drive-by17:33
=== deryck is now known as deryck[lunch]
intellectronicabac: looks fine, r=me. thanks for remembering to reactivate that test17:35
bacintellectronica: thanks17:35
=== danilos is now known as danilos-afk
=== salgado-lunch is now known as salgado
leonardrintellectronica, henninge, or abentley: a very simple branch for one of you18:06
leonardrhttps://code.edge.launchpad.net/~leonardr/lazr.authentication/correct-scheme/+merge/1358618:07
henningeleonardr: I am on it18:07
=== 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
henningeleonardr: looks nice!18:14
henningeleonardr: shouldn't the base class do some check if self.AUITH_SCHEME actually exists or provide a default value?18:15
henningeOr am I being over-paranoid?18:15
leonardrhmm18:15
leonardrthere's no sensible default value18:15
leonardrif 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
leonardrso i think it's ok18:16
henningeleonardr: ok, but please add a comment to the base class saying that derived classes need to implement this.18:17
henningeto the doc string18:17
henningeleonardr: r=me18:20
rockstarhenninge, can I hop into your queue?18:31
henningerockstar: no queue, just forgot to update the topic18:32
henninge;)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
rockstarhenninge, okay, I'll send it directly to you then.18:32
henningerockstar: show me the mp18:32
rockstarhenninge, sending now.18:32
leonardrhenninge, abentley: even more trivial than my last branch: https://code.edge.launchpad.net/~leonardr/lazr.restful/0.9.13/+merge/1358818:34
henningeleonardr: r=me18:38
rockstarhenninge, https://code.edge.launchpad.net/~rockstar/launchpad/code-import-cvs-oops/+merge/1358918:41
rockstarhenninge, how's it going?18:48
henningerockstar: done, r=me18:48
rockstarhenninge, thanks!18:49
=== henninge changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: -, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
barryhenninge, abentley-lunch ping.  i have a very simple branch18:58
henningebarry: bring it on18:58
barryhenninge: thanks! mp'ing it now18:58
=== henninge changed the topic of #launchpad-reviews to: on call: henninge,abentley || reviewing: barry, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
barryhenninge: it's almost ridiculously trivial :)  https://code.launchpad.net/~barry/launchpad/mm2112/+merge/1359119:18
henningebarry: the trivality is soothing at this hour ... ;-)19:20
henningebarry: r=me19:20
=== abentley-lunch is now known as abentley
barryhenninge: thanks! :)19:21
=== 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
EdwinGrubbssinzui: 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:53
sinzuiEdwinGrubbs: I really think that the link to "PPA packages related to Curtis Hovey" should be (i) Related PPA packages20:56
EdwinGrubbssinzui: sounds good20:56
sinzuihmm. My page is wrong too20:57
sinzuiHas my hate for these pages evolved to apathy. I am not even surprised that they are wrong20:57
sinzuiWhy would motus trust these page?20:57
EdwinGrubbssinzui: which piece of information is wrong on your page?21:04
sinzuiEdwinGrubbs: That package in my ppa is superseded. I expected to see the one that users can download.21:05
sinzuiEdwinGrubbs: have you looked at my related projects. I weep every time I see that page21:06
* sinzui thinks team participation has no place in related projects or packages21:06
EdwinGrubbssinzui: I don't think that is as big of a problem for people that are not in the Registry Administrators team.21:08
EdwinGrubbsrockstar: can you do a secondary ui review of https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-426899-ppa-links/+merge/1350521:09
sinzuiI 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 one21:10
rockstarEdwinGrubbs, sure.21:11
rockstarEdwinGrubbs, looks good.21:13
=== flacoste is now known as flacoste_afk
=== matsubara is now known as matsubara-afk
jmlsinzui, huh what?23:41

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!