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