=== Ursinha-afk is now known as Ursinha [03:17] Edwin: I approved your branch [03:44] benji: r=me [03:44] sinzui: thanks === StevenK changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [benji,StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === Ursinha is now known as Ursinha-afk [05:14] StevenK: are you a reviewer? [05:17] EdwinGrubbs: I am, but my mentor is on holidays [05:17] * mwhudson can mentor something if it's easy [05:18] (not because i'm lazy but because i need to disappear fairly soon) [05:19] StevenK: well, I want you to look at the branch since it is a soyuz testfix for db-devel. [05:20] EdwinGrubbs: Sure, point me to the MP? [05:24] StevenK: here it is: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/broken_test_do_not_copy_disabled_dases/+merge/37815 === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [benji,StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [05:24] Oh, that's my fault :-/ === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [05:25] EdwinGrubbs: The code + test that was on db-devel pre-dates the devel code, and I was meaning to fix it, but sleep got in the way. [05:29] EdwinGrubbs: So, I can tell you my ideas on the MP, or I can just fix it since it's my fault. Which do you think? [05:30] StevenK: you can just fix it. [05:32] EdwinGrubbs: Okay, doing so, thanks. [06:53] StevenK: Quick Soyuz one, if you're interested: https://code.edge.launchpad.net/~wgrant/launchpad/bug-655648-a-f-maverick/+merge/37820 [06:55] wgrant: Added my code* to it [06:57] StevenK: Thanks. [07:47] Hi stub, would you have time for a tiny db-review? https://code.launchpad.net/~michael.nelson/launchpad/638090-base_version-property-for-differences/+merge/37742 [07:59] * stub has a look [08:06] noodles775: Might we need to join or lookup rows using base_version ? I'm wondering if it needs an index or not. [08:10] stub: Hrm... I can't think of an example. Currently the use-case is, here's a (loaded) difference, do you want to request a PackageDiff to be generated between the base version and the derived version etc. But right, it might save pain in the future when we add a query and assume an index? [08:21] noodles775: Sounds like we don't need it. [08:23] noodles775: r=stub patch-2208-24-0.sql [08:23] Great, thanks stub! [09:28] hi henninge [09:35] Hi bac! [09:36] * henninge looks at activereviews [09:36] hi henninge, could you have another look at my MP? sinzui did the UI mentor [09:37] henninge: i'm also having some trouble with a translation test and need some domain-specific geniusity [09:40] oh, is there a planned downtime for code hosting atm? [09:41] bac: I imaging the help link looks just like what it looks on the project page, right? [09:41] codehosting is back [09:41] yes indeedy [09:43] bac: r=me [09:43] bac: what's the failing test? [09:44] thanks [09:44] henninge: i'll paste it in a sec [09:44] henninge: when using factory.makePOTemplate it looks like sourcepackagename is non-optional, right? [09:46] if so that is, uh, dumb [09:48] henninge: http://pastebin.ubuntu.com/507871/ [09:49] henninge: the failure is at line 116 [09:49] and it is barfing on this part of the page template: [09:50] [09:50] full list of templates. [09:50] templates do not exist [09:50] henninge: so i think i have not done something that is required in the setup [09:58] henninge: you still around? [09:59] bac: sorry, got called awy. back now [09:59] np [09:59] henninge: you can see the failure in devel with [09:59] bin/test -vvm lp.translations -t TestRobotsDistroSeries [10:00] if you download that test [10:01] the setup looks ok === maxb_ is now known as maxb [10:06] bac: I wonder if the distribution needs official_rosetta set [10:06] hmm [10:06] but I am still preparing to run the test [10:06] bac: can you try ubuntu (but a new distroseries)? [10:08] henninge: in that test (look at set_usage in the base class) i do set the translation_usage property, which sets official_rosetta [10:10] oh, that's what the test is about ... ;-) [10:10] * henninge is catching on [10:13] bac: which branch is this on? [10:14] * henninge downloaded the wrong branch [10:15] henninge: lp:~bac/launchpad/bug-652280-pg-trans [10:15] give it a second as i'm pushing [10:18] and done [10:19] branching [10:33] bac: ok, I see the failure but am not any wiser yet. [10:34] henninge: i see the menu item is enabled_with_permission(launchpad.Edit) [10:34] bac: ah yes, that is true ;) [10:35] henninge: the template does not check permissions. but if that is the case i'm not sure how this works at all [10:35] bac: can you point me to the template, please [10:35] distroseries-translations.pt (i think) [10:37] sounds sensible [10:41] bac: hm, I can see ubuntu/maverick/+templates, even when logged out. [10:41] yeah, i just saw that [10:50] bac: btw, you can do without the test_suite at the end if you turn your "Base" into a "Mixin" [10:51] henninge: you mean make it not be a TestCase? [10:51] yes and make the actual testcases inherit from (TestCase, Mixin) [10:53] i'm +0 on that. maybe -0 [11:00] ;-) [11:00] Why is that? [11:05] bac: the test has no problems rendering +templates [11:06] henninge: i don't think actually rendering +templates is an issue, it is using the menu to simply construct the URL [11:06] * bac baffled [11:07] bac: removing the permissions check from the menu item does not help either. [11:08] bac: so clearly it is accessing the wrong menu. [11:08] henninge: and removing the section from the page template results in another KeyError for language_packs...similar construct [11:13] bac: do you know how menu lookup works? [11:14] I mean, how "context/menu:..." works? [11:15] yeah, i think so [11:16] henninge: it is found through an adaption of the 'usedfor' interface on the given facet [11:17] henninge: was that sufficiently hand-wavy? [11:18] bac: yes, thank you ;) [11:18] It just explained why I don't see a connection made in zcml [11:18] henninge: odd. i did just put a breakpoint in the 'templates' menu item...and it was not hit [11:19] bac: makes sense. if the tempalte does not see it it will not call it. [11:20] duh [11:20] bac: I'd ust like to get a "repr" of the menu that is actually used in the template [11:20] s/ust/just/ [11:23] bac: TranslationUnavailable: Translations for this release series are not available yet. [11:23] * henninge greps for TranslationUnavailable [11:24] huh? [11:24] where'd that come from? [11:25] bac: I commented out both problem sites in the template. [11:25] lib/canonical/launchpad/webapp/error.py:class TranslationUnavailableView(SystemErrorView): [11:26] henninge: so do you see something i'm missing in my setup that would cause translations to not be available? [11:26] bac: check_distroseries_translations_viewable [11:27] distroseries.hide_all_translations [11:27] bac: I'll try setting that to false [11:28] yes, it is true by default [11:29] hmph [11:29] bac: no joy :( [11:34] i made that change to set hide_all_translations=False. now i get a ComponentLookupError on +hierarchy on all tests [11:35] bac: oh, really? I just get the old error. But I also reverted all my other changes. [11:35] danilos: ! [11:36] henninge, hey [11:38] danilos: bac created a new distroseseries on a test and tries to render +translations on it but it fails. What could be missing? ;) [11:38] henninge, sorry, otp with bigjools [11:38] :( [11:38] danilos: np [12:04] henninge: thank you for your help. i must EOD now in bitter defeat. [12:06] bac: sorry but tomorrow is s a new day ... ;) === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [12:56] StevenK: There's no branch for you on launchpad/+activereviews, so is your queue position stale data? [13:07] * allenap assumes it's stale. === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bryceh || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === matsubara-afk is now known as matsubara === Ursinha-afk is now known as Ursinha === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:43] allenap: Can I get a review for https://code.edge.launchpad.net/~gmb/launchpad/launchpadformify-subscribe-view-bug-651240/+merge/37850 please? [14:43] gmb: Certainly :) === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:45] Ta.] [14:46] allenap: the changes in your branch look good. But what is the reason for the changes? Do you assume that the SQL query for the old method would be too slow? [14:48] adeuring: It's easier to select the targets elsewhere, and it's makes the resulting code closer to the existing structural subs code, so it's easier to reuse. [14:48] adeuring: My follow-on branch is now simpler to implement. [14:49] allenap: ok. But now you may get a subscription more than once, if somebody is subscribed to two or more bug targets having bug tasks for a given bug [14:50] adeuring: That turns out to be a problem. So far :) [14:50] adeuring: That turns out to *not* be a problem. So far :) [14:52] allenap: interesting. But anyway, r=me; the possible duplicate results can be treated later [14:52] wgrant: moving here.... [14:53] wgrant: test_ftparchive.py doesn't seem to test anything? [14:55] adeuring: Thanks :) [14:58] bigjools: It tests that a disabled arch doesn't make it into the a-f config. It's from before I had the other tests -- it's redundant now, but more direct. I'm not sure if I should remove it or not. [14:59] wgrant: I can't see any new assertions in it, just setup [14:59] bigjools: The assertion is a-f config file comparison. [15:00] wgrant: I can't see any comparisons [15:00] It's probably a bad place to put it, but the rest of the tests are so black-box that it seems bad to rely on solely on them. [15:00] lines 81-89 in the diff [15:01] sets up the disabled das and re-generates config [15:01] bigjools: I'm adding a new disabled distroarchseries. The point of the test is to show that it doesn't show up in the a-f config file. So the a-f config comparison does not change from the original test. [15:02] wgrant: that's not a good test, since it works with or without that setup [15:02] Right, exactly. [15:02] But I don't see how else to do it. [15:02] Except for the even blacker-box approach of the other test. [15:03] add a direct assertion that it's not in the config [15:04] Eh. I suppose. [15:04] well, that's a bit shit too [15:04] It is. [15:04] This part of the publisher is, sadly, shit. [15:05] And the tests are worse. [15:05] so, we need a test that it does appear when enabled, and not there when disabled [15:05] Ideally. I could just drop this test, since it's covered by the other one now. [15:05] true [15:06] they are better tests [15:06] since you have negative and positive [15:06] They are. But it feels a little awkward to be testing this behaviour only at that very high level. [15:06] But I guess the code leaves us little choice. [15:07] my only comment about them is that they do a lot of asserting and if one fails you won't see the results of the others [15:07] so if you can think of a way to assert all at once that would be better [15:07] That would certainly be a good thing. But I don't know of any easy way. [15:08] So, I'll drop the test_ftparchive test. [15:08] one way is to make two lists and compare them [15:10] I would love to improve the test, but we sort of need this code soon, and I need to leave for uni in a little over 6 hours. So I might not. [15:10] that's ok [15:11] ditch the a-f test then [15:11] and I'll land the branch [15:11] set the MP to "needs review" though :) [15:12] Pushing... [15:13] stab stab stab dogfood [15:13] It's still going? [15:13] yes. perhaps it needs more coal. [15:14] It seriously cannot take that long to dump 100000 rows from a view. [15:14] Or are there other dirty pockets? [15:15] well, it's chugging on lucid. Perhaps I should restart with -s maverick [15:15] :/ [15:15] That may be a reasonable idea. [15:15] le sigh [15:16] bigjools: Diff updated. I am happy enough with the branch now. [15:17] ok [15:17] wgrant: I'll test and land, thanks for writing it [15:18] Thanks. [15:18] A little unfortunate that I couldn't avoid that wonderful [7:] [15:19] shrug :/ [15:19] But I tried to block it in a few other places, and they wouldn't work, so... [15:21] so it was quicker this time [15:22] the i386 Packages didn't get re-written though :/ [15:22] I'll try again, the ctrl-c may have done nasty stuff [15:23] Ugh. [15:23] There'll be no dirty pockets. [15:23] You'll need to flip one of the pubs back to Pending. [15:23] yes [15:23] or upload a new package, which is a better test [15:23] But that'll take another 10 hours. [15:23] Anyway, I should sleep. [15:23] nope [15:23] Good luck with that... [15:24] 5 mins [15:24] thanks wgrant [15:24] Thanks for fixing my rather spectacular fuckup. [15:24] it's not quite done yet, we need to send grovelling emails to PPA owners [15:24] but enough of making you feel bad for one day :) [15:25] g'night [15:25] Most of them are probably already ancient, unnoticed history. [15:25] But we need to check that out, yeah. [15:25] Night. [15:30] allenap: How's that review looking? It contains a fix for some of the current buildbot breakage so the sooner it's RTL the better. [15:30] (I'll split that bit out if necessary, though). [15:31] gmb: I'm about half way through it, but it's going quickly. I'll put the pedal to the floor though :) [15:31] allenap: Ta. [15:42] gmb, was the test wrong? is the subscription to mozilla-firefox (Ubuntu)? [15:43] sinzui: Well, I'm just looking now, and I'm getting the opposite results locally. It should be to Mozilla Firefox, not mozilla-firefox (Ubuntu) because the Mozila Firefox task is the primary task for the bug and we subscribe to the bug not the task. [15:43] ah [15:43] sinzui: So, the test passes as it is in stable on my local machine. Change it to fix the BB failure and it fails locally. [15:45] I think my brain may melt out through my ears shortly. [15:52] gmb: Sorry, I fscked up your review by missing the prerequisite branch. Approved though. === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:52] allenap: Thanks. [15:53] allenap: So _subscribers_for_current_user is used quite a bit in the subsequent branch, so I'm happier to leave that as-is. [15:54] allenap: And _isSubscriptionRequest() is vestigial and no longer in use. [15:54] gmb: Yeah, those comments of mine need a lot of salt today. [15:54] (Which I just noticed) [15:54] allenap: Fair enough. I'll remove one and leave the other, then. === noodles775 changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [16:02] Hi allenap, do you have time for https://code.edge.launchpad.net/~michael.nelson/launchpad/656166-expose-dsd-diffs/+merge/37858 [16:02] noodles775: otp, but sure. [16:03] Thanks. [16:05] * noodles775 fixes conflicts === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: noodles775 || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === deryck is now known as deryck[lunch] [16:14] jml: I have a fix for the xx-person-subscriptions failure if you'd like to cast a weather eye over it; https://code.edge.launchpad.net/~gmb/launchpad/stable-unfark/+merge/37860 [16:16] gmb: thanks. looking now. [16:21] allenap: ok, the MP is now updated and without conflicts. [16:22] noodles775: Cool. === matsubara is now known as matsubara-lunch === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === deryck[lunch] is now known as deryck === salgado is now known as salgado-lunch === allenap 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 === matsubara-lunch is now known as matsubara === salgado-lunch is now known as salgado === gary_poster_ is now known as gary_poster === benji is now known as benji-lunch [19:20] EdwinGrubbs, ping [19:22] EdwinGrubbs, do you have time to review [19:22] https://code.launchpad.net/~sinzui/launchpad/mlist-sync-0/+merge/37885 that fixes an oops? [19:23] sinzui: sure === salgado is now known as salgado-afk [19:50] sinzui: r=me [19:51] thanks EdwinGrubbs === benji-lunch is now known as benji === EdwinGrubbs is now known as Edwin-lucnh === Edwin-lucnh is now known as Edwin-lunch === ajmitch_ is now known as ajmitch === matsubara is now known as matsubara-afk