i removed that rendered the padlock was leftover from the pre-3.0 design [13:33] ok, r=me === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:33] thanks [14:38] leonardr: Hi, can I add another branch to the queue? [14:38] rinze, sure [14:38] give me the link [14:39] https://code.edge.launchpad.net/~jelmer/launchpad/594237-arm-checkbox/+merge/28750 [14:39] leonardr: Sorry, I just noticed I proposed against lp:launchpad by accident [14:40] leonardr, I'll get back to you [14:41] sure [14:42] leonardr: https://code.edge.launchpad.net/~jelmer/launchpad/594237-arm-checkbox/+merge/28751 === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: rinze || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:00] rinze, i must ask some basic questions [15:00] what makes an architecture "restricted" [15:01] ? [15:01] leonardr: Some architectures (such as ARM) only have a limited number of slow builders so we don't want to make them available to everybody who is building openoffice in their PPA [15:02] ok [15:02] leonardr: Those architectures are marked as restricted in the database and archives that need to build on those architectures need an explicit entry in the archivearch database table. [15:05] rinze: [15:05] + terms.append(SimpleTerm(family, token=family.name, [15:05] 45 + title=family.title)) [15:06] i think our coding style requires putting all the arguments to SimpleTerm on a single new line [15:06] ah, ok [15:09] leonardr, I have an oversized branch... think it's something you could review? https://code.launchpad.net/~jtv/launchpad/recife-limited-setcurrenttranslation/+merge/28715 === henninge_ is now known as henninge [15:37] henninge: maybe you could review the branch we discussed? It's almost twice as big as it's supposed to be, I'm afraid; those tests are pretty exhaustive. [15:38] jtv, leonardr: I am taking it. [15:39] thanks henninge [15:51] rinze: what happens if you get the enabled_restricted_families_collection_link. are IProcessorFamily objects actually published in the web service? [15:52] leonardr: That's a good question, I hadn't actually considered that - just changed the arm_builds_enabled variable which was exposed. [15:53] leonardr, it looks like it doesn't actually work because of IProcessorFamily not being available so I should probably remove the exported() bit. [15:54] rinze: ok, either way you are breaking the backwards compatibility of the web service. [15:54] in the unlikely event that someone wrote a web service client that checks .arm_builds_enabled [15:54] we have two options [15:55] you can reimplement arm_builds_enabled as a property, export in 'beta', and de-export it in 'devel' [15:55] (i can show you how to do this) [15:56] or, we can decide that it's so unimportant that we can simply remove it retroactively with no worries [15:56] flacoste, your opinion? -^ [15:56] leonardr: Sorry, I'm being very clumsy today [15:56] rinze: np, it's a complex change [15:56] It looks like arm_builds_enabled wasn't actually exported, but I made a mistake in making enabled_restricted_families exported. [15:57] aha [15:57] ok, so simply don't export it, and there will be no backwards compatibility issues [15:57] you may want to export it later, but no need to do it now [15:57] flacoste, never mind [15:57] leonardr: ack [15:57] leonardr: right - and thanks for pointing that one out [15:59] rinze: r=me with changes. i'll summarize in the review [16:00] leonardr: thanks! [16:04] rinze, review written [16:04] be sure when you remove exported() to also remove the extra links from your tests === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [16:08] leonardr: will do [16:22] leonardr: could I add another (simpler) branch to the queue? [16:23] rinze, sure [16:23] leonardr: Thanks, the MP is at https://code.edge.launchpad.net/~jelmer/launchpad/394798-auto-buildd-secret/+merge/28759 === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: rinze || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === matsubara is now known as matsubara-lunch === sinzui changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: rinze || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:17] leonardr, may I hop on your queue? [17:17] rockstar, sure add it === rockstar changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: rinze || queue: [sinzui, rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:19] rockstar, give me the mp url as well [17:25] leonardr, putting together the mp right now. [17:25] great [17:26] rinze, what happens if you specify a build secret for a public archive? is that tested in code that you didn't touch? [17:27] leonardr: yeah, that's already disallowed by code I didn't touch (though I'm not sure if it's tested) [17:28] ok [17:28] r=me === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: sinzui || queue: [rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:29] sinzui, what's the branch you want reviewed? registry-tales-2? [17:30] leonardr, thanks [17:30] yes please [17:42] sinzui, r=me [17:42] thanks === matsubara-lunch is now known as matsubara === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: - || queue: [rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:56] leonardr, https://code.edge.launchpad.net/~rockstar/launchpad/delete-recipes/+merge/28773 [18:05] rockstar: i was just about to ask [18:15] rockstar, "if self.buildqueue_record:" should be "if self.buildqueue_record is not None:" [18:15] leonardr, ah, yeah, that's probably better. [18:16] rockstar: do test_destroySelf sets up a recipe build that has no associated build queue record, to test your new conditional? [18:17] leonardr, yeah, it does. [18:17] leonardr, I actually wrote the test to reproduce the bug first, and then fixed the code to make the test pass properly. [18:17] ok, r=me with that minor change === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === salgado is now known as salgado-lunch [18:20] leonardr, thanks. === salgado-lunch is now known as salgado [19:35] leonardr: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-576388-proposing-invited-team/+merge/28786 [19:54] edwingrubbs: sure === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [19:54] leonardr: I could use a review of https://code.edge.launchpad.net/~brian-murray/launchpad/595124/+merge/28543 [19:55] bdmurray: ok, you're in the queue [20:40] leonardr: I need to get some food. Do you have any questions for me about the branch before I go? [20:41] edwin: no, sorry, i'm just getting started [20:41] i'll put questions in irc if i have them [20:51] edwin: i would like to see your test show the string 'Accepted invitation whilst trying to propose the team' somewhere [20:52] also, 'while' should be just as good there as 'whilst' [20:53] in fact that whole sentence was hard for me to parse. 'Accepted an already pending invitation while trying to propose the team for membership" might be better [20:53] other than that, r=me === EdwinGrubbs is now known as Edwin-lunch === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: bdmurray || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [21:07] bdmurray, can you summarize the old behavior? it sounds like bugs used to say "this bug might expire eventually" and people interpreted this as "it's time for this bug to expire"? [21:08] leonardr: yes, that is what bug 595124 is about [21:08] <_mup_> Bug #595124: A bug's can_expire attribute is confusing [21:11] findExpirableTasks(days_old=60) finds bugs that have not had any activity in 60 days? [21:11] bdmurray -^ [21:13] in that case i'm confused about what isExpirable(days_old=0) means. it means that the bug can expire even if it was just changed? [21:14] leonardr: isExpirable(days_old=0) is what can_expire used to be [21:14] so might expire eventually [21:15] the thing i don't understand is where days_old=0 fits into that [21:15] It is for showing on a bug's web page that it will expire in XYZ days [21:16] for example bug 301020 [21:16] <_mup_> Bug #301020: Video no longer plays on S3 Savage in Intrepid

