/srv/irclogs.ubuntu.com/2009/09/30/#launchpad-reviews.txt

al-maisanGood morning06:58
noodles775gmb: I'm just updating those comments in the js-test - you're right, I can see how it's now much easier to read as a dev. :)07:36
=== allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
noodles775allenap: I'll have one ready for you in 5 :)09:06
=== noodles775 changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com
allenapnoodles775: Jolly good :)09:07
noodles775allenap: thanks! https://code.launchpad.net/~michael.nelson/launchpad/432152-orig-file-counts/+merge/1264509:32
allenapnoodles775: Cool.09:32
=== allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: noodles || queue: [] || This channel is logged: http://irclogs.ubuntu.com
allenapnoodles775: I don't understand the query really. It reads like you're selecting distinct LFAs.filename where the corresponding content is the same size.10:16
allenapnoodles775: I don't understand how that can be a valid thing to ask for, so maybe that's explained by the other clauses in the query.10:17
noodles775allenap: I'm just wanting distinct filenames to then sum the size - it just so happens that when the lfas with the same filename will also have the same size.10:19
noodles775allenap: the unit test might help to demo what it's doing?10:19
allenapnoodles775: I should probably read it :)10:19
allenapnoodles775: If I apply http://pastebin.ubuntu.com/282003/ to your branch, the tests still pass (note the differing filecontent). Should that happen?10:30
* allenap will be back in ~5-10 minutes.10:31
noodles775allenap: yes, that's correct.10:31
noodles775allenap: it should fail if you change the name.10:32
noodles775allenap: ah, I see... the filecontent is the actual content, not the length, so if you use filecontent='222' (ie. 3 bytes) it should also fail.10:32
allenapnoodles775: When does it not matter if the content is different but the size the same?10:42
noodles775allenap: the query is only checking the filesize - not the filecontent.10:42
noodles775For publishing tarballs/dsc files, (according to cprov), the fact that the filename is the same is enough to ensure that they are the same file (in that archive).10:43
allenapnoodles775: Ah, okay, that's what I was looking for :)10:43
noodles775But it might make it clearer if we also include the LFC.sha1 in the query, so it's obvious that that's what's happening (I tried to clarify that in the comment, but it's still confusing).10:43
allenapnoodles775: Yeah, including the sha1 will probably document this better, and will also mean that future developers don't suspect a smell when there isn't one because of the data model.10:45
noodles775allenap: great, I'll do that then.10:46
allenapnoodles775: Thanks, r=me, with one other suggestion, which you are welcome to ignore if you don't like it.11:02
=== allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
noodles775Thanks allenap !11:02
=== jtv changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com
stuballenap, jtv: https://code.edge.launchpad.net/~stub/launchpad/librarian-gc/+merge/12650 , https://code.edge.launchpad.net/~stub/meta-lp-deps/updates/+merge/12322 , https://code.edge.launchpad.net/~stub/launchpad/sanitize-db/+merge/1252012:06
stub(in order both smallest to largest and in priority too)12:06
jtvjust when the lunch gong goes...12:06
jtvallenap: shall I take that last one then?12:06
allenapstub: Is the first the highest or lowest priority?12:07
stubfirst is highest since it might need to be cherry picked.12:07
allenapjtv: Perhaps you should do the first then.12:08
jtvallenap: okay12:09
allenapjtv: I am about to go to lunch too :) But I'll pick up as soon as I'm back.12:09
=== jtv is now known as jtv-afk
=== allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: - || queue: [stub, stub, stub] || This channel is logged: http://irclogs.ubuntu.com
allenapSorry stub, looks like both of us are afk now. Nothing to do with you, well, on my part at least :)12:10
stubk12:10
=== jtv-afk is now known as jtv
thumpersinzui: https://code.edge.launchpad.net/~thumper/launchpad/rehash-branch-index/+merge/1265112:25
jtvstub: I'm surprised to hear that the librarian-gc branch fixes anything (unless content_id is None as well), since AFAIK None < anything12:29
stubNone < some_integer == True12:30
stubWe don't want that test to pass if next_wanted_content_id is None12:30
jtvstub: oic12:31
stubOr we would be reporting 'LibraryFileContent None exists in the database', or we would if it didn't crash when trying to format None as an integer.12:31
jtvstub: I can't help feeling that that loop would be a lot prettier with the config check outside it, but if this is for CP...12:34
jtvIt would be good to have a test though.12:34
stubjtv: Hmm.... I don't see that working as I still need to lookup the next wanted id even if there is an upstream librarian12:36
stubI was hoping nobody noticed the lack of test ;)12:37
stubNow I have to work out what actually triggers this case :)12:38
stubI think it occurs if the highest id isn't wanted12:38
jtvit can avoid disappointments...12:38
stub(Hmm.... that should be a generator really)12:39
jtvstub: to get me into the right context: is this a CP candidate?12:45
stubjtv: Yes12:46
stubI can try adding a test in a minute12:46
jtvstub: I'd appreciate that12:47
=== noodles775 changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: - || queue: [stub, stub, stub, noodles] || This channel is logged: http://irclogs.ubuntu.com
stubOh sod off. I know there is another mp for this branch.12:57
stubfacist review system :-P12:57
=== jtv changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: —, stub || queue: [stub, stub, noodles] || This channel is logged: http://irclogs.ubuntu.com
stubCan http://paste.ubuntu.com/282095/ be reviewed without a mp ?12:58
jtvstub: you could stick a tinyurl on the queue13:03
stubnp. worked out how to do the resubmit.13:03
stubhttps://code.edge.launchpad.net/~stub/launchpad/memcache/+merge/1265313:04
jtvstub: that second branch has a review request for Francis... do I take it you don't want to wait for that?13:05
stubIf you know anything about debian packaging, sure.13:05
jtvstub: rusty, but this looks pretty trivial.  Why don't you tell me what could go wrong?13:06
stubI just assigned francis as he suggested this workflow and it sat there for a few days ignored ;)13:06
stubjtv: I have no idea what could go wrong. I don't even know what happens after it gets merged in to build the package ;)13:06
jtvstub: well my apt has no objections to installing memcached, so you've got my imprimatur13:08
jtvstub: any progress on test?13:09
stubjtv: Nah - reject it and I'll resubmit when I have something13:09
jtvstub: as you wish13:09
=== matsubara-afk is now known as matsubara
noodles775allenap, jtv: just in case you get to it before I'm back, my MP is here: https://code.launchpad.net/~michael.nelson/launchpad/436182-newlines-in-sources-bz2-indices/+merge/1265513:20
allenapnoodles775: Cool.13:20
=== allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: stub, stub || queue: [stub, noodles] || This channel is logged: http://irclogs.ubuntu.com
jtvnoodles775: in call13:22
jtvwill get back later13:22
=== allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: stub, ☎ || queue: [stub, noodles] || This channel is logged: http://irclogs.ubuntu.com
deryckI want a ☎13:56
deryckallenap, hi13:56
allenapderyck: Hi13:57
deryckallenap, I have a small branch, but can I leave it on the queue for offline review.  About to return to tl sprint stuff.13:58
allenapderyck: Okay, cool.13:58
deryckallenap, thanks!13:58
deryckallenap, never mind.  gpg fail on new laptop setup.  will catch up later about it.13:59
allenapstub: Does ALTER TABLE not implicitly commit a transaction?13:59
allenapderyck: Heh :)13:59
stuballenap: No. PG is entirely transactional that way.14:14
=== jtv changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: stub, — || queue: [stub, noodles] || This channel is logged: http://irclogs.ubuntu.com
jtvallenap: which one for stub is still on the queue?14:28
allenapjtv: https://code.edge.launchpad.net/~stub/launchpad/memcache/+merge/12653 I think14:28
jtvallenap: ok14:28
=== jtv changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: stub² || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com
jtvstub, about the memcached test: why not self.assertEqual(self.want_memcached, is_live)?14:36
stubjtv: Better error messages14:37
jtvI guess...14:40
jtvSomething that bugs me about layers: can't we have split-transaction startup & teardown?14:41
stubWhat do you mean?14:42
jtvSo e.g. memcached can start gearing up before Librarian has completed its startup, or vice versa.14:43
jtvPut those extra cores to work.14:43
stubjtv: Not out of the box.14:43
jtvShame.14:44
jtvOn a different note: in Memcachedlayer.setUp, you assign test_key twice, sort of defeating the purpose.14:44
stubjtv: We could do it ourselves if we wanted - launch stuff in separate threads and block until they are all ready, but nobody has tried that yet.14:44
stubI did?14:44
jtvWell somebody did14:44
stubfixed14:45
jtvstub: http://paste.ubuntu.com/282095/ lines 150 & 16814:45
jtvI'm not a huge fan of firing off threads for this sort of thing, since it gets hard to manage.  But why sit around looping for your service to become available when you could just return and let the next service start?  Managing dependencies is a bitch, of course.14:47
stubI wouldn't want to bother until we migrate to something superior to layers anyway - no point making the change harder than it already is.14:48
jtvsomething I don't understand about your atexit handler: how does its default argument get handled?  How does it avoid dereferencing MemcachedLayer._memcached_process if that's already been torn down?14:50
jtvreview meeting in 10...14:50
stubjtv: The default value for the pid argument is evaluated when the function is defined.14:52
stub(Not when the function is invoked)14:52
jtvstub: Ah.  Also, since the atexit handler is one of those things you won't want to test, can you share a bit more of the killing code between the two places where it's written?14:52
stubWhat would you share?14:54
stubI'd like the tearDown to raise exceptions when things screw up. The atexit is just a second chance of not leaving random processes running if things go wrong.14:54
jtvhmmm... yeah, not much to share14:56
jtvonly "if pid is not None: os.kill(pid, signal.SIGKILL)"14:57
jtvIt would make tearDown a bit nicer to read though, since you'd be rid of the line break in the "if" condition.14:58
stubI personally prefer that than the extra indirection14:59
jtvok, no particularly strong feelings.15:00
jtvstub: oh, it's not in the pastebin but above Memcachedlayer.launch you had a double blank line.15:01
jtvWhereas one is missing above MemcacheClientTestCase15:02
jtvstub: are you in the reviewers meeting?15:03
stubWhich file is that?15:03
stubnah15:03
jtvlib/lp/services/memcache/tests/test_memcache_client.py15:03
stubfixed15:04
jtvstub: ok, then r=me15:05
stubTa. Now if I can work out how to get lp-dev-deps rebuilt I might one day be able to land it ;)15:05
jtvI _would_ say "let the Foundations people worry about that," but...15:07
allenapstub: In line 194 of http://paste.ubuntu.com/282095/ it should be is *not* None I think.15:11
=== allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: noodles, stub || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com
=== allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: noodles, stub || queue: [] || This channel is logged: http://irclogs.ubuntu.com
stuballenap: yup. fixing.15:23
allenapstub: Cool. I commented on the bug too, just in case you'd gone, not because I like to harass you ;)15:24
=== jtv changed the topic of #launchpad-reviews to: on call: allenap, jtv || reviewing: noodles, — || queue: [] || This channel is logged: http://irclogs.ubuntu.com
=== EdwinGrubbs changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: noodles, — || queue: [] || This channel is logged: http://irclogs.ubuntu.com
jtvallenap: I've got a pretty urgent MP myself... shall I take it?  Or chuck it on the queue?  :-)15:26
jtvoh, EdwinGrubbs maybe?15:26
allenapjtv: I'm doing noodles' review now, but I can push yours on the stack.15:27
EdwinGrubbsjtv: which review is it?15:27
jtvallenap: stack?  Not queue?15:27
jtvEdwinGrubbs: redo-uploads15:27
allenapjtv: I'm a stack based machine.15:27
jtvuh-oh15:27
allenapjtv: Don't worry. I rarely overflow.15:28
jtvtmi dude, tmi15:28
jtvEdwinGrubbs: this one: https://code.edge.launchpad.net/~jtv/launchpad/redo-uploads/+merge/1265915:29
=== jtv changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: noodles, deryck || queue: [] || This channel is logged: http://irclogs.ubuntu.com
EdwinGrubbsjtv: ok, I'll do it.15:29
jtvEdwinGrubbs: thanks!15:29
=== EdwinGrubbs changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: noodles, —, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com
=== jtv changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: noodles, deryck, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com
jtvtopic serialization failure15:31
jtvderyck: what if a bug summary is a full sentence?15:33
jtvderyck: I sometimes paste an entire error message into a bug search, for instance.15:33
allenapnoodles775: In bug 436182, it says "there's a superfluous newline after libuno-cli-cppuhelper1.0-cil", but it's actually after the comma that follows libuno-cli-cppuhelper1.0-cil. Could that mean that it's really a leading newline in libuno-cli-ure1.0-cil?15:35
mupBug #436182: Newlines in Sources.bz2 indices <oem-services> <soyuz-upload> <Soyuz:Triaged by michael.nelson> <https://launchpad.net/bugs/436182>15:35
jtvderyck: Is "ValueError: The date you gave was in a timezone that is not divisible by 12 minutes, which somehow matters to some hypohetical piece of code." one of these?15:35
noodles775allenap: that seems to be just the cause of the issue - what happens then is that we (soyuz) incorrectly parse it and add a newline to the end of the field (dsc_binary in this case), as demo'd in the test.15:36
noodles775allenap: cprov did a separate query showing that the relevant SourcePackageReleases have a '\n' appended to the end of the dsc_binaries field. We should add that to the bug.15:38
deryckjtv, I think that's ok.  That's why I used quotes.  It's hard to echo on the page nicely, and beuno did sign off on the layout, maybe he has ideas.15:45
beunoderyck, maybe "Is this the same as: $summary"15:47
beunoand no quotes?15:47
deryckbeuno, but what is "this"?15:47
allenapnoodles775: This change will affect every field in the index stanza. That sounds like a good bit of belt-and-braces engineering. Might even be worth .strip()ing, not just .rstrip()ing.15:47
beunoderyck, "Is this bug the same as: $summary?"15:47
noodles775allenap: I initially had it just on the dsc_binaries, but then chatted with cprov and we decided to apply it generally to all the fields.15:48
deryckbeuno, but it would be more accurately, "Is one of these bugs the same as:  $summary"  which is long, I think.  but I'm not picky.15:48
noodles775allenap: we've only encountered the problem on dsc_binaries, but it's the same parsing function that parses all the fields.15:48
deryckbeuno, your needed in our room at 4.15:49
* deryck has to have coke before the next meeting, sorry15:49
cprovallenap, noodles775: and debian policy states it clearly ... http://www.debian.org/doc/debian-policy/ch-controlfields.html15:49
noodles775allenap: and it's important that it's only rstrip() (imo), as there can be other single line-breaks within the fields values, just not at the end.15:49
cprovMany fields' values may span several lines;''15:49
beunoderyck, I will be there in 5ish15:50
jtvderyck: you're good to go15:51
allenapcprov, noodles775: Cool, thanks. r=me15:51
=== allenap changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: -, deryck, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com
=== jtv changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: —, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com
jtvallenap, EdwinGrubbs: sneaking out15:53
jtvI'll still be here for a while, but not looking for new reviews to do.15:53
noodles775Thanks allenap !15:55
deryckjtv, thanks!  Sorry I couldn't be more attentive to review.15:56
jtvderyck: you're sprinting, no worries.  FWIW I did check that the text gets HTML-escaped, and it does.  Just in case.  :)15:57
deryckjtv, thanks! :)15:57
EdwinGrubbsjtv: I don't understand where I go to 'pick a source package in "main" for some Ubuntu release'.16:47
jtvEdwinGrubbs: https://translations.launchpad.dev/ubuntu/hoary/+source/evolution should do it16:47
jtvfor a dev system16:48
EdwinGrubbsjtv: When I run reupload-translations.py -s hoary -p evolution16:58
EdwinGrubbsjtv: I get this: WARNING Found no translations upload for evolution in Ubuntu Hoary.16:58
jtvEdwinGrubbs: that's normal16:58
jtvit seems none of these packages have translations uploads in the sample data.  :/16:59
allenapleonardr: Is this proposal - https://code.edge.launchpad.net/~leonardr/launchpadlib/uses-restfulclient/+merge/5348 - still in need of review?17:11
leonardrallenap, no, it's long merged17:12
allenapleonardr: Okay, I'll change the status. Thanks.17:12
EdwinGrubbsjtv: what does this mean in TestReuploadScript? "INFO\s*Processing [^\s]+ in .*\n"17:13
allenapgmb: You want someone to review ajaxify-branch-linking?17:13
jtvEdwinGrubbs: a log message that looks like INFO    Processing <something>17:13
jtvEdwinGrubbs: the package names are arbitrary thanks to the factory, and I don't want to rely on the ordering even if they weren't.17:14
gmballenap: No, Tom's already reviewed that; waiting for a child branch to be ready for reviewing, but I've been sidetracked again.17:16
gmbthis is going to be one of those I-only-land-one-branch weeks, I think.17:16
allenapgmb: Okay, cool.17:17
gmballenap: Thanks for reviewing that trac-launchpad-migrator branch. I'd forgotten it even existed.17:17
allenapintellectronica: Could you mark https://code.edge.launchpad.net/~gmb/launchpad/ajaxify-branch-linking/+merge/12514 as reviewed by you, so it disappears off the "Requested reviews I can do" bit on +activereviews please? :)17:17
gmballenap: We need an nag email for merge proposals, I think...17:17
intellectronicaallenap: sure thing17:17
allenapgmb: Yeah, I noticed it when I started using launchpad-project/+activereviews. Got a bit of a shock!17:18
allenapgmb: Are you getting sidetracked by checkwatches?17:18
intellectronicamaaan, the edit button still evades me whenever i go to this page :)17:18
gmbintellectronica: You're not the only one.17:18
gmbI think it should still be next to the status myself, but maybe that should only happen when status editing is ajaxified.17:18
allenapintellectronica: Thanks!17:19
adeuringallenap: EdwinGrubbs: could one of you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-refactor-haldevice/+merge/12669 ?17:19
allenapadeuring: I don't think I'll have time today.17:21
adeuringallenap: np17:21
allenapsinzui: Are you able to land https://code.edge.launchpad.net/~sinzui/launchpad/sprint-is-physical/+merge/12026 now?17:22
allenapsinzui: I'm just doing some late late spring cleaning in +activereviews.17:22
allenapEdwinGrubbs: Are you reviewing https://code.edge.launchpad.net/~jtv/launchpad/bug-436426-2/+merge/12610?17:25
EdwinGrubbsjtv: Why are only uploads in the ROSETTA_TRANSLATIONS format re-uploaded? What other formats are there?17:27
jtvallenap: he's doing my other one17:28
EdwinGrubbsallenap: no, I'm reviewing the redo-uploads branch17:28
jtvEdwinGrubbs: the other formats are Soyuz stuff unrelated to Translations.  "Format" looks to be a bit of a misnomer IMHO17:28
EdwinGrubbsjtv: It also seems that there is no test which exercises the for-loop which calls addOrUpdateEntriesFromTarball().17:29
jtvEdwinGrubbs: it has to do with the way a single source upload spawns various work items making their own respective ways through the Soyuz work queue.17:29
allenapEdwinGrubbs, jtv: Okay, thanks. I don't actually have time to review it so I'm not sure why I was asking now!17:29
jtvEdwinGrubbs: you mean, before I exercised it in this branch?17:30
jtvallenap: that'll teach you  :-P17:30
jtvEdwinGrubbs: nm, I misread17:30
jtvEdwinGrubbs: you're talking about _processPackage, right?  That's called from main(), and is exercised in test_processPackage17:31
sinzuiallenap: The branch bounced many times. mthhadon fixed db-devel a few hours ago. (It was stuck in release-critical)17:31
allenapsinzui: Okay. I'll mark it as approved then so it drops out of the reviews list, if that's okay.17:32
sinzuioh. thanks17:33
EdwinGrubbsjtv: ok, so that test would not have output like "WARNING\s*Found no translations upload for" if you were testing the output?17:34
EdwinGrubbsjtv: so, is this script just going to be run once manually for karmic and probably never run again unless there is another bug?17:34
jtvEdwinGrubbs: right on both counts17:36
jtvwant me to inject a mock logger and test the output?17:36
jtv(actually there's easier ways to check: uploadless_packages)17:37
jtvEdwinGrubbs: oh, I already check that uploadless_packages is empty there.17:38
jtvPackages are appended to that in tandem with producing that log message.17:38
jtvBut the test needs a comment!17:38
jtvDone.17:41
EdwinGrubbsjtv: and I assume that summarize_translations_queue() ensures that the main() actually processed something, so an empty uploadless_packages couldn't be because nothing was processed?17:42
EdwinGrubbsjtv: merge-conditional17:45
jtvEdwinGrubbs: it summarizes what's on the translations queue for that package.  Since the package was just created, it was previously empty.17:46
jtvThe test verifies that _processPackages did upload files to the queue.17:46
jtvSo a pretty thorough test; not just the script's own opinion.17:46
jtvEdwinGrubbs: thanks!17:47
=== EdwinGrubbs changed the topic of #launchpad-reviews to: on call: allenap, jtv, Edwin || reviewing: noodles, —, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com
EdwinGrubbsjtv: did you want me to review your other branch when I get back from lunch? bug-436426-2?17:50
jtvEdwinGrubbs: if you have time, yes please; but not as urgent.17:50
=== matsubara is now known as matsubara-lunch
=== EdwinGrubbs is now known as Edwin-lunch
=== matsubara-lunch is now known as matsubara
=== Edwin-lunch is now known as EdwinGrubbs
=== EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com
barryEdwinGrubbs: ping21:53
=== cprov is now known as cprov-afk
=== matsubara is now known as matsubara-afk
=== Ursinha is now known as Ursinha-afk

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