Re: [Libcdio-devel] [PATCH 2/5] Add extract ISO/UDF example

[Rocky Bernstein]

On Sun, Feb 12, 2012 at 7:36 PM, Pete Batard <address@hidden> wrote:

> On 2012.02.12 14:53, Rocky Bernstein wrote:
>
>> I get a failure running the check_udf.sh created by this patch:
>>
>
> Well, there are some UDF patches missing from -pbatard, so the test is not
> supposed to work at this stage. It is added so that we can check that UDF
> extraction works _when_ it is actually meant to work.
>

There is a misunderstanding or difference here between the function of the
tests and how we use the tests.

Although I said in test-driven development, advocates suggest writing tests
first, so tests start out broken, by the time one commits code, the tests
should work.

How was I or anyone else supposed to know that this was supposed to fail?
You said something about tests failing because of symbolic links.  But this
failing tests has nothing to do with that. So I spent time with gdb which I
probably didn't have to do and might have been spent instead looking at and
trying other things.

When someone wants me to review patches for integration, one of my first
lines of defense is whether things are now broken. The process I use for
installing software is the check out or update the code, build it, *test*
it, perhaps try it uninstalled and finally install it. If the testing
doesn't work, there is a serious consideration on my part as to whether to
go forward.

Also, I want to encourage people at any time to check out what is in git
and use and give feedback if something is wrong. It is not uncommon in my
projects for various packagers to release code to the general public based
on a point in time that the packager decided to check out the code in git.
But if we have code in there that isn't passing tests, how are *they*
supposed to know tests are supposed to fail?

The proper way to mark a test as "not quite done" is to mark it skippable.
In this particular project which uses the built-in autotools mechansism,
the test should exit with with return code 77. Additionally it would be
nice to print a message to stderr giving a reason why the test is skipped.
For example: not finished yet, or not applicable for this OS or
architecture.



>
> My plan is to add the tests so that once we fix things, we can get some
> validation.


Again backwards from the way I do development. The tests *are* my
first-line of validation. And especially when, as is going on here, there
are massive changes.  The tests are what gives me confidence that things
haven't gotten worse. Getting user-acceptance is absolutely important, but
I have found that fewer people are willing to try code in projects when the
tests (regularly) fail.

And just because there are branches doesn't necessarily mean it's okay for
tests to fail in the branches. Recently in libcdio-help someone mentioned
failing compilation on MinGW. I suggested trying the pbatard branch. Things
are probably stalled here because the code doesn't compile for him. See
http://lists.gnu.org/archive/html/libcdio-help/2012-02/threads.html  . But
I imagine if it had compiled, checking that tests pass in the pbatard
branch would have next been tried.

But that doesn't mean that the test should PASS or even work right ahead.
> As previously indicated, I have quite a few fixes for the tests planned,
> that aren't in pbatard-integration yet, because I would really like us to
> first go through these 5 patches before processing some more.
>
>
>  The test works  in the pbatard branch; one difference between the two are
>> the unions added to inlcude/cdio/ecma_167.h and corresponding code
>> adjustments.
>>
>
> Seems logical. Again, this is the beginning of integration and there is
> quite some way to go to get everything working...
>
> Remember, that this first salvo of patches was meant to stop short of
> fixing the tests and doesn't include any of the necessary changes for core.
>
>
>  Not strictly related. I have made some changes to extract.c to report
>> filenames more often on error.
>>
>
> OK.
>
>
>  And that reminds me, looking at extract.c I see that creates and writes
>> are
>> preformed using the full path. That's fine, but in those cases where there
>> are lots of files to extract and the path is very long, it used to be the
>> case that you can speed things up (at least on Unix) by doing a CD into
>> the
>> directory and then doing relative (not absolute) opens and writes. I have
>> no idea if it will make a difference here, but if speed is a concern, this
>> is one thing to consider.
>>
>
> Thanks for the pointer. Doesn't seem to be necessary right now, but I'll
> keep that in mind.


Ok. Since this might be of interest,  let me go into why "chdir'ing" is
faster on GNU/Linux.  I suspect this is true of MS Windows as well. When
one does a mkdir() or fopen() of a path, the OS has to break the string
into path components, and stat() each directory to see if the directory
exists and that you have permission to read, write, or descend into that
directory. Although largely unnecessary in the pattern of usage for
extract.c, breaking a string into path components isn't the time consuming
part -- even when there are lots of directories as I would expect there to
be for OS distributions.  But the stat() can involve I/O unless this
information is cached in disk buffers.  And if there is caching, one has
increased active page consumption.

In the case of extract.c, probably everything is read/write bound. However
if one were writing a listing program like "tar -t", "ls -r", or "du" then
the difference becomes more pronounced.


>
>  Another change to pbatard-integration was to correct the failing
>> check-cue.sh test. This change I think was already in pbatard, although it
>> wasn't in master which it should have been. So master has been changed
>> too.
>>
>
> I was planning to poduce a fix for check-cue.sh.in patch in the next
> salvo!
>
> If there's anything already in pbatard that I haven't pushed into
> integration but that you want to see integrated, I'd really appreciate if
> you let me know, as I need to adjust my plans then...
>

> This patch was not pushed because it pertains to the fixing of the tests,
> which is not the goal of the current 5 preliminary patches.
>
> If you don't want to do it like this, that's fine, but please let me know
> so that I can produce patches in an order that we can all agree on.
>
> Right now, I am still not planning to push anything more until the 5
> patches I have proposed have been reviewed and integrated (or redone if
> required), and this is regardless of whether some tests are failing or not.
> If this is not what you have in mind, then we need to review what the order
> should be...
>

>
>  And finally in pbatard and pbatard-integration cdio_config.h how has the
>> CDIO_ prefixes in #defines.
>>
>
> OK. I only see it in pbatard right now which is fine, as I'd rather you
> don't touch pbatard-integration actually (but you can push as many changes
> as you want to pbatard)
>

Ok. Got it.


>
> Whatever is in pbatard will end up into pbatard-integration one way or
> another, and subsequently as an official patch proposal for
> mainline/master. It may not be in the order you would prefer, but it will
> happen.
> Also be mindful that I may very well revert commits from
> pbatard-integration on a whim if needed (or delete commit proposals and
> pick the ones from mainline once integrated) => the less you push there the
> better. In fact, I'd appreciate if you don't push anything in
> pbatard-integration without first letting me know. Better consider it a
> personal workdir that also happens to be public...
>

Ok.


>
> Now, if there are any modifications you would like to see on the 5
> proposed patches, let me know.


Ok. Now that I understand where things stand, I'm okay with looking at with
what is there in pbatard-integration. Give me some more time to look it
over and compile on various platforms. I'll try to get to this week as I'm
going out of town next weekend.


Otherwise, I'll wait for you to integrate them before starting producing
> fixes for the tests.
>

Thanks.


>
> Regards,
>
> /Pete
>
>