[21:17] ok, it says it will expire in 59 days [21:17] so isExpirable(0) must have returned true [21:17] but i don't understand the meaning of the zero in there [21:17] presumably isExpirable(61) would have returned false? [21:18] well 60 which is the configuration value yes [21:18] what is the difference between isExpirable(0) and isExpirable(60)? [21:19] AND Bug.date_last_updated < CURRENT_TIMESTAMP [21:19] AT TIME ZONE 'UTC' - interval '%s days' [21:19] it is passed to that ^ in findExpirableBugTasks [21:20] so isExpirable(0) gets a bug no matter when it was updated -- will this bug eventually expire [21:20] yes [21:20] isExpirable(60) only gets bugs that have not been updated in a long time, and 60 days is the de facto point at which expirable bugs that haven't been updated _are_ expired [21:21] correct [21:21] ok [21:22] in the test, you set a bug to invalid because an invalid bug won't expire? [21:22] right otherwise I would have changed all the other tests [21:23] apropos the tests you did change, i don't have enough context to know why all these bugs changed from EXPIRE=True to EXPIRE=False. which change caused that? [21:24] switching from "expire sometime" to "expirable" the 31 is the days old [21:24] ok [21:28] bdmurray, in incomplete-bugs, why did you switch from a test that adds a comment to a test that directly manipulates a field value? [21:29] because the comment would set date_last_updated to today which is less than 60 days ago [21:30] ok [21:30] one more question. in places like bugtask-index.pt, i assume you changed can_expire to context.bug.isExpirable(0) for clarity's sake, since the two statements are equivalent? [21:31] they aren't equivalent because can_expire uses 60 days now [21:36] ok, in that case you're changing the behavior of the web service in a way that's not backwards compatible [21:36] since some bugs that used to have can_expire=True will now have False [21:37] however, i don't think it's worth devoting a lot of effort to restoring the old behavior in old versions. flacoste, do you agree? [21:37] okay, is there a policy about that? [21:39] the policy is to change behavior only in the 'devel' version of the web service, and maintain the old behavior in all previous versions [21:40] and actually, this case might be one i didn't plan for [21:40] actually, nm, i have it [21:40] i hope flacoste will say this is not a big enough deal to maintain the old behavior, but if he says we have to maintain it, here's what to do [21:42] basically you will define two can_expire properties [21:42] can_expire and can_expire_beta [21:42] hi [21:42] whats the protocol to get something reviewed - put your hand up and wait ? [21:42] can_expire will be the one you have now, but you will only publish it in version 'devel' of the web service [21:43] lifeless: usually, except i'm at eod, so i won't be able to review your branch. i suggest picking a specific person who's around and asking them [21:43] ok [21:43] bdmurray: can_expire_beta will be implemented as per the old behavior, and you will publish it as 'can_expire' starting in the 'beta' version and remove it as of the 'devel' version [21:44] I'll start with a 'is there any one here up for reviewing two small refactoring branches' [21:44] :) [21:45] bdmurray: for an example, take a look at lazr.restful/src/lazr/restful/example/multiversion/resources.py [21:46] you'll see that a_comment first shows up in version 1.0 as 'comment' -- you'll want to make can_expire_beta show up in version beta as 'can_expire', and can_expire show up in version devel [21:46] you'll also see that 'deleted' is set to exported=False in version 3.0. you'll want to set can_expire_beta to exported=False in version devel [21:48] i will put this information into the review [21:48] leonardr: great, thanks [21:50] gary_poster: are you EOD yet? [21:50] on call [21:50] do you mean 'on a phone call' ? [conflation with 'on call' in the topic] === leonardr changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [21:53] lifeless, he's probably on the phone [21:53] flacoste, i'd like your opinion on bdmurray's branch https://code.edge.launchpad.net/~brian-murray/launchpad/595124/+merge/28543 [21:54] https://lpstats.canonical.com/graphs/LPProjectBugTriage/ [21:56] trending down [21:57] sinzui: perhaps you are around and not EOD ? [21:58] I am about [21:58] I wrote a couple of patches in the weekend [21:58] prompted by our discussion about the milestone page performance [21:58] I was following my nose into the code [21:58] they seem like improvements to me, and I'd like to get them reviewed. [21:59] https://code.edge.launchpad.net/~lifeless/launchpad/lsprof/+merge/28583 [21:59] and [21:59] https://code.edge.launchpad.net/~lifeless/launchpad/lognamer/+merge/28574 [21:59] if you have the time, some feedback would be great === matsubara is now known as matsubara-afk [22:00] I will review them now [22:00] thank you! [22:13] lifeless: thanks for changes. I need to run, but will look more closely later. [22:13] I misunderstood the goal of the lognamer branch (I thought you were tackling making the naming cross-process-safe) but now that I understand, cool. I like the new explanatory comment, but I don't think an XXX (which we exclusively mean to be associated with an LP bug) is appropriate myself, because I doubt we care enough to change it. It is more of a warning. [22:13] For the other branch, I also like the changes on first blush. You do have this comment: "". It's possible and even easy to do that if you know the right incantation...I don't remember but lemme see if I can find it really fast... [22:14] gary_poster: thank you [22:15] gary_poster: I'd be delighted to make it cross process safe; the XXX I can downgrade, but it is actually unsafe at the moment, and that should be recorded - theres an expectation mismatch presently. [22:16] lifeless: Apparently there is an expectation mismatch, but I've categorized that in my mind as a documentation/understanding problem, not a code problem. Arguable, and I won't argue it now. ;-) [22:16] gary_poster: :) [22:17] gary_poster: no need to argue, as you request I'll down grade it to a non-XXX note; I was trying to be clear is all [22:18] no, understood. I would have gone along with the XXX if you really wanted too (but with a bug) [22:18] ok [22:19] so, you need zcml:condition [22:19] gary_poster: bugs that folk don't have time to work on are a bit close to busywork ;) [22:19] agreed :-) [22:19] in your configure element, or in an include, you say something like zcml:condition="have apidoc" except you test on the devel state... [22:20] trying to remember how to do that [22:20] gary_poster: don't worry about it [22:20] ./zopeappsession.zcml:106 [22:20] OK [22:20] anyway [22:20] gary_poster: its currently on in prod [22:20] heh [22:20] removing it is orthogonal to making it event driven [22:20] ok [22:20] true === salgado is now known as salgado-afk [22:20] ok, running away [22:20] bye [22:20] I am likely to keep pulling on this thread to get actual kcachegrind profiles [22:20] ciao [22:20] yay! [23:09] sinzui: thanks for the reviews, I has replied. === Edwin-lunch is now known as EdwinGrubbs === krkhan_ is now known as krkhan