=== jelmer_ is now known as jelmer [07:10] hello! [07:10] fun & short patch here: https://code.edge.launchpad.net/~jml/launchpad/import-fascist-tweaks/+merge/16560 [07:13] hello jtv, bugging you re. https://code.edge.launchpad.net/~al-maisan/launchpad/pending-jobs-499861/+merge/16550 as requuested :) [07:22] hello jml, could we trade reviews please? [07:23] jml: I am looking at your import fascist branch right now :) [07:24] al-maisan, thanks. [07:24] al-maisan, which branch do you want me to review? [07:24] jml: https://code.edge.launchpad.net/~al-maisan/launchpad/pending-jobs-499861/+merge/16550 [07:24] al-maisan, will do. [07:24] jml: thanking you. [07:26] Does anybody know what happened about bug #500018? [07:27] wgrant: I guess nothing so far [07:29] al-maisan: OK, there was some discussion about finding somebody to cowboy it. [07:29] wgrant: I'll need to touch base with jelmer and/or get the hole plugged myself [07:30] wgrant: I will start on it in 20 minutes or so. [07:36] jml: the imports code you removed from lib/lp/soyuz/model/archive.py : were they just superfluous? [07:36] * jml looks [07:37] al-maisan, they were, after I deleted the two lines like: [07:37] - source_package_name = getUtility(ISourcePackageNameSet)[source_name] [07:37] al-maisan, those lines assigned to an unused local variable [07:37] al-maisan, and have no obvious side-effects [07:37] jml: I see, thanks! [07:42] jml: the changes in lib/lp/scripts/utilities/importfascist.py look reasonable, did you test them in any way? [07:43] al-maisan, the fascist itself doesn't have tests that I can find. [07:43] al-maisan, I've been running the fascist locally. [07:43] jml: but I guess running anything will exercise it [07:43] al-maisan, correct. [07:58] al-maisan, I've hammered & sent out an email reply to your review [07:58] al-maisan, to your patch, rather. [07:59] al-maisan, I'm going to get some food now, but I'll be back in a bit to handle round #2 [07:59] jml: thanks, I am just running "bin/test -vv -t mail -t archive" in your branch to be on the safe side [08:02] al-maisan, heh. thanks. though I was certainly going to submit it via ec2 test in any case. [08:06] jml: FYI, the test on line 2407 in doc/archive.txt is failing [08:07] http://pastebin.ubuntu.com/345778/ [08:08] al-maisan, I guess those lines do have side effects! [08:09] jml: yeah .. either deliberate or inadvertent ones :P [08:13] al-maisan, I see what's going on.... [08:13] al-maisan, getting the sourcepackagename object there gives a good error for those functions if it doesn't exist [08:14] but that's all it does [08:14] I'll remove the local assignment to keep pyflakes happy and add a comment saying why we do something that appears useless at first glance. [08:14] jml: ah, OK, it's a deliberate point of failure. [08:15] al-maisan, http://pastebin.ubuntu.com/345781/ [08:16] * jml really dining now. [08:18] jml: with the change in the pastebin above: r=me [08:19] hmm .. clicking on "Claim review" on https://code.edge.launchpad.net/~jml/launchpad/import-fascist-tweaks/+merge/16560 results in an OOPS [08:20] .. a reproducible one [08:20] http://pastebin.ubuntu.com/345785/ [09:03] al-maisan, I've heard mention of a similar bug... [09:04] jml: after refreshing the page, the review would appear to be claimed by me and I was able to finish the review approval workflow. [09:06] al-maisan, hmm. I can't find a record of that in the bug tracker. [09:06] jml: would you like me to add a bug? [09:06] al-maisan, yes please. assign it to thumper, I reckon. [09:06] jml: will do. [09:08] al-maisan, I'm just about to sign off for the year. Do you want to go one more round on the buildJobQuery branch first? [09:10] jml: ideally yes, I am about to reply to your review email. It will take another 5 minutes or so. [09:12] al-maisan, no worries. [09:16] jml: reply sent. [09:17] al-maisan, thanks. [09:19] al-maisan: hi. Do you have time for landing this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-492375/+merge/16279 ? Thanks! I have done a full test and everthing was ok. [09:20] adiroiban: yes, I'll do it in the course of the next hour or so.. [09:22] jml: fixes to the branch under review so far: http://pastebin.ubuntu.com/345813/ [09:37] al-maisan, replied. [09:37] al-maisan, if the classProvides / interface thing is too much, I guess adding a XXX would be OK, but I think that for this particular area of work, we should be trying very hard to avoid tech debt. [09:37] al-maisan, (he says by way of a postfix) [09:38] al-maisan, good night. I'll see you in the new year. [09:39] jml: thanks for looking at this, have a great break! === jtv1 is now known as jtv [12:00] henninge: I'm working on an MP for our buildfarm work... you up for a review? [12:00] al-maisan: which reminds me... did you find a reviewer for yours? [12:01] jtv: sorry no, my year is almost done. [12:01] hello jtv, yes, jml reviewed the branch but it would be good if you could rubber-stamp it once I have the changes he requested in place. [12:01] jtv: how long will you be available today? [12:01] al-maisan: ok, I'll be around for a few more hours [12:02] jtv: I should have something in the next hour or so.. [12:02] I'm just putting up my MP, though if you land an extension to IBuildFarmJob first, it will break my test. :-) [12:02] henninge: how about you? Are you working normal hours today? [12:03] jtv: that's a concern .. why would it break the tests though ? [12:04] there is a minimal implementation of the 3 methods added to the interface [12:04] al-maisan: because my test verifies that my IBuildFarmJob implementation really does provide all of the interface's API [12:04] oh, if there is a minimal implementation in BuildFarmJobBase then it won't break things [12:04] al-maisan: no, got the afternoon off. [12:05] OK [12:55] al-maisan: my mp is up [12:55] al-maisan: want to review each other mutually? [13:04] jtv: I can review your branch but mine is not ready yet [13:04] al-maisan: then you can't review mine either, because you've got other things to do. :-) [13:05] jtv: if you give me 30 minutes it may work out for both of us.. [13:05] \o/ [13:45] jtv: OK .. ready now .. please note that jml already approved of the branch (https://code.edge.launchpad.net/~al-maisan/launchpad/pending-jobs-499861/+merge/16550) .. [13:45] al-maisan: I was just reading up on the preceding review thread [13:45] .. provided I fix a few things [13:46] the fixes are here: http://pastebin.ubuntu.com/345918/ [13:46] .. and I am just pushing the branch [13:47] al-maisan: btw what exactly does virtualization mean here? That the slave must be in a vm? [13:47] yes [13:48] al-maisan: that'll become relevant when I update the branch I just put up for review to match the one you're having reviewed. :-) [13:49] jtv: as I said before: if your branch lands first I will add a perfunctory getPendingJobsQuery() to your class in my branch to avoid breakage [13:50] oh, I read that differently earlier... thanks. [13:59] al-maisan: "get" as a bit generic and over-used as an opening verb for method names... how about composePendingJobsQuery to reflect that it puts together a textual query? [13:59] (This is a verb I use a lot in these situations) [13:59] jtv: OK .. that's fine. [14:00] thanks [14:05] al-maisan: and in its docstring, the docs for min_score and processor end without punctuation. [14:05] jtv: will fix that. [14:05] thx [14:07] al-maisan: in the test you use processor='386'... not 'i386'? [14:07] yes, you can see it in the psql excerpt I have in the test as well [14:08] oh well, not our problem [14:08] jtv: line 232 for example [14:11] al-maisan: here's my mp btw: https://code.edge.launchpad.net/~jtv/launchpad/db-bug-499404/+merge/16564 [14:11] jtv: thanks -- I am free to look at it now .. sorry for the delays [14:12] stop apologizing! [14:12] * al-maisan apologizes for apologizing ;P [14:13] * jtv glares back [14:13] jtv: the changes you suggested: http://pastebin.ubuntu.com/345928/ [14:14] looks fine [14:15] jtv: thanks! [14:15] The comment you added in TestBuildPackageJob.setUp chains two "so that"s [14:15] Oh! [14:16] And I see that elsewhere you use x86 instead of 386 (or i386 or IA32 or 80x86...) [14:17] But not worth changing that last bit. [14:17] hmm .. ok [14:18] I didn't know about classProvides btw; it's a bit meta but it does look useful [14:18] Wonder how we test for it. [14:20] jtv: the tests in the branch exercise the method specified in the interface mentioned in classProvides() [14:21] al-maisan: I was thinking of "make a test verify that the class really provides the complete interface" [14:22] jtv: it's only one method in this case [14:23] True... sometimes though I come across methods whose interface definitions and model-class definitions have grown apart === jelmer_ is now known as jelmer [14:23] Imagine that you add a parameter to that one method: it'd be nice if all classes had tests verifying that they support the new parameter. [14:24] jtv: hmm ... I see what you mean .. /me starts grep'ing for classProvides in tests [14:29] * al-maisan finds from canonical.launchpad.webapp.testing import verifyObject [14:30] Who knows, it might work on classes and classProvides as well as objects and implements [14:30] hmm.. [14:30] def test_providesInterface(self): [14:30] """Ensure that BranchDiffJob implements IBranchDiffJob.""" [14:30] verifyObject( [14:30] IBranchDiffJob, BranchDiffJob.create(1, '0', '1')) [14:30] Yes, you normally pass it an interface and an object [14:31] I've been looking for mentions of interfaces that this is used for in tests, but zip so far [14:32] ah! lib/lp/code/model/tests/test_diff.py [14:32] verifyObject(IMyInterface, MyClass) [14:32] yep [14:38] jtv: the changes discussed: http://pastebin.ubuntu.com/345943/ [14:39] al-maisan: perfect, thanks [14:39] jtv: thank *you* [14:39] I'm about ready to give that rubber stamp now. :-) [14:39] jtv: you are one picky reviewer ;) [14:39] yup [14:40] * al-maisan feels compelled to type "meine Rache wird bitter sein" for some reason :-) [14:40] Hmm... nicht süß? [14:41] jtv: na ja, das auch :) [14:41] I've gone through enough horrible crap code to be really, really grateful and environmentally conscious about this stuff... [14:41] one of my favourites: [14:41] jtv: fully agree :) [14:41] for (i=0; i < result.size(); ++i) { [14:42] switch (i % 3) { [14:42] case 0: [14:42] blablah(); [14:42] break; [14:42] case 1: [14:42] foofoo(); [14:42] break; [14:42] case 2: [14:42] all specimen of finest code :) [14:42] barbar(); [14:42] break; [14:42] case 3: [14:42] foofoo(); [14:42] barbar(); [14:42] break; [14:42] } [14:42] return; [14:42] } [14:43] ah, there was an "if" as well [14:43] jtv: no goto? [14:43] * al-maisan is slightly disappointed :) [14:43] ah! no goto, but I forget the "default:" case [14:43] :-) [14:43] Note how the "case 3:" won't be exercised except in cases where you have much, much worse things to worry about [14:44] like broken CPUs? [14:44] or broken compilers [14:44] and this one was broken... oh God was it broken [14:44] jtv: if you continue like this your branch will never be reviewed .. [14:45] :) [14:45] * jtv swallows some great anecdotes about Microsoft's compiler [14:45] we can chat next year :-) [14:45] ok :) [14:45] in NZ [14:45] ok [14:48] al-maisan: I added my approval [14:50] jtv: thank you! [14:50] np [15:04] sinzui: ping [15:04] Hi EdwinGrubbs [15:04] sinzui: are you working today? [15:04] yes and no [15:05] EdwinGrubbs: I am hacking on some critical items, but I am also om my day of [15:05] off [15:05] EdwinGrubbs: How can I help you? [15:06] sinzui: just wondering if you wanted to review a 2810 line branch that only has 731 lines which are not formulaic changes, but that is not a good way to spend your day off. [15:07] sinzui: bye now. Have a good holiday. === salgado is now known as salgado-lunch [15:44] al-maisan: any remarks from the review so far? [15:46] jtv: none so far .. [15:47] jtv: thanks for cleaning up the doc strings for some of the IBuildFarmJob methods! [15:48] nearly removed them for fear of causing conflicts with your work :-) [15:49] jtv: why the "pylint: disable-msg=E0211,E0213" on line 119? [15:49] al-maisan: IIRC that's the warnings about "hey this method doesn't take a 'self' argument" [15:49] lemme check [15:49] thanks [15:51] al-maisan: actually, it seems I only need to suppress 213 [15:51] so I'll remove the other one [15:51] thanks again [15:53] my first thought re. `IBuildFarmJobBehavior`: do we really have to impose an implementation of this interface upon each job class? It seems pretty generic. [16:01] Yes, I was wondering about that too... but then again the best way to find out is to duplicate and then unify [16:10] jtv: yeah .. I guess there's no way around it. [16:11] I think it won't be too hard to factor most of it away, but first we'll have to find out what differences we need... [16:12] hmm.. [16:17] jtv: I like your '4010' job score :) [16:18] al-maisan: we may want to improve on it later, but right now I just statically computed what seemed the closest equivalent for package build jobs [16:18] as you said in the comment: if the duration is short it may not matter much .. [16:19] ..except if you flood the build farm with gazillions of these jobs [16:19] ...at which point we'll probably just want to tweak the score statically to keep them down a bit. :-) [16:22] jtv: what would happen if we dropped one of these on the build farm as it stands? Would TranslationTemplatesBuildBehavior.dispatchBuildToSlave() work? [16:22] al-maisan: my impression was that it wouldn't, because BuildQueue isn't really ready for that [16:23] (at least until your branch lands...) [16:24] Well, there's a much more fundamental build farm piece that needs adjustment: candidate job selection [16:25] that, too—though I guess that would not affect this particular method [16:25] the branch you approved previously is just a part of the work that needs to get done in order to have job dispatch time estimation working across different job types [16:25] right. [16:28] one problem is that I don't really know what affects what there; it's actually not unthinkable that the method might work as-is [16:29] jtv: OK .. so, I did not look at the db part .. the code part looks reasonable .. but if I approve it what have we gained .. not quite sure where we stand..? [16:29] [Nice: D'Addario colour-codes its guitar strings... no individually labeled packaging needed] [16:31] al-maisan: we'll have the BuildFarmJob part for our type of job basically ready. Next I add testing for the dispatch method; implement the method you just defined; and register the new job type in specific_job_types. [16:31] (Of course I'll also get a db review) [16:32] jtv: OK .. makes sense .. do you want me to approve the branch in that context? [16:32] al-maisan: not sure I understand the question [16:33] jtv: re. "register the new job type in specific_job_types": this part is in doubt, there are some other ideas floating around how this can be implemented in a cleaner way in BuildQueue [16:34] al-maisan: I understand... all the more reason why I didn't implement those in-flux parts yet [16:34] my impression though was that we have some pretty stable interfaces already, and those parts it seems to make sense to land [16:35] jtv: I have no qualms about the ITranslationTemplatesBuildJob* part [16:36] the build behaviour part though is more obscure and untested [16:36] just a minute please [16:36] * jtv strings up another string === salgado-lunch is now known as salgado [16:38] * al-maisan looks for any tests for dispatchBuildToSlave() [16:39] jtv: this is still very much work in progress and as such looks good, r=me (code) [16:40] al-maisan: wonderful, thanks [16:40] jtv: thank you! [16:40] I don't like going in with code that's not complete enough to be operational, but it's better than letting it bit-rot unmerged [16:40] jtv: yeah .. I understand [16:41] [Some things I do miss in this guitar: the monkey grip and having the tuning knobs all on one side of the head] [16:42] jtv: enjoy your break! [16:42] thanks, same to you too! I believe there's some custom-made egg-nog waiting for me [16:42] ..and thanks for bearing with me for such a protracted period of time ;) [16:43] no worries... kinda peaceful here tonight. Good night, happy holidays, & see you in 2553 BE! [16:43] * al-maisan is being called to the dinner table [16:43] * al-maisan will google "2553 BE" later :P [16:48] jelmer: you can submit the *Publishing fix with rs=bac if you want, assuming there is nothing else in it. [16:54] bac: Thanks! [17:39] jelmer: please reply to the email on the launchpad list about the buildbot failure stating you've submitted a fix. otherwise someone else may waste time investigating. [17:49] bac: ok [17:50] jelmer: do you know anything about the new flood of import warnings spewed by bin/test? [17:56] bac: No, sorry [18:30] jelmer: jml's branch 10090 changed the importfascist and caused all of the new warnings. [18:46] bac: yep, http://bazaar.launchpad.net/~launchpad-pqm/launchpad/devel/revision/10090 [18:47] al-maisan: i haven't looked into the warnings to see if they are accurate or not... [18:49] same here, [18:50] but jml's branch looked good. [18:50] * al-maisan needs to go [18:50] have a nice vacation!