Merge lp:~vila/bzr/822649-bsd-no-fdatasync into lp:bzr

[Vincent Ladeuil]

os.fdatasync is not defined on BSD-based OSes

Don't try to run the test if os.fdatasync is not defined.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/8/2011 2:52 PM, Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging
> lp:~vila/bzr/822649-bsd-no-fdatasync into lp:bzr.
>
> Requested reviews: bzr-core (bzr-core) Related bugs: Bug #822649 in
> Bazaar:
> "bzrlib.tests.test_transport.TestLocalTransportWriteStream.test_local_fdatasync_calls_fdatasync
> fails on BSDs" https://bugs.launchpad.net/bzr/+bug/822649
>
> For more details, see:
> https://code.launchpad.net/~vila/bzr/822649-bsd-no-fdatasync/+merge/70733
>
> Don't try to run the test if os.fdatasync is not defined.

I'm wondering if it would be better to just teach 'overrideAttr' to not
fail when the target attribute doesn't exist. Otherwise:

 merge: approve

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk4/4HUACgkQJdeBCYSNAAPeygCcDUXlrm5BK5t1UW5Yih56Baf6
gswAn3ip+MnVQY8SIFVny+04v/EngA5A
=CGJq
-----END PGP SIGNATURE-----

review: Approve

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/8/2011 2:52 PM, Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging
> lp:~vila/bzr/822649-bsd-no-fdatasync into lp:bzr.
>
> Requested reviews: bzr-core (bzr-core) Related bugs: Bug #822649 in
> Bazaar:
> "bzrlib.tests.test_transport.TestLocalTransportWriteStream.test_local_fdatasync_calls_fdatasync
> fails on BSDs" https://bugs.launchpad.net/bzr/+bug/822649
>
> For more details, see:
> https://code.launchpad.net/~vila/bzr/822649-bsd-no-fdatasync/+merge/70733
>
> Don't try to run the test if os.fdatasync is not defined.

I should also mention that the test could proceed and make sure the
system doesn't crash if fdatasync doesn't exist. (Though that could be a
separate test that forcibly removes fdatasync always.)

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk4/4KkACgkQJdeBCYSNAAO9sQCfZpPZU6m73z/M+UBw4ecIYUJb
BQ4An0rFmOsHnygZmOMC4u1zEUV6e8it
=1BBM
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> John A Meinel <email address hidden> writes:

    > On 8/8/2011 2:52 PM, Vincent Ladeuil wrote:
    >> Vincent Ladeuil has proposed merging
    >> lp:~vila/bzr/822649-bsd-no-fdatasync into lp:bzr.
    >>
    >> Requested reviews: bzr-core (bzr-core) Related bugs: Bug #822649 in
    >> Bazaar:
    >> "bzrlib.tests.test_transport.TestLocalTransportWriteStream.test_local_fdatasync_calls_fdatasync
    >> fails on BSDs" https://bugs.launchpad.net/bzr/+bug/822649
    >>
    >> For more details, see:
    >> https://code.launchpad.net/~vila/bzr/822649-bsd-no-fdatasync/+merge/70733
    >>
    >> Don't try to run the test if os.fdatasync is not defined.

    > I should also mention that the test could proceed and make sure the
    > system doesn't crash if fdatasync doesn't exist. (Though that could be a
    > separate test that forcibly removes fdatasync always.)

AIUI, the purpose of this test is to ensure fdatasync is called. I trust
Martin have other tests to ensure the coverage.

[Vincent Ladeuil (vila)]

>>>>> John A Meinel <email address hidden> writes:

    > Review: Approve
    > On 8/8/2011 2:52 PM, Vincent Ladeuil wrote:
    >> Vincent Ladeuil has proposed merging
    >> lp:~vila/bzr/822649-bsd-no-fdatasync into lp:bzr.
    >>
    >> Requested reviews: bzr-core (bzr-core) Related bugs: Bug #822649 in
    >> Bazaar:
    >> "bzrlib.tests.test_transport.TestLocalTransportWriteStream.test_local_fdatasync_calls_fdatasync
    >> fails on BSDs" https://bugs.launchpad.net/bzr/+bug/822649
    >>
    >> For more details, see:
    >> https://code.launchpad.net/~vila/bzr/822649-bsd-no-fdatasync/+merge/70733
    >>
    >> Don't try to run the test if os.fdatasync is not defined.

    > I'm wondering if it would be better to just teach 'overrideAttr' to not
    > fail when the target attribute doesn't exist.

For other cases, calling *override*Attr for an attribute that doesn't
exist and getting yelled at... sounds appropriate :)

Not failing in this specific case would have masked the issue.

