Erik de Castro Lopo wrote:>> I've just grabbed a copy of SVN head for libogg and 'make check' >> is failing: >The breakage happened in this commit: > > r17098 | gmaxwell | 2010-03-29 16:35:11 +1100 (Mon, 29 Mar 2010) | 1 line > Firstly, is gmaxwell on this list?[snip]> Secondly should be be trying controvertial changes in HEAD? Surely thats > what separate branches are for, so that the trunk never breaks like this.Nothing is broken. The libogg test-cases are especially particular checks on exact (non-normative) aspects of the library behaviour. This is fine for their intended use: catching compiler/porting bugs on target systems, but it makes them less useful for development regression testing. In this case, I didn't update the test case while making the change. Xiphmont left the tree in exactly the same state after making the change to pack 4 frames, this was fixed in r17039. As far as where changes are made? we have _releases_ for the purpose of providing stable code, though because the norm around here is to have a mostly working trunk I never would have committed a change which could reasonably have been expect to cause any real breakage. As far as controversial goes, I probably should have put it in scare quotes. The kind of controversial I'm talking about is the name of the function I added (I don't like _fill() much myself, but nothing better seemed obvious) or the decision to add a somewhat kludgey additional input function rather than break the ABI and add a one shot control. If the new function stays it will likely need a test case. I'm not going to write one until the decision is final that we'll go that route. Cheers
Gregory Maxwell wrote:> Nothing is broken.The test suite failed. AFAIAC, that means that what is in HEAD is not currently fit for release. If for instance there was new CVE released against libogg, how do you do a new release that fixes that CVE without fixing the other issues as well? Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/
Erik de Castro Lopo wrote:> Gregory Maxwell wrote: > >> Nothing is broken. > > The test suite failed. AFAIAC, that means that what is in HEAD is not > currently fit for release.The test suite should already be fixed, see r17124. Thanks for reporting the problem.> If for instance there was new CVE released against libogg, how do > you do a new release that fixes that CVE without fixing the other > issues as well?The amount of code in libogg is so small and changes so infrequent that it would be trivial to backport almost any conceivable change to the prior release tag (which was just a few days ago) and work from there, if that was necessary. In fact, in Monty's work with Red Hat, they have usually _required_ him to backport security fixes to all prior releases and make new point releases with them, because they are still shipping old versions in many of their stable products. In this case, fixing the test suite was also relatively easy. No need to panic.
On Mon, Apr 5, 2010 at 4:44 AM, Erik de Castro Lopo <mle+la at mega-nerd.com> wrote:> Gregory Maxwell wrote: > >> Nothing is broken. > > The test suite failed. AFAIAC, that means that what is in HEAD is not > currently fit for release. > > If for instance there was new CVE released against libogg, how do > you do a new release that fixes that CVE without fixing the other > issues as well?By patching against the tagged release. Monty
So if I understand the libogg development practises correctly.>> (r17098 | gmaxwell | 2010-03-29 16:35:11 +1100 (Mon, 29 Mar 2010))Gregory Maxwell checked in a change ...>> I've just grabbed a copy of SVN head for libogg and 'make check' >> is failing:... which broke the test suite ...> If the new function stays it will likely need a test case. I'm not > going to write one until the decision is final that we'll go that > route.... without testing the change (to demonstrate it works, or is useful, or ... anything) ...> As far as controversial goes, I probably should have put it in scare > quotes. The kind of controversial I'm talking about is the name of the > function I added (I don't like _fill() much myself, but nothing better > seemed obvious) or the decision to add a somewhat kludgey additional > input function rather than break the ABI and add a one shot control.... with misleading check in comments. However it is rather moot since ...> Nothing is broken. The libogg test-cases are especially particular > checks on exact (non-normative) aspects of the library behaviour. This > is fine for their intended use: catching compiler/porting bugs on > target systems, but it makes them less useful for development > regression testing.... the test suite isn't particularly useful and ...> In this case, I didn't update the test case while making the change. > > Xiphmont left the tree in exactly the same state after making the > change to pack 4 frames, this was fixed in r17039. As far as where > changes are made? we have _releases_ for the purpose of providing > stable code, though because the norm around here is to have a mostly > working trunk I never would have committed a change which could > reasonably have been expect to cause any real breakage.... it is normal to break the test suite and patch it later. I know some places insist on updating the test suite with altered and new test when (or before) committing new functionality. For libogg, the approach is to commit to HEAD first and possibly test it later? When it comes to new functionality, rather than discussing it on the list, one commits the code first and discusses later if anyone comments? This definitely accelerates adding new changes. Neil Leathers
On Mon, Apr 5, 2010 at 10:15 AM, Neil Leathers <neil.leathers at rogers.com> wrote:>> If the new function stays it will likely need a test case. I'm not >> going to write one until the decision is final that we'll go that >> route. > > ... without testing the change (to demonstrate it works, or is useful, or ... anything) ...Of course I tested the change. Moreover, the change isn't of a nature which could cause failure beyond the minimum extent _any_ change can be expected to cause failure.> ... with misleading check in comments.Come on now.> ... the test suite isn't particularly useful and ...It is useful for its intended purpose: It checks the that libogg behaves exactly the way the check expected to behave. For example, it hands libogg a sequence of packets and validates that the output is exactly as expected, down to the checksums. Any change in the default pagination will cause test failures until the test is updated to reflect the new behaviour.> ... it is normal to break the test suite and patch it later. > > I know some places insist on updating the test suite with altered and new test when (or before) committing new functionality. For libogg, the approach is to commit to HEAD first and possibly test it later? When it comes to new functionality, rather than discussing it on the list, one commits the code first and discusses later if anyone comments? This definitely accelerates adding new changes.No, I'd been discussing it on IRC, along with the justification and such. Checking the commit log, discussions on the IRC channels are inclusive of every developer to commit to libogg since 2006 (2004, save one person).
On Mon, Apr 5, 2010 at 10:15 AM, Neil Leathers <neil.leathers at rogers.com> wrote:> So if I understand the libogg development practises correctly. > >>> (r17098 | gmaxwell | 2010-03-29 16:35:11 +1100 (Mon, 29 Mar 2010)) > > Gregory Maxwell checked in a change ... > >>> I've just grabbed a copy of SVN head for libogg and 'make check' >>> is failing: > > ... which broke the test suite ... > >> If the new function stays it will likely need a test case. I'm not >> going to write one until the decision is final that we'll go that >> route. > > ... without testing the change (to demonstrate it works, or is useful, or ... anything) ...It was discussed in IRC and checked into SVN to give the possibility of wider evaluation. Greg also cleared the change with me personally before checking it in.> >> As far as controversial goes, I probably should have put it in scare >> quotes. The kind of controversial I'm talking about is the name of the >> function I added (I don't like _fill() much myself, but nothing better >> seemed obvious) ?or the decision to add a somewhat kludgey additional >> input function rather than break the ABI and add a one shot control. > > ... with misleading check in comments."Add a 'ogg_stream_pageout_fill' function to allow smart applications with delay sensitive flushing to produce big pages. Increase the default minimum fill amoun t to 8 based on latency measurements with actual files. These changes may be con troversial but since we've recently had a release I thought there would be no ha rm in getting them into the repository for discussion." That's pretty clear and accurate.> >> In this case, I didn't update the test case while making the change. >> >> Xiphmont left the tree in exactly the same state after making the >> change to pack 4 frames, this was fixed in r17039. ?As far as where >> changes are made? we have _releases_ for the purpose of providing >> stable code, though because the norm around here is to have a mostly >> working trunk I never would have committed a change which could >> reasonably have been expect to cause any real breakage. > > ... it is normal to break the test suite and patch it later.It is normal to put probationary changes for wider testing on HEAD between releases. It is not normal to put changes on HEAD that are known to be broken. Monty