Bazaar: Export directly from the remote branch.

[jelmer]

Bazaar: Export directly from the remote branch.

This significantly improves performance, since it allows the remote
server to directly stream a tarball that just contains the requested
revision rather than the full repository contents.

[jelmer]

@cjerdonek This is the "bzr export" performance improvement split out from #5445 as discussed in that pull request.

[jelmer]

The failures on the other platforms are because "bzr" is not available there.

[pradyunsg]

The failures on the other platforms are because "bzr" is not available there.

You should decorate the method with need_bzr from test.lib

[cjerdonek]

The failures on the other platforms are because "bzr" is not available there.

@jelmer Can you confirm if the test you added is running and passing anywhere in CI? When the test was failing, I saw it failing on Windows, macOS, and Linux, which is all of the platforms. It is probably worth exploring how easy it would be to install bzr in at least, say, the Linux testing environment.

[pradyunsg]

Can you confirm if the test you added is running and passing anywhere in CI?

IIRC, None of our CI have bzr. 🔥

[jelmer]

On Sun, Jan 20, 2019 at 05:58:53AM -0800, Pradyun Gedam wrote: > Can you confirm if the test you added is running and passing anywhere in CI? IIRC, None of our CI have `bzr`. 🔥

I've updated the travis config to install bzr.

…

-- Jelmer Vernooij <[email protected]> PGP Key: https://www.jelmer.uk/D729A457.asc

[cjerdonek]

@pradyunsg Is there or do we have a standard way (e.g. environment variable) to tell whether the tests are being run under Travis?

I think there should be a test of the form "if Travis is running, than is_bzr_installed() is true" (where is_bzr_installed() is computed using the same logic / code path as need_bzr()). This will ensure that the Bazaar tests are being run in at least one setting.

[pradyunsg]

https://docs.travis-ci.com/user/environment-variables/#default-environment-variables

Yes. See also #6059 which had to detect being run on Travis CI to skip hg tests (since hg was broken on Travis).

---

I suggest having a separate PR for installing (and a test for ensuring) bzr is installed on Travis.

[jelmer]

Could that be deferred to later? There have been several changes to the bazaar.py file over the last year (when I first proposed this change). This change is at least adding a first test for the Bazaar export support that runs on travis.

[cjerdonek]

I think it's important for the tests being added to actually run in CI (and for us to be able to know they're being run) before the code is changed in a substantive way. The changes before were refactors that preserved the logic and didn't e.g. change any command invocations. The new test I'm talking about to check that it's running would just be a few lines using existing code. I would be okay with it being in the same or a separate PR given how small it is.

[jelmer]

Trivial change added to verify that bzr is available when running under travis. Please let me know if this is the best place (tests/lib/init.py houses need_bzr, so test_lib.py seemed like the best choice).

[cjerdonek]

Here is what I would recommend doing:

1. Add is_bzr_installed() just before need_bzr() since the code is almost identical. (These two functions should really be calling the same underlying function, or need_bzr() should be calling is_bzr_installed(). But you don't have to DRY this up now.)

2. At the beginning of test_vcs_bazaar.py, add test_ensure_bzr_available() and decorate it with @pytest.mark.skipif(<travis not running>). The reason not to call this test test_is_bzr_installed() is that it's not testing that is_bzr_installed() was written correctly but rather testing that Bazaar is in fact installed.

[jelmer]

On Tue, Jan 22, 2019 at 12:29:11AM -0800, Chris Jerdonek wrote: Here is what I would recommend doing: 1. Add `is_bzr_installed()` just before `need_bzr()` since the code is almost identical. (These two functions should really be calling the same underlying function, or `need_bzr()` should be calling `is_bzr_installed()`. But you don't have to do this now.) 2. At the beginning of `test_vcs_bazaar.py`, add `test_is_bzr_installed()` and decorate it with ***@***.***(<travis not running>)`](https://docs.pytest.org/en/latest/skipping.html#id1).

Thanks! I've made the changes you suggested.

…

-- Jelmer Vernooij <[email protected]> PGP Key: https://www.jelmer.uk/D729A457.asc

[cjerdonek]

Thanks, @jelmer. But read (2) a little more closely. I was saying to add the Travis test to the beginning of test_vcs_bazaar.py, and to give that test a different name.

[jelmer]

Done.

[cjerdonek]

Looking a lot better -- thanks! Some tiny comments, and a bigger comment.

[jelmer]

@cjerdonek all done

[cjerdonek]

Okay, thanks. I'll take another look after the release issues settle down.

[cjerdonek]

Thanks, @jelmer, for your work on this and for your patience!

[jelmer]

Thanks for the reviews and merging, @cjerdonek !

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.