Thanks for the prompt review.

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/8/2011 4:26 PM, Vincent Ladeuil wrote:
>>>>>> John A Meinel <email address hidden> writes:
>
>> Review: Approve On 8/8/2011 2:52 PM, Vincent Ladeuil wrote:
>>> Vincent Ladeuil has proposed merging
>>> lp:~vila/bzr/822649-bsd-no-fdatasync into lp:bzr.
>>>
>>> Requested reviews: bzr-core (bzr-core) Related bugs: Bug #822649
>>> in Bazaar:
>>> "bzrlib.tests.test_transport.TestLocalTransportWriteStream.test_local_fdatasync_calls_fdatasync
>
>>>
>>>
>> fails on BSDs" https://bugs.launchpad.net/bzr/+bug/822649
>>>
>>> For more details, see:
>>> https://code.launchpad.net/~vila/bzr/822649-bsd-no-fdatasync/+merge/70733
>
>>>
>>>
>>
>>> Don't try to run the test if os.fdatasync is not defined.
>
>> I'm wondering if it would be better to just teach 'overrideAttr'
>> to not fail when the target attribute doesn't exist.
>
> For other cases, calling *override*Attr for an attribute that doesn't
> exist and getting yelled at... sounds appropriate :)
>
> Not failing in this specific case would have masked the issue.

I don't see what issue it is masking. We have a test suite that is not
testing that 'fdatasync' is being called when it is available on some
platforms. Often we've tried to make sure the test suite runs on all
platforms in such a way as to mimic the other platforms. For example, we
would *like* it to fail in PQM if it would fail on Windows.

Similarly, I think we would like it to fail on Windows if it would fail
in PQM. Say I change something while on Windows, and it no longer calls
fdatasync. The test suite will pass for me, only to be rejected by PQM.

So I disagree that the "better" fix is to skip the test. I still think
the "better" fix is to fake an fdatasync call, and test that it is being
called when it is available.

>
> Thanks for the prompt review.
>

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk4//XoACgkQJdeBCYSNAAOIuQCePcYvZeqFKSakSP5/04BRUphi
1voAn2ael1AZI0V0sjRYnz9oP4gMB89v
=/02a
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...

>>> Don't try to run the test if os.fdatasync is not defined.
>
>> I should also mention that the test could proceed and make sure
>> the system doesn't crash if fdatasync doesn't exist. (Though that
>> could be a separate test that forcibly removes fdatasync always.)
>
> AIUI, the purpose of this test is to ensure fdatasync is called. I
> trust Martin have other tests to ensure the coverage.
>

That is, indeed, the purpose of this test. However, there is no other
direct coverage. There isn't a test that calling "w.fdatasync()" does
nothing when fdatasync is not available.

There is a bit of side-effect coverage because the rest of the test
suite doesn't fail (because in fdatasync is likely to be called in parts
of the codebase).

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk4//i8ACgkQJdeBCYSNAAPzaQCgulAjR49c5yCpXwrBnd9qaDqE
5CMAoJFzKxem+YY+GB9KDvNiG2I+1EtK
=qG4c
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> John A Meinel <email address hidden> writes:

<snip/>

    > I don't see what issue it is masking.

That, contrary to the dev belief, fdatasync is not available on BSDish
systems.

    > We have a test suite that is not testing that 'fdatasync' is being
    > called when it is available on some platforms.

Huh ? The test ensures that fdatasync is called when it is available.

<snip/>

    > So I disagree that the "better" fix is to skip the test.
    > I still think the "better" fix is to fake an fdatasync call,

Why would I fake something the python devs didn't implement ?

    > and test that it is being called when it is available.

That's what the test does. Please read it.

[Vincent Ladeuil (vila)]

>>>>> John A Meinel <email address hidden> writes:

    > That is, indeed, the purpose of this test. However, there is no other
    > direct coverage. There isn't a test that calling "w.fdatasync()" does
    > nothing when fdatasync is not available.

    > There is a bit of side-effect coverage because the rest of the test
    > suite doesn't fail (because in fdatasync is likely to be called in parts
    > of the codebase).

Right, so the fix is not about fixing the coverage, it's about fixing
the test failure.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/8/2011 6:18 PM, Vincent Ladeuil wrote:
>>>>>> John A Meinel <email address hidden> writes:
>
> <snip/>
>
>> I don't see what issue it is masking.
>
> That, contrary to the dev belief, fdatasync is not available on
> BSDish systems.
>
>> We have a test suite that is not testing that 'fdatasync' is being
>> called when it is available on some platforms.
>
> Huh ? The test ensures that fdatasync is called when it is
> available.
>
> <snip/>
>
>> So I disagree that the "better" fix is to skip the test. I still
>> think the "better" fix is to fake an fdatasync call,
>
> Why would I fake something the python devs didn't implement ?
>
>> and test that it is being called when it is available.
>
> That's what the test does. Please read it.
>

Which *isn't* being run on some platforms. The point is to have the test
suite be as platform agnostic as possible, so that I can run it on
Windows/Linux and not worry that it will fail on a different platform
that I'm *not* running it on.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5A7BkACgkQJdeBCYSNAAOlAQCgxK+CA1P92MZ78Y7Js4yuk2a3
p6IAoKTHV39F2ZIpWkWtu/AJU3lZWSj4
=jWfU
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> John Arbash Meinel <email address hidden> writes:

<snip/>

    > Which *isn't* being run on some platforms. The point is to have the test
    > suite be as platform agnostic as possible, so that I can run it on
    > Windows/Linux and not worry that it will fail on a different platform
    > that I'm *not* running it on.

I don't understand. You seem to be advocating for what the patch is
doing and at the same time disagree with what it does >-/

This test want to check that fdatasync is called. As such it doesn't
make sense to run it on platforms where fdatasync is not implemented.

Surely you're not advocating for providing a fdatasync implementation
good enough to satisfy the test ?

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/9/2011 10:46 AM, Vincent Ladeuil wrote:
>>>>>> John Arbash Meinel <email address hidden> writes:
>
> <snip/>
>
>> Which *isn't* being run on some platforms. The point is to have the
>> test suite be as platform agnostic as possible, so that I can run
>> it on Windows/Linux and not worry that it will fail on a different
>> platform that I'm *not* running it on.
>
> I don't understand. You seem to be advocating for what the patch is
> doing and at the same time disagree with what it does >-/
>
> This test want to check that fdatasync is called. As such it doesn't
> make sense to run it on platforms where fdatasync is not
> implemented.

It makes sense that even on Windows/BSD, if we fake a fdatasync being
available, we can test that it gets called.

>
> Surely you're not advocating for providing a fdatasync
> implementation good enough to satisfy the test ?
>

We aren't testing that the data gets synced. We're testing that if a
function is available, it gets called. It is easy to be hacking on a
platform where the function isn't available, and accidentally break the
code without knowing it.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5A/VYACgkQJdeBCYSNAANn1ACeMmZBtmbVnwg3YUaMSgMFiD/H
pUYAn3/ohFBTUKVc95nTAME90HcIHv49
=EpIX
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> John A Meinel <email address hidden> writes:

<snip/>

    >>
    >> This test want to check that fdatasync is called. As such it doesn't
    >> make sense to run it on platforms where fdatasync is not
    >> implemented.

    > It makes sense that even on Windows/BSD, if we fake a fdatasync being
    > available, we can test that it gets called.

See below as why I think it's a bad idea to fake it.

    >>
    >> Surely you're not advocating for providing a fdatasync
    >> implementation good enough to satisfy the test ?
    >>

    > We aren't testing that the data gets synced. We're testing that if a
    > function is available, it gets called.

Yes, that's what the test is doing, *iff* fdatasync is available.

    > It is easy to be hacking on a platform where the function isn't
    > available, and accidentally break the code without knowing it.

s/break the code/break the *test*/ which is what happened here and what
the fix avoids.

If we start adding a fake implementation, the test will probably need to
be rewritten to do *less* assumptions about the fdatasync
implementation.

You can argue that the test is too eager right now and so on, but from
my POV that's out-of-scope for this fix.

=== modified file 'bzrlib/tests/test_transport.py'
--- bzrlib/tests/test_transport.py	2011-08-02 01:10:27 +0000
+++ bzrlib/tests/test_transport.py	2011-08-08 12:54:08 +0000
@@ -748,6 +748,10 @@
         We can't easily observe the external effect but we can at least see
         it's called.
         """
+        sentinel = object()
+        fdatasync = getattr(os, 'fdatasync', sentinel)
+        if fdatasync is sentinel:
+            raise tests.TestNotApplicable('fdatasync not supported')
         t = self.get_transport('.')
         calls = self.recordCalls(os, 'fdatasync')
         w = t.open_write_stream('out')

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-08-06 03:52:34 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-08-08 12:54:08 +0000
@@ -156,6 +156,9 @@
   due to the order that `build_snapshot` performs its actions.
   (Andrew Bennetts)
 
+* Don't require ``os.fdatasync`` to be defined on all supported OSes
+  (BSD-based OSes don't define it).  (Vincent Ladeuil, #822649)
+
 * Fix compatibility with testtools 0.9.12. (Jelmer Vernooij, #815423)
 
 * `TestCaseWithMemoryTransport` is faster now: `_check_safety_net` now