=== mrevell is now known as mrevell-lunch
=== mrevell-lunch is now known as mrevell
[15:00] <barry> #startmeeting
[15:00] <MootBot> Meeting started at 09:00. The chair is barry.
[15:00] <MootBot> Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE]
[15:00] <barry> hello everybody and welcome to this week's ameu reviewer's meeting.  who's here today?
[15:00] <noodles775> me
[15:00] <henninge> me
[15:00] <bac> me
[15:00] <danilos> me
[15:01] <barry> gmb sends his apologies
[15:01] <bigjools> me
[15:01] <adeuring> me
[15:02] <flacoste> me
[15:02] <henninge> jtv has better things^W^W^W cannot be here either ...
[15:02] <henninge> ;)
[15:02] <barry> henninge: thanks :)
[15:02] <barry> flacoste, gary_poster, salgado, cprov, allenap, mars, sinzui ping
[15:02] <salgado> me
[15:02] <cprov> me
[15:02] <sinzui> me
[15:02] <flacoste> re-me
[15:02] <allenap> me
[15:02] <gary_poster> me
[15:02] <barry> flacoste: oops!
[15:03] <barry> [TOPIC] agenda
[15:03] <MootBot> New Topic:  agenda
[15:03] <flacoste> barry: that's ok, you are used to pinging me
[15:03] <barry>  * Roll call
[15:03] <barry>  * Action items
[15:03] <barry>  * UI review call update
[15:03] <barry>  * `__iter__()` in model objects? [cprov, barry]
[15:03] <barry>  * mkstemp()/open() combination leaks file-descriptors [cprov]
[15:03] <barry>  * Guidelines changes for tests checking systems pre-requisites (specific umasks, app configurations) [maxb, cprov]
[15:03] <barry>  * Invalid markup; The <script> tag requires a close tag (Do not: <script type="text/javascript" src="..."/>) [Curtis]
[15:03] <barry>  * 4-space indents for CSS styles [barry]
[15:03] <barry>  * Peanut gallery (anything not on the agenda)
[15:03] <barry>  
[15:03] <barry> flacoste: :)
[15:03] <barry> [TOPIC] action items
[15:03] <MootBot> New Topic:  action items
[15:03] <barry>  * gary_poster and barry will transfer review guidelines from the old wiki and old old wiki to the new wiki
[15:03] <barry> we suck
[15:04] <gary_poster> yup, joint suckage
[15:04] <barry> gary_poster: that sounds icky :)
[15:04] <gary_poster> heh, yeah, I thought that after I read it too
[15:04] <barry> [TOPIC]  * UI review call update
[15:04] <MootBot> New Topic:   * UI review call update
[15:05] <barry> gary_poster: let's get together later today to plan that out
[15:05] <gary_poster> +1
[15:05] <barry> i was not on the ui review call because of the us holiday.  was anybody here on that call, and was anything interesting discussed?
[15:05] <noodles775> barry: it was canceled as there was only intellectronica jtv and myself.
[15:06] <barry> noodles775: cool, thanks
[15:06] <barry> [TOPIC]  * `__iter__()` in model objects? [cprov, barry]
[15:06] <MootBot> New Topic:   * `__iter__()` in model objects? [cprov, barry]
[15:06] <barry> cprov: do you remember what this one was about?
[15:06] <intellectronica> me too
[15:07] <cprov> barry: well, it's about the how confusing it is to read code that iterate over a distroseries and get distroarchseries ;)
[15:08] <barry> cprov: so, it being more readable to do something like "for das in ds.distro_arch_series:" than "for das in ds"?
[15:09] <bigjools> are you saying __iter__ should be confined to collections?
[15:09] <cprov> bigjools: being more radical, I think we should never need __iter__ in content classes.
[15:09] <bigjools> that's what I mean :)
[15:09] <barry> bigjools: i was going to say, object's that have only one natural iteration, but i think your rule is better
[15:10] <cprov> bigjools: iterate of a multiple-join (ish) property.
[15:10] <cprov> s/of/over, sorry.
[15:11] <barry> cprov: i think +1.  you'll never be confused by naming the property explicitly.  i can only think that maybe it would be okay for IFnordSet to have an __iter__ that iterates over IFnords
[15:11] <sinzui> I agree
[15:12] <cprov> agreed
[15:12] <barry> beauty.  any objections?
[15:12] <barry> 5...4...3...2...1
[15:12] <barry> [TOPIC]  * mkstemp()/open() combination leaks file-descriptors [cprov]
[15:12] <MootBot> New Topic:   * mkstemp()/open() combination leaks file-descriptors [cprov]
[15:12] <barry> (got a few for cprov today :)
[15:13] <cprov> barry: everyone know mkstemp returns and open file descriptor, right ?
[15:13]  * barry does
[15:13] <flacoste> fire
[15:14] <cprov> barry: and if it is not bound to the file you open afterwards it won't be automatically released by python.
[15:14] <cprov> when you close the file.
[15:14] <flacoste> why use mkstemp?
[15:14] <flacoste> i usually prefer creating a temporary directory
[15:14] <flacoste> and creating files in it
[15:15] <flacoste> and removing the directory at end of test
[15:15] <flacoste> using addCleanup
[15:15] <barry> flacoste: yes, and if you need a tempfile and don't want to hassle with a directory, you should probably close the fd right after mkstemp() and reopen the file name with open()
[15:16] <cprov> flacoste: that's a good point for testing use-cases
[15:16] <cprov> flacoste: soyuz uses temp files in production workflows as well.
[15:16] <barry> <cough>with-statement</cough> :)
[15:17] <bigjools> how's that python2.6 readiness coming along then barry? :)
[15:17]  * barry looks at gary_poster and flacoste 
[15:17] <gary_poster> it's coming. :-) 2.5 first
[15:17] <wgrant> The 2.5 status is actually pretty good.
[15:18] <wgrant> Mostly benign failures.
[15:18] <bigjools> then let's JFDI
[15:18] <barry> my new machine arrives today (hopefully) and i'm going straight to karmic.  i'll be itchin' and bitchin' for 2.6 :)
[15:18] <cprov> anyway, the message I was trying to pass is: be aware of msktemp() ... open() callsites. They should be mkstemp()...os.close(fd)
[15:18] <allenap> barry: Or use os.fdopen().
[15:19] <sinzui> benign failures? Does it apologise before going belly up?
[15:19] <barry> allenap: yep; though i almost always want the filename for various purposes
[15:20] <barry> cprov: +1, thanks.  so, when you're reviewing code, watch out for mkstemp(), mkdtemp(), open() or anything else that acquires external resources.  ask your dev "are you properly releasing this resource, and if so, where?"
[15:20] <allenap> barry: I think you get that with fdopen though? It returns a file object for the fd, and the second element of the tuple returned from mkstemp() is the filename.
[15:21] <barry> allenap: yep, if you want the file object and the filename
[15:21] <barry> cprov: thanks for this topic.  i have one more for ya
[15:22] <barry> [TOPIC]  * Guidelines changes for tests checking systems pre-requisites (specific umasks, app configurations) [maxb, cprov]
[15:22] <MootBot> New Topic:   * Guidelines changes for tests checking systems pre-requisites (specific umasks, app configurations) [maxb, cprov]
[15:23] <cprov> IIRC, it was specifically about our test suite running on system with use a non-default umask (022)
[15:23] <wgrant> (and the devscripts config issue)
[15:23] <cprov> in lp.archivepublisher there as few tests that check files permission
[15:24] <barry> cprov: well, that's under user control, right?  or does our test suite set the umask?  are you aware of specific umask failures in our test suite?  i remember fixing a bunch of those when i first came on board because i use umask 0002
[15:25] <cprov> barry: I don't remember exactly which tests failed for maxb. Do you run the test suite locally frequently ?
[15:25]  * maxb waves
[15:26] <maxb> and reads scrollback
[15:26] <barry> cprov: i can't get through the whole thing on my current dev box.  we'll see about my new one (drive faster fedex!)
[15:26] <maxb> barry: umask 0002 currently breaks some (one?) soyuz test
[15:27] <barry> in general though, i would say that any test that is umask sensitive is broken
[15:27] <wgrant> There is only one.
[15:27] <barry> maxb: is there a bug on that?  imo, it's a broken test
[15:27] <maxb> The point of the action item is to suggest that the test guidelines should stipulate either that tests should set the appropriate environment, or should check and error clearly if they can't
[15:28] <cprov> barry: right, I agreed, the test should adjust umask to the expected value.
[15:28] <maxb> I don't think there's a bug at present, /me scribbles note
[15:28] <barry> maxb, cprov +1
[15:28] <sinzui> the tests or the code should set the mask? if there is a test for the mask, I believe the code should set it
[15:28] <barry> so, as you review branches, be aware of code that might be sensitive to environmental issues, and question whether the tests are fragile because of that
[15:29] <barry> sinzui: agreed
[15:30] <barry> cprov, maxb thanks for bringing this up.  anything else on this topic?
[15:31] <cprov> barry: possibly, there is a pending change in our guideline.
[15:31] <cprov> barry: we use to rely on a pristine ubuntu env for testing and deploying LP
[15:32] <cprov> barry: from what we've discussed code sensitive to environment changes should set/ensure the expected configurations now.
[15:32] <barry> cprov: but umask is really part of the user env, not ubuntu env
[15:33] <flacoste> and even relying on a prisitine ubuntu env is broken
[15:33] <cprov> barry: right, extent ubuntu env to 'default ubuntu user env'
[15:33] <maxb> Relying on a pristine environment to run the testsuite is developer-unfriendly
[15:33] <flacoste> exactly
[15:33] <barry> cprov: would you take an action item to update our guidelines with this change?
[15:33] <cprov> right, and that's the change that should be included in our guidelines.
[15:34] <cprov> barry: yes, sure.
[15:34] <barry> cprov: great, thanks.
[15:34] <barry> [ACTION] cprov to update guidelines to clarify how code sensitive to env changes should be written
[15:34] <MootBot> ACTION received:  cprov to update guidelines to clarify how code sensitive to env changes should be written
[15:35] <barry> thanks again for bringing this up
[15:35] <barry> [TOPIC]  * Invalid markup; The <script> tag requires a close tag (Do not: <script type="text/javascript" src="..."/>) [Curtis]
[15:35] <MootBot> New Topic:   * Invalid markup; The <script> tag requires a close tag (Do not: <script type="text/javascript" src="..."/>) [Curtis]
[15:35] <sinzui> The HTML 4.x spec requires that the script element have a close tag.
[15:35] <sinzui> Do not do this: <script type="text/javascript" src="..."/> because the template will not fix our mistake
[15:35] <sinzui> I saw the compact form in the DSP index and added the issue to todays agenda
[15:36] <intellectronica> can we lint for this?
[15:36] <sinzui> two hours later cprov reported that half the content of the apge was missing. I closed the tag and made it work
[15:36] <sinzui> intellectronica: We need a HTML processor
[15:36]  * sinzui is writing one
[15:36] <intellectronica> we currently use xmllint, but we miss the opportunity to automatically discover things that are html
[15:37] <sinzui> We use a bastardised version my navellint script that I wrote 7 years ago
[15:37] <noodles775> wont a standard (x)html validator pick that up? (and help us get all our pages valid?)
[15:37] <sinzui> I am now using an all python script that has a HTML validator
[15:37] <noodles775> :)
[15:38] <sinzui> noodles775: comtact is always correct for XML. this is an issue of schema/DTD validity
[15:38] <sinzui> noodles775: and this is really hard given that PT is generating  HTML, it is not HTML
[15:38] <noodles775> sinzui: yes, but I thought the w3c xhhtml validator pointed ... ah, ok.
[15:39] <noodles775> So we could validate pages as part of our stories - but that's a separate topic I guess.
[15:40] <sinzui> We know this has not happened often because Forefox hates it. The issue was fixed within a day of it landing.
[15:40] <sinzui> noodles775: That is not a story though.
[15:40] <barry> sinzui: is it just the <script> tag or other tags too?
[15:40] <sinzui> We have a checker that runs over stanging
[15:40] <noodles775> True (just convenient given we've rendered the content)
[15:41] <sinzui> <script> is the only one that comes to mind.
[15:41] <sinzui> noodles775: I do not
[15:41] <barry> cool
[15:41] <sinzui> noodles775: I render content in view tests. stories just hop from link to link and verify messages that the user sees
[15:42] <noodles775> sinzui: yes - I don't mean that you output the content in the story (urgh), just that the time taken to render the content has already happened when you call click().
[15:42] <sinzui> noodles775: another way of putting the issue is that users do not look at the source, we only care about the test that is seen and the links that are traversed
[15:43] <noodles775> Yes, I agree - it's not the place for validation.
[15:43] <intellectronica> i really don't think we should do validation in tests. that's what lint is fo
[15:43] <sinzui> noodles775: given that content/markup is conditional in most templates, that does is using luck to test
[15:44] <barry> we also often get false positives when linting our pts
[15:44] <flacoste> we could have a builder runs the test suite in validation mode
[15:44] <noodles775> intellectronica: sure, but if we wanted to do that, it would mean that lint would need render templates. It would be easier to simply run an external validator over lp.net, I think.
[15:44] <sinzui> intellectronica: I agree. We want to trust the template engine makes sane markup. that requires up to create sane templates. This is really an issue where HTML 4.x is not XML.
[15:46] <barry> folks.  we're out of time.  i don't think we'll solve this here, although let's keep sinzui's specific recommendation in mind.  we can talk more about pt validation on list
[15:46] <sinzui> There is also the issue were we do this: <p></p>; which is also invalid. a <p> must contain content
[15:46] <barry> #endmeeting
[15:46] <MootBot> Meeting finished at 09:46.
[15:46] <intellectronica> thanks barry
[15:46] <barry> thanks everyone and apologies for going over
[15:46] <deryck> thanks barry
=== henninge is now known as henninge-bbl
=== cprov is now known as cprov-lunch
=== henninge-bbl is now known as henninge
=== salgado is now known as salgado-lunch
=== salgado-lunch is now known as salgado
=== cprov-lunch is now known as cprov
=== salgado is now known as salgado-afk