Merge lp:~xaav/loggerhead/export-tarball into lp:loggerhead

Proposed by xaav
Status: Merged
Merged at revision: 461
Proposed branch: lp:~xaav/loggerhead/export-tarball
Merge into: lp:loggerhead
Diff against target: 359 lines (+131/-14)
12 files modified
.bzrignore (+2/-0)
loggerhead/apps/branch.py (+4/-2)
loggerhead/config.py (+3/-0)
loggerhead/controllers/__init__.py (+1/-1)
loggerhead/controllers/download_ui.py (+39/-8)
loggerhead/controllers/revision_ui.py (+2/-0)
loggerhead/exporter.py (+46/-0)
loggerhead/history.py (+5/-1)
loggerhead/templates/menu.pt (+4/-0)
loggerhead/templates/revision.pt (+4/-1)
loggerhead/tests/test_controllers.py (+20/-0)
loggerhead/tests/test_simple.py (+1/-1)
To merge this branch: bzr merge lp:~xaav/loggerhead/export-tarball
Reviewer Review Type Date Requested Status
Gavin Panella (community) Abstain
John A Meinel Needs Information
Ian Booth (community) code* Approve
Martin Pool Approve
Robert Collins Pending
Vincent Ladeuil Pending
Martin Albisetti Pending
Review via email: mp+66408@code.launchpad.net

This proposal supersedes a proposal from 2011-06-08.

Description of the change

This branch **may** accomplish exporting the tarball using chunked transfer encoding. The code all looks to be correct, but I have not tested it, so I would like your opinion.

Thanks!

To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

Thanks very much, that'd be a really useful feature to have. Thanks
also for making it optional, because probably some installations would
not want it on.

This looks broadly reasonable -- I'm not deeply familiar with
loggerhead -- but I am very curious why you apparently reimplemented
the export-to-tarball feature. I'd rather reuse the bzr code and if
necessary change it to let it be reused here.

When you say "not tested" do you mean you haven't even run it, or only
that you didn't add automatic tests?

Revision history for this message
Robert Collins (lifeless) wrote : Posted in a previous version of this proposal

This looks very interesting.

Three isues:

1)
+class TarExporterFileObject(object):
+
+ def __init__(self):
+ self._buffer = ''
+
+ def write(self, str):
+ self._buffer += str
+
+ def get_buffer(self):
+ buffer = self._buffer
+ self._buffer = ''
+ return buffer

This is going to be somewhat inefficient. Try this:

+class TarExporterFileObject(object):
+
+ def __init__(self):
+ self._buffer = []
+
+ def write(self, str):
+ self._buffer.append(str)
+
+ def get_buffer(self):
+ try:
+ return ''.join(self._buffer)
+ finally:
+ self._buffer = []

2) There are no tests for this. the test suite is still pretty new, but its a good idea to test things - in particular in cases like this we need to be fairly confident it will be incremental and not block on the export - I can imagine the wsgi layer buffering everything, for instance. [in fact, I'll lay odds it will unless we fix a few things].

3) The export function is a copy-paste-tweak of the core from bzrlib. This will lead to bugs as that code base evolves - we should instead get a supported function in bzrlib lib that we can import and use.

I'm putting this back to WIP - but its a great start. Please keep at it and just shout if you need pointers.

review: Needs Fixing
Revision history for this message
xaav (xaav) wrote : Posted in a previous version of this proposal

> 3) The export function is a copy-paste-tweak of the core from bzrlib. This will lead to bugs as
> that code base evolves - we should instead get a supported function in bzrlib lib that we can
> import and use.

Well, there is only one problem with that. According to the WSGI spec, you must return an iterable that will export the blocks. If I were to call the provided function, it would be impossible to break the response into pieces because the provided function would export it all at once. I know that it is a copy and paste tweak, but there is really no way I can inject the 'yield' keyword into the provided function. If you have another suggestion, I would be glad to hear it.

Issue number one I will be glad to fix.

Regarding issue number two, I have not written tests before but I will try my best.

Revision history for this message
Robert Collins (lifeless) wrote : Posted in a previous version of this proposal

On Wed, Jun 1, 2011 at 9:46 AM, Geoff <email address hidden> wrote:
>> 3) The export function is a copy-paste-tweak of the core from bzrlib. This will lead to bugs as
>> that code base evolves - we should instead get a supported function in bzrlib lib that we can
>> import and use.
>
> Well, there is only one problem with that. According to the WSGI spec, you must return an iterable that will export the blocks. If I were to call the provided function, it would be impossible to break the response into pieces because the provided function would export it all at once. I know that it is a copy and paste tweak, but there is really no way I can inject the 'yield' keyword into the provided function. If you have another suggestion, I would be glad to hear it.

Extract the function in bzrlib into two parts - a generator (what you
have here) and a consumer than consumes it all triggering the writes.

Then we can reuse the generator.

Revision history for this message
xaav (xaav) wrote : Posted in a previous version of this proposal

> On Wed, Jun 1, 2011 at 9:46 AM, Geoff <email address hidden> wrote:
> >> 3) The export function is a copy-paste-tweak of the core from bzrlib. This
> will lead to bugs as
> >> that code base evolves - we should instead get a supported function in
> bzrlib lib that we can
> >> import and use.
> >
> > Well, there is only one problem with that. According to the WSGI spec, you
> must return an iterable that will export the blocks. If I were to call the
> provided function, it would be impossible to break the response into pieces
> because the provided function would export it all at once. I know that it is a
> copy and paste tweak, but there is really no way I can inject the 'yield'
> keyword into the provided function. If you have another suggestion, I would be
> glad to hear it.
>
> Extract the function in bzrlib into two parts - a generator (what you
> have here) and a consumer than consumes it all triggering the writes.
>
> Then we can reuse the generator.

Okay, I see what you mean.

Revision history for this message
xaav (xaav) wrote : Posted in a previous version of this proposal

See Bug #791005 for further information on this.

Revision history for this message
xaav (xaav) wrote : Posted in a previous version of this proposal

Okay, I think this should work, but I haven't tested it yet.

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

Thanks, xaav.

When you say "haven't tested" do you mean just "not written any tests", or "not even been able to run it"?

Revision history for this message
xaav (xaav) wrote : Posted in a previous version of this proposal

I haven't written any tests (Sorry, I'll do this ASAP.)
I also have not been able to run it because I'm lazy and it requires too much work to get loggerhead running on W1nd0w$.

Revision history for this message
xaav (xaav) wrote : Posted in a previous version of this proposal

Okay, I've added a simple tarfile test. However, I am not able to run the test to I would appreciate if someone would do that for me and/or try to download a tarball from their browser.

Revision history for this message
Gavin Panella (allenap) wrote : Posted in a previous version of this proposal

Once I'd set up a virtualenv with the right prerequisites, I got the
following error when running the test suite:

{{{
Traceback (most recent call last):
  ...
  File ".../loggerhead/tests/test_controllers.py", line 8, in <module>
    from loggerhead.apps.branch import BranchWSGIApp
  File ".../loggerhead/apps/branch.py", line 36, in <module>
    from loggerhead.controllers.download_ui import DownloadUI, DownloadTarballUI
  File ".../loggerhead/controllers/download_ui.py", line 29, in <module>
    from loggerhead.exporter import export_tarball
ImportError: cannot import name export_tarball
}}}

After fixing that I got the following error from
TestDownloadTarballUI.test_download_tarball:

{{{
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/testtools/runtest.py", line 169, in _run_user
    return fn(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/testtools/testcase.py", line 499, in _run_test_method
    return self._get_test_method()()
  File ".../loggerhead/tests/test_controllers.py", line 135, in test_download_tarball
    app = self.setUpLoggerhead()
  File ".../loggerhead/tests/test_simple.py", line 47, in setUpLoggerhead
    branch_app = BranchWSGIApp(self.tree.branch, '', **kw).app
AttributeError: 'TestDownloadTarballUI' object has no attribute 'tree'
}}}

Obviously this needs some work.

We've been talking about taking more of a "patch pilot" approach in
Launchpad. That seems to mean that one of the core team - fwiw, I
would be happy to do it - would actively help getting this landed,
rather than just reviewing it. Would you like that, or would you
prefer to iterate on your own?

review: Needs Fixing
Revision history for this message
xaav (xaav) wrote : Posted in a previous version of this proposal

Sorry, I had been gone. I will be sure and look into this right away!

On Thu, Jun 16, 2011 at 8:56 AM, Gavin Panella
<email address hidden>wrote:

> Review: Needs Fixing
> Once I'd set up a virtualenv with the right prerequisites, I got the
> following error when running the test suite:
>
> {{{
> Traceback (most recent call last):
> ...
> File ".../loggerhead/tests/test_controllers.py", line 8, in <module>
> from loggerhead.apps.branch import BranchWSGIApp
> File ".../loggerhead/apps/branch.py", line 36, in <module>
> from loggerhead.controllers.download_ui import DownloadUI,
> DownloadTarballUI
> File ".../loggerhead/controllers/download_ui.py", line 29, in <module>
> from loggerhead.exporter import export_tarball
> ImportError: cannot import name export_tarball
> }}}
>
> After fixing that I got the following error from
> TestDownloadTarballUI.test_download_tarball:
>
> {{{
> Traceback (most recent call last):
> File "/usr/lib/python2.7/dist-packages/testtools/runtest.py", line 169, in
> _run_user
> return fn(*args, **kwargs)
> File "/usr/lib/python2.7/dist-packages/testtools/testcase.py", line 499,
> in _run_test_method
> return self._get_test_method()()
> File ".../loggerhead/tests/test_controllers.py", line 135, in
> test_download_tarball
> app = self.setUpLoggerhead()
> File ".../loggerhead/tests/test_simple.py", line 47, in setUpLoggerhead
> branch_app = BranchWSGIApp(self.tree.branch, '', **kw).app
> AttributeError: 'TestDownloadTarballUI' object has no attribute 'tree'
> }}}
>
> Obviously this needs some work.
>
> We've been talking about taking more of a "patch pilot" approach in
> Launchpad. That seems to mean that one of the core team - fwiw, I
> would be happy to do it - would actively help getting this landed,
> rather than just reviewing it. Would you like that, or would you
> prefer to iterate on your own?
>
> --
> https://code.launchpad.net/~xaav/loggerhead/export-tarball/+merge/63931
> You are the owner of lp:~xaav/loggerhead/export-tarball.
>

Revision history for this message
xaav (xaav) wrote : Posted in a previous version of this proposal

We've been talking about taking more of a "patch pilot" approach in
Launchpad. That seems to mean that one of the core team - fwiw, I
would be happy to do it - would actively help getting this landed,
rather than just reviewing it. Would you like that, or would you
prefer to iterate on your own?

That would be great! Any help would be greatly appreciated.

Revision history for this message
xaav (xaav) wrote : Posted in a previous version of this proposal

Okay, I've fixed some stuff (the tests are still broken), but here is what I'm getting:

Traceback (most recent call last):
  File "C:\Python27\lib\site-packages\paste-1.7.5.1-py2.7.egg\paste\httpserver.py", l
ine 1068, in process_request_in_thread
    self.finish_request(request, client_address)
  File "C:\Python27\lib\SocketServer.py", line 323, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "C:\Python27\lib\SocketServer.py", line 639, in __init__
    self.handle()
  File "C:\Python27\lib\site-packages\paste-1.7.5.1-py2.7.egg\paste\httpserver.py", l
ine 442, in handle
    BaseHTTPRequestHandler.handle(self)
  File "C:\Python27\lib\BaseHTTPServer.py", line 337, in handle
    self.handle_one_request()
  File "C:\Python27\lib\site-packages\paste-1.7.5.1-py2.7.egg\paste\httpserver.py", l
ine 437, in handle_one_request
    self.wsgi_execute()
  File "C:\Python27\lib\site-packages\paste-1.7.5.1-py2.7.egg\paste\httpserver.py", l
ine 289, in wsgi_execute
    for chunk in result:
  File "C:\Users\Xaav\workspace\loggerhead\loggerhead\exporter.py", line 31, in e
xport_archive
    for _ in get_export_generator(tree=tree, fileobj=fileobj, format=format):
  File "C:\Python27\lib\site-packages\bzrlib\export\__init__.py", line 112, in get_ex
port_generator
    root = get_root_name(dest)
  File "C:\Python27\lib\site-packages\bzrlib\export\__init__.py", line 174, in get_ro
ot_name
    dest = os.path.basename(dest)
  File "C:\Python27\lib\ntpath.py", line 198, in basename
    return split(p)[1]
  File "C:\Python27\lib\ntpath.py", line 170, in split
    d, p = splitdrive(p)
  File "C:\Python27\lib\ntpath.py", line 125, in splitdrive
    if p[1:2] == ':':
TypeError: 'NoneType' object is not subscriptable

Any help would be appreciated.

Revision history for this message
Gavin Panella (allenap) wrote : Posted in a previous version of this proposal

Cool. I am busy this week, but I might get to it. If not, next week for sure.

Revision history for this message
Vincent Ladeuil (vila) wrote : Posted in a previous version of this proposal
Download full text (3.9 KiB)

Hi,

I almost got the test running with some additional fixes
(available at lp:~vila/loggerhead/export-tarball) only to run
into a bug in bzr itself (I think you should be able to fix that
one ;).

Note that your code requires bzr >= 2.4 (launchpad only runs
2.3.3 so far) so we'll need some support from the lp guys to
deploy a more recent version there.

Summary of my fixes:

- you need to create a branch (with some content even) before
  being able to call

         app = self.setUpLoggerhead()

  So I've added a setUp method for your class to do that. You
  probably want to add *more* tests to check that you get a valid
  tarball with the expected content (which an empty branch
  doesn't allow ;).

- you're calling get_export_generator without dest nor root and
  the code in bzrlib defaults to dest to set root. This raises an
  interesing point: which root should be used here (i.e. what do
  we want to prefix all the paths in the archive
  with). <project>-<branch nick>-<revno> may be nice (but ask
  others for feedback too).

- you used '.tar.gz' for the format but bzr expects either 'tgz'
  OR a dest file name to deduce the format from the file
  suffix. I just used 'tgz' there.

With these fixes in place we get:

======================================================================
ERROR: bzrlib.plugins.loggerhead.loggerhead.tests.test_controllers.TestDownloadTarballUI.test_download_tarball
----------------------------------------------------------------------
_StringException: Text attachment: log
------------
0.622 creating repository in file:///tmp/testbzr-KY_qfE.tmp/bzrlib.plugins.loggerhead.loggerhead.tests.test_controllers.TestDownloadTarballUI.test_download_tarball/work/.bzr/.
0.624 creating branch <bzrlib.branch.BzrBranchFormat7 object at 0x22ae990> in file:///tmp/testbzr-KY_qfE.tmp/bzrlib.plugins.loggerhead.loggerhead.tests.test_controllers.TestDownloadTarballUI.test_download_tarball/work/
0.631 trying to create missing lock '/tmp/testbzr-KY_qfE.tmp/bzrlib.plugins.loggerhead.loggerhead.tests.test_controllers.TestDownloadTarballUI.test_download_tarball/work/.bzr/checkout/dirstate'
0.631 opening working tree '/tmp/testbzr-KY_qfE.tmp/bzrlib.plugins.loggerhead.loggerhead.tests.test_controllers.TestDownloadTarballUI.test_download_tarball/work'
0.642 export version <InventoryRevisionTree instance at 29ece90, rev_id='null:'>
0.649 opening working tree '/tmp/testbzr-KY_qfE.tmp'
------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/testtools/runtest.py", line 169, in _run_user
    return fn(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/testtools/testcase.py", line 499, in _run_test_method
    return self._get_test_method()()
  File "/home/vila/.bazaar/plugins/loggerhead/loggerhead/tests/test_controllers.py", line 140, in test_download_tarball
    res = app.get('/tarball')
  File "/usr/lib/pymodules/python2.7/paste/fixture.py", line 208, in get
    return self.do_request(req, status=status)
  File "/usr/lib/pymodules/python2.7/paste/fixture.py", line 389, in do_request
    **req.environ)
  File "/usr/lib/pymodules/python2.7/paste/wsgilib.py", li...

Read more...

review: Needs Fixing
Revision history for this message
Gavin Panella (allenap) wrote :

Hi,

Please can you resolve the conflicts?

  Text conflict in loggerhead/apps/branch.py
  Text conflict in loggerhead/controllers/revision_ui.py
  Text conflict in loggerhead/tests/test_controllers.py

Gavin.

Revision history for this message
xaav (xaav) wrote :

Okay.

lp:~xaav/loggerhead/export-tarball updated
451. By xaav <email address hidden> on 2011-07-07

Merged lp:loggerhead

452. By xaav <email address hidden> on 2011-07-07

Fixed extension issue

Revision history for this message
xaav (xaav) wrote :

Is there any reason this is being held up? It worked fine for me.

Revision history for this message
Martin Pool (mbp) wrote :

[fix] It looks like you want tarballs disabled by default (which I agree with), so it seems a bit strange to me that the keyword arguments default to true.

[fix] You seem to be giving people uncompressed tarballs? .tgz is probably better. The default value to export_archive is unnecessary and probably just a distraction if it's called from only one place that does specify the value.

Were you able to get the tests to run and pass?

Aside from that it looks good to me.

review: Needs Fixing
Revision history for this message
xaav (xaav) wrote :

> [fix] You seem to be giving people uncompressed tarballs? .tgz is probably better. The default value to export_archive is unnecessary and probably just a distraction if it's called from only one place that does specify the value.

.tgz tarballs are not possible due to a bug in bzr. It is not within the scope of this patch to fix that bug.

> Were you able to get the tests to run and pass?

Yes. Everything passes.

> [fix] It looks like you want tarballs disabled by default (which I agree with), so it seems a bit strange to me that the keyword arguments default to true.

What line are you referring to?

lp:~xaav/loggerhead/export-tarball updated
453. By xaav on 2011-07-10

Fixed buggy merging.

454. By xaav <email address hidden> on 2011-07-10

Fixed buggy merging and removed IDE files

Revision history for this message
Martin Pool (mbp) wrote :

> .tgz tarballs are not possible due to a bug in bzr. It is not within the scope of this patch to fix that bug.

So, when we fix that bug, we'll change this to send tgz instead?

Could you tell me the bug number? Perhaps it would be worth
mentioning this in a comment
>
>
>> Were you able to get the tests to run and pass?
>
> Yes. Everything passes.

Great!

>
>> [fix] It looks like you want tarballs disabled by default (which I agree with), so it seems a bit strange to me that the keyword arguments default to true.
>
> What line are you referring to?
>
@@ -49,7 +49,7 @@

     def __init__(self, branch, friendly_name=None, config={},
                  graph_cache=None, branch_link=None, is_root=False,
- served_url=_DEFAULT, use_cdn=False):
+ served_url=_DEFAULT, use_cdn=False, export_tarballs=True):
         self.branch = branch
         self._config = config
         self.friendly_name = friendly_name

Revision history for this message
xaav (xaav) wrote :

> Could you tell me the bug number? Perhaps it would be worth mentioning this in a comment.

I really don't know if the bug has been reported. All I know is if it is changed to .tar.gz, a 500 internal server error occurs and the traceback goes right into bzrlib.

I'm not the only one with the problem, vila encountered the same problem. For more information, see his comment.

lp:~xaav/loggerhead/export-tarball updated
455. By xaav on 2011-07-11

Fixed serve tarballs issue.

Revision history for this message
xaav (xaav) wrote :

>
>> [fix] It looks like you want tarballs disabled by default (which I agree with), so it seems a bit strange to me that the keyword arguments default to true.
>
> What line are you referring to?
>
@@ -49,7 +49,7 @@

     def __init__(self, branch, friendly_name=None, config={},
                  graph_cache=None, branch_link=None, is_root=False,
- served_url=_DEFAULT, use_cdn=False):
+ served_url=_DEFAULT, use_cdn=False, export_tarballs=True):
         self.branch = branch
         self._config = config
         self.friendly_name = friendly_name

Fixed.

Revision history for this message
Martin Pool (mbp) wrote :

Oh I see. That looks pretty trivial; I'll see about fixing it.

Revision history for this message
Martin Pool (mbp) wrote :

[fix] You'll need to also remove this line:

  from bzrlib.export.tar_exporter import export_tarball

from history.py since it's not used and doesn't exist.

I fixed the unbound local warning here: https://code.launchpad.net/~mbp/bzr/trivial/+merge/67474

When I run the tests for this (with those two fixes) I get a failure in your new test_download_tarball, because the response is

303 See Other
The resource has been moved to /;
you should be redirected automatically.

which makes it look like it's not actually matching the URL. Do you see this too?

Revision history for this message
Martin Pool (mbp) wrote :

I'd really like to see this landed and live, so if you want any help just ask, here or on #bzr irc.

Revision history for this message
xaav (xaav) wrote :

> When I run the tests for this (with those two fixes) I get a failure in your new test_download_tarball, because the response is

This is probably due to revision 455. I'll see what I can do.

Revision history for this message
xaav (xaav) wrote :

Try now.

lp:~xaav/loggerhead/export-tarball updated
456. By xaav on 2011-07-11

Fixed tests.

Revision history for this message
Martin Pool (mbp) wrote :

I get the same failure. You don't?

Revision history for this message
Martin Pool (mbp) wrote :

However, changing it to just `res.body` fixes it, and then everything passes. I'll also add some checks that the response has the right status code and headers, and I'll see if we can change it to get a tgz now my fix has landed.

Revision history for this message
xaav (xaav) wrote :

> and I'll see if we can change it to get a tgz now my fix has landed.

This is bleeding-edge code, so you'll need to get lp to upgrade their bzr version.

Revision history for this message
Martin Pool (mbp) wrote :

On 11 July 2011 13:34, Xaav <email address hidden> wrote:
>> and I'll see if we can change it to get a tgz now my fix has landed.
>
> This is bleeding-edge code, so you'll need to get lp to upgrade their bzr version.

I think this is ok; landing a new bazaar at the same time as a new bzr
is no harder.

I'm going to switch it to sending tgz, and test it a bit
interactively, and I think then we'll be good to go.

lp:~xaav/loggerhead/export-tarball updated
457. By xaav on 2011-07-11

Added to UI.

Revision history for this message
xaav (xaav) wrote :

I reverted the change in 455, because I couldn't download tarballs via a web browser.

Revision history for this message
Martin Pool (mbp) wrote :

> I reverted the change in 455, because I couldn't download tarballs via a web
> browser.

Right, specifically turning it back on by default. I think that's fine.

I'm just testing it interactively in a browser and it does seem to work ok, which is great.

[fix] There is one bug which is that the tarball link always gets you the tarball from the current version of the branch, even if you're looking at an older version, which I think people would find confusing. Could you try to fix that?

My small changes are in lp:~mbp/loggerhead/export-tarball

Revision history for this message
xaav (xaav) wrote :

BTW, that link is broken.

Revision history for this message
xaav (xaav) wrote :

> [fix] There is one bug which is that the tarball link always gets you the tarball from the current version of the branch, even if you're looking at an older version, which I think people would find confusing. Could you try to fix that?

This is really a UI problem. Following the link /tarball/12 downloads revision 12, but I can't access the revision number from menu.pt, and I'm not that familiar w/ TAL.

lp:~xaav/loggerhead/export-tarball updated
458. By xaav on 2011-07-12

UI fixes.

Revision history for this message
Martin Pool (mbp) wrote :

Sorry for the delay - that looks good to me.

Revision history for this message
Martin Pool (mbp) :
review: Approve
Revision history for this message
Ian Booth (wallyworld) wrote :

Given the number of eyeballs that have already seen this mp, it looks ok to me apart from some small lint issues. I can fix those and then land this mp.

review: Approve (code*)
Revision history for this message
xaav (xaav) wrote :

Is this merged? If so, should the bug be closed?

Revision history for this message
Martin Pool (mbp) wrote :

Hi xaav, it's not merged yet.

I spoke to Ian, and our plan is:
 * we will get a final review, maybe from John
 * Ian will test it out locally and if that's ok he will merge it in
to loggerhead trunk and shepherd deployment on lp

Revision history for this message
John A Meinel (jameinel) wrote :

[info] You are already exposing information "can_export" into the menu.pt. Why not just expose the revision number there instead. So "exported_tarball_revno" can be None indicating it has no tarball to download, or can be a string indicating what URL (or part of url, or whatever) to use.

[needsinfo] This defaults to always offering download links. Which means that if this rolls out on Launchpad, we will be letting people download via tarball. It hasn't been clear to me if this was specifically approved for Launchpad. As such *I* would tend to change:

25 def __init__(self, branch, friendly_name=None, config={},
26 graph_cache=None, branch_link=None, is_root=False,
27 - served_url=_DEFAULT, use_cdn=False):
28 + served_url=_DEFAULT, use_cdn=False, export_tarballs=True):

To "export_tarballs=False". Then if Launchpad wants to expose tarballs, it can chose to do so. We can easily update the 'bzr serve --http' and './serve_branches' code so that they default to exposing tarballs, without changing Launchpad's policy without their explicit review of it.

[needsinfo] This does all the exporting as raw '.tar' files. Without a way to configure it. One idea would be to change the 'export_tarballs' flag. Instead of being a boolean, it could be "tarball_format=None". If the format is None, then we don't export tarballs. Otherwise it can be 'tar' or 'tar.gz', or whatever.

I'm certainly open to discussion on these points, but I do think we should have answers for them before we land this.

review: Needs Information
Revision history for this message
Gavin Panella (allenap) :
review: Abstain
Revision history for this message
Robert Collins (lifeless) wrote :

FWIW the default being on is fine - we can override when we upgrade
LP's revno. The format should default to something sensible like
tar.gz I think.

Revision history for this message
Martin Pool (mbp) wrote :

So the main remaining point then seems to be it might be nice to
compress them by default. But even there, it's pretty marginal
benefit.

And then it just needs someone to shepherd this through landing and deployment.

Anything else?

Martin

Revision history for this message
Robert Collins (lifeless) wrote :

Nothing else from me.

Revision history for this message
Martin Pool (mbp) wrote :

I will see about turning on compression, testing it, and landing it.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file '.bzrignore'
--- .bzrignore 2011-03-10 14:22:12 +0000
+++ .bzrignore 2011-07-12 03:33:24 +0000
@@ -8,3 +8,5 @@
8loggerhead-memprofile8loggerhead-memprofile
9./docs/_build/9./docs/_build/
10tags10tags
11.project
12.pydevproject
1113
=== modified file 'loggerhead/apps/branch.py'
--- loggerhead/apps/branch.py 2011-06-28 16:09:37 +0000
+++ loggerhead/apps/branch.py 2011-07-12 03:33:24 +0000
@@ -33,7 +33,7 @@
33from loggerhead.controllers.atom_ui import AtomUI33from loggerhead.controllers.atom_ui import AtomUI
34from loggerhead.controllers.changelog_ui import ChangeLogUI34from loggerhead.controllers.changelog_ui import ChangeLogUI
35from loggerhead.controllers.diff_ui import DiffUI35from loggerhead.controllers.diff_ui import DiffUI
36from loggerhead.controllers.download_ui import DownloadUI36from loggerhead.controllers.download_ui import DownloadUI, DownloadTarballUI
37from loggerhead.controllers.filediff_ui import FileDiffUI37from loggerhead.controllers.filediff_ui import FileDiffUI
38from loggerhead.controllers.inventory_ui import InventoryUI38from loggerhead.controllers.inventory_ui import InventoryUI
39from loggerhead.controllers.revision_ui import RevisionUI39from loggerhead.controllers.revision_ui import RevisionUI
@@ -49,7 +49,7 @@
4949
50 def __init__(self, branch, friendly_name=None, config={},50 def __init__(self, branch, friendly_name=None, config={},
51 graph_cache=None, branch_link=None, is_root=False,51 graph_cache=None, branch_link=None, is_root=False,
52 served_url=_DEFAULT, use_cdn=False):52 served_url=_DEFAULT, use_cdn=False, export_tarballs=True):
53 self.branch = branch53 self.branch = branch
54 self._config = config54 self._config = config
55 self.friendly_name = friendly_name55 self.friendly_name = friendly_name
@@ -61,6 +61,7 @@
61 self.is_root = is_root61 self.is_root = is_root
62 self.served_url = served_url62 self.served_url = served_url
63 self.use_cdn = use_cdn63 self.use_cdn = use_cdn
64 self.export_tarballs = export_tarballs
6465
65 def get_history(self):66 def get_history(self):
66 file_cache = None67 file_cache = None
@@ -126,6 +127,7 @@
126 'revision': RevisionUI,127 'revision': RevisionUI,
127 'search': SearchUI,128 'search': SearchUI,
128 'view': ViewUI,129 'view': ViewUI,
130 'tarball': DownloadTarballUI,
129 }131 }
130132
131 def last_updated(self):133 def last_updated(self):
132134
=== modified file 'loggerhead/config.py'
--- loggerhead/config.py 2010-05-12 14:38:05 +0000
+++ loggerhead/config.py 2011-07-12 03:33:24 +0000
@@ -36,6 +36,7 @@
36 use_cdn=False,36 use_cdn=False,
37 sql_dir=None,37 sql_dir=None,
38 allow_writes=False,38 allow_writes=False,
39 export_tarballs=True,
39 )40 )
40 parser.add_option("--user-dirs", action="store_true",41 parser.add_option("--user-dirs", action="store_true",
41 help="Serve user directories as ~user.")42 help="Serve user directories as ~user.")
@@ -75,6 +76,8 @@
75 help="The directory to place the SQL cache in")76 help="The directory to place the SQL cache in")
76 parser.add_option("--allow-writes", action="store_true",77 parser.add_option("--allow-writes", action="store_true",
77 help="Allow writing to the Bazaar server.")78 help="Allow writing to the Bazaar server.")
79 parser.add_option("--export-tarballs", action="store_true",
80 help="Allow exporting revisions to tarballs.")
78 return parser81 return parser
7982
8083
8184
=== modified file 'loggerhead/controllers/__init__.py'
--- loggerhead/controllers/__init__.py 2011-06-28 16:09:37 +0000
+++ loggerhead/controllers/__init__.py 2011-07-12 03:33:24 +0000
@@ -21,7 +21,7 @@
21import simplejson21import simplejson
22import time22import time
2323
24from paste.httpexceptions import HTTPNotFound24from paste.httpexceptions import HTTPNotFound, HTTPSeeOther
25from paste.request import path_info_pop, parse_querystring25from paste.request import path_info_pop, parse_querystring
2626
27from loggerhead import util27from loggerhead import util
2828
=== modified file 'loggerhead/controllers/download_ui.py'
--- loggerhead/controllers/download_ui.py 2010-05-05 19:03:40 +0000
+++ loggerhead/controllers/download_ui.py 2011-07-12 03:33:24 +0000
@@ -19,46 +19,51 @@
1919
20import logging20import logging
21import mimetypes21import mimetypes
22import os
22import urllib23import urllib
2324
24from paste import httpexceptions25from paste import httpexceptions
25from paste.request import path_info_pop26from paste.request import path_info_pop
2627
27from loggerhead.controllers import TemplatedBranchView28from loggerhead.controllers import TemplatedBranchView
29from loggerhead.exporter import export_archive
2830
29log = logging.getLogger("loggerhead.controllers")31log = logging.getLogger("loggerhead.controllers")
3032
3133
32class DownloadUI (TemplatedBranchView):34class DownloadUI (TemplatedBranchView):
3335
34 def __call__(self, environ, start_response):36 def encode_filename(self, filename):
35 # /download/<rev_id>/<file_id>/[filename]37
3638 return urllib.quote(filename.encode('utf-8'))
37 h = self._history39
3840 def get_args(self, environ):
39 args = []41 args = []
40 while True:42 while True:
41 arg = path_info_pop(environ)43 arg = path_info_pop(environ)
42 if arg is None:44 if arg is None:
43 break45 break
44 args.append(arg)46 args.append(arg)
47 return args
4548
49 def __call__(self, environ, start_response):
50 # /download/<rev_id>/<file_id>/[filename]
51 h = self._history
52 args = self.get_args(environ)
46 if len(args) < 2:53 if len(args) < 2:
47 raise httpexceptions.HTTPMovedPermanently(54 raise httpexceptions.HTTPMovedPermanently(
48 self._branch.absolute_url('/changes'))55 self._branch.absolute_url('/changes'))
49
50 revid = h.fix_revid(args[0])56 revid = h.fix_revid(args[0])
51 file_id = args[1]57 file_id = args[1]
52 path, filename, content = h.get_file(file_id, revid)58 path, filename, content = h.get_file(file_id, revid)
53 mime_type, encoding = mimetypes.guess_type(filename)59 mime_type, encoding = mimetypes.guess_type(filename)
54 if mime_type is None:60 if mime_type is None:
55 mime_type = 'application/octet-stream'61 mime_type = 'application/octet-stream'
56
57 self.log.info('/download %s @ %s (%d bytes)',62 self.log.info('/download %s @ %s (%d bytes)',
58 path,63 path,
59 h.get_revno(revid),64 h.get_revno(revid),
60 len(content))65 len(content))
61 encoded_filename = urllib.quote(filename.encode('utf-8'))66 encoded_filename = self.encode_filename(filename)
62 headers = [67 headers = [
63 ('Content-Type', mime_type),68 ('Content-Type', mime_type),
64 ('Content-Length', str(len(content))),69 ('Content-Length', str(len(content))),
@@ -67,3 +72,29 @@
67 ]72 ]
68 start_response('200 OK', headers)73 start_response('200 OK', headers)
69 return [content]74 return [content]
75
76
77class DownloadTarballUI(DownloadUI):
78
79 def __call__(self, environ, start_response):
80 """Stream a tarball from a bazaar branch."""
81 # Tried to re-use code from downloadui, not very successful
82 format = "tar"
83 history = self._history
84 self.args = self.get_args(environ)
85 if len(self.args):
86 revid = history.fix_revid(self.args[0])
87 else:
88 revid = self.get_revid()
89 if self._branch.export_tarballs:
90 root = 'branch'
91 encoded_filename = self.encode_filename(root + '.' + format)
92 headers = [
93 ('Content-Type', 'application/octet-stream'),
94 ('Content-Disposition',
95 "attachment; filename*=utf-8''%s" % (encoded_filename,)),
96 ]
97 start_response('200 OK', headers)
98 return export_archive(history, root, revid, format)
99 else:
100 raise httpexceptions.HTTPSeeOther('/')
70101
=== modified file 'loggerhead/controllers/revision_ui.py'
--- loggerhead/controllers/revision_ui.py 2011-06-27 17:11:24 +0000
+++ loggerhead/controllers/revision_ui.py 2011-07-12 03:33:24 +0000
@@ -136,6 +136,7 @@
136 self._branch.friendly_name,136 self._branch.friendly_name,
137 self._branch.is_root,137 self._branch.is_root,
138 'changes'))138 'changes'))
139 can_export = self._branch.export_tarballs
139140
140 values.update({141 values.update({
141 'history': self._history,142 'history': self._history,
@@ -149,6 +150,7 @@
149 'filter_file_id': filter_file_id,150 'filter_file_id': filter_file_id,
150 'diff_chunks': diff_chunks,151 'diff_chunks': diff_chunks,
151 'query': query,152 'query': query,
153 'can_export': can_export,
152 'specific_path': path,154 'specific_path': path,
153 'start_revid': start_revid,155 'start_revid': start_revid,
154 })156 })
155157
=== added file 'loggerhead/exporter.py'
--- loggerhead/exporter.py 1970-01-01 00:00:00 +0000
+++ loggerhead/exporter.py 2011-07-12 03:33:24 +0000
@@ -0,0 +1,46 @@
1# Copyright (C) 2011 Canonical Ltd
2#
3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by
5# the Free Software Foundation; either version 2 of the License, or
6# (at your option) any later version.
7#
8# This program is distributed in the hope that it will be useful,
9# but WITHOUT ANY WARRANTY; without even the implied warranty of
10# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11# GNU General Public License for more details.
12#
13"""Exports an archive from a bazaar branch"""
14
15from bzrlib.export import get_export_generator
16
17
18class ExporterFileObject(object):
19
20 def __init__(self):
21 self._buffer = []
22
23 def write(self, s):
24 self._buffer.append(s)
25
26 def get_buffer(self):
27 try:
28 return ''.join(self._buffer)
29 finally:
30 self._buffer = []
31
32
33def export_archive(history, root, revid, format=".tar.gz"):
34 """Export tree contents to an archive
35
36 :param history: Instance of history to export
37 :param root: Root location inside the archive.
38 :param revid: Revision to export
39 :param format: Format of the archive
40 """
41 fileobj = ExporterFileObject()
42 tree = history._branch.repository.revision_tree(revid)
43 for _ in get_export_generator(tree=tree, root=root, fileobj=fileobj, format=format):
44 yield fileobj.get_buffer()
45 # Might have additonal contents written
46 yield fileobj.get_buffer()
047
=== modified file 'loggerhead/history.py'
--- loggerhead/history.py 2011-03-25 13:09:10 +0000
+++ loggerhead/history.py 2011-07-12 03:33:24 +0000
@@ -33,6 +33,7 @@
33import re33import re
34import textwrap34import textwrap
35import threading35import threading
36import tarfile
3637
37from bzrlib import tag38from bzrlib import tag
38import bzrlib.branch39import bzrlib.branch
@@ -44,6 +45,7 @@
44from loggerhead import search45from loggerhead import search
45from loggerhead import util46from loggerhead import util
46from loggerhead.wholehistory import compute_whole_history_data47from loggerhead.wholehistory import compute_whole_history_data
48from bzrlib.export.tar_exporter import export_tarball
4749
4850
49def is_branch(folder):51def is_branch(folder):
@@ -816,4 +818,6 @@
816 renamed=sorted(reporter.renamed, key=lambda x: x.new_filename),818 renamed=sorted(reporter.renamed, key=lambda x: x.new_filename),
817 removed=sorted(reporter.removed, key=lambda x: x.filename),819 removed=sorted(reporter.removed, key=lambda x: x.filename),
818 modified=sorted(reporter.modified, key=lambda x: x.filename),820 modified=sorted(reporter.modified, key=lambda x: x.filename),
819 text_changes=sorted(reporter.text_changes, key=lambda x: x.filename))821 text_changes=sorted(reporter.text_changes,
822 key=lambda x: x.filename))
823
820824
=== added directory 'loggerhead/static/downloads'
=== modified file 'loggerhead/templates/menu.pt'
--- loggerhead/templates/menu.pt 2011-02-09 22:33:59 +0000
+++ loggerhead/templates/menu.pt 2011-07-12 03:33:24 +0000
@@ -6,6 +6,8 @@
6 id string:on">Changes</a></li>6 id string:on">Changes</a></li>
7 <li><a tal:attributes="href python:url('/files', clear=1);7 <li><a tal:attributes="href python:url('/files', clear=1);
8 title string:Files">Files</a></li>8 title string:Files">Files</a></li>
9 <li><a tal:attributes="href python:url(['/revision', change.revno], clear=1);
10 title string:Tarball">Tarball</a></li>
9 </tal:changes-active>11 </tal:changes-active>
10 <tal:fileview-active tal:condition="fileview_active">12 <tal:fileview-active tal:condition="fileview_active">
11 <li><a tal:attributes="href python:url('/changes', clear=1);13 <li><a tal:attributes="href python:url('/changes', clear=1);
@@ -13,6 +15,8 @@
13 <li><a tal:attributes="href python:url('/files', clear=1);15 <li><a tal:attributes="href python:url('/files', clear=1);
14 title string:Files;16 title string:Files;
15 id string:on">Files</a></li>17 id string:on">Files</a></li>
18 <li><a tal:attributes="href python:url(['/revision', change.revno], clear=1);
19 title string:Tarball">Tarball</a></li>
16 </tal:fileview-active>20 </tal:fileview-active>
17 </ul>21 </ul>
18</tal:menu>22</tal:menu>
1923
=== modified file 'loggerhead/templates/revision.pt'
--- loggerhead/templates/revision.pt 2011-06-28 16:51:55 +0000
+++ loggerhead/templates/revision.pt 2011-07-12 03:33:24 +0000
@@ -84,7 +84,10 @@
84 tal:attributes="href python:url(['/diff', change.revno], clear=1)">download diff</a>84 tal:attributes="href python:url(['/diff', change.revno], clear=1)">download diff</a>
85 <a tal:condition="python:compare_revid is not None"85 <a tal:condition="python:compare_revid is not None"
86 tal:attributes="href python:url(['/diff', change.revno, history.get_revno(compare_revid)], clear=1)">download diff</a>86 tal:attributes="href python:url(['/diff', change.revno, history.get_revno(compare_revid)], clear=1)">download diff</a>
87 </li>87 </li>
88 <li tal:condition="python:can_export">
89 <a tal:attributes="href python:url(['/tarball', change.revno])">Download tarball</a>
90 </li>
88 <li id="last"><a tal:attributes="href python:url(['/changes', change.revno]);91 <li id="last"><a tal:attributes="href python:url(['/changes', change.revno]);
89 title string:view history from revision ${change/revno}"92 title string:view history from revision ${change/revno}"
90 tal:content="string:view history from revision ${change/revno}"></a></li>93 tal:content="string:view history from revision ${change/revno}"></a></li>
9194
=== modified file 'loggerhead/tests/test_controllers.py'
--- loggerhead/tests/test_controllers.py 2011-06-28 16:06:12 +0000
+++ loggerhead/tests/test_controllers.py 2011-07-12 03:33:24 +0000
@@ -1,3 +1,10 @@
1from cStringIO import StringIO
2import logging
3import tarfile
4
5from paste.httpexceptions import HTTPServerError
6
7from bzrlib import errors
1import simplejson8import simplejson
29
3from loggerhead.apps.branch import BranchWSGIApp10from loggerhead.apps.branch import BranchWSGIApp
@@ -204,3 +211,16 @@
204 revlog_ui = branch_app.lookup_app(env)211 revlog_ui = branch_app.lookup_app(env)
205 self.assertOkJsonResponse(revlog_ui, env)212 self.assertOkJsonResponse(revlog_ui, env)
206213
214class TestDownloadTarballUI(BasicTests):
215
216 def setUp(self):
217 super(TestDownloadTarballUI, self).setUp()
218 self.createBranch()
219
220 def test_download_tarball(self):
221 app = self.setUpLoggerhead()
222 res = app.get('/tarball')
223 f = open('tarball', 'w')
224 f.write(res)
225 f.close()
226 self.failIf(not tarfile.is_tarfile('tarball'))
207\ No newline at end of file227\ No newline at end of file
208228
=== modified file 'loggerhead/tests/test_simple.py'
--- loggerhead/tests/test_simple.py 2011-06-28 14:19:58 +0000
+++ loggerhead/tests/test_simple.py 2011-07-12 03:33:24 +0000
@@ -46,7 +46,7 @@
46 self.tree = self.make_branch_and_tree('.')46 self.tree = self.make_branch_and_tree('.')
4747
48 def setUpLoggerhead(self, **kw):48 def setUpLoggerhead(self, **kw):
49 branch_app = BranchWSGIApp(self.tree.branch, '', **kw).app49 branch_app = BranchWSGIApp(self.tree.branch, '', export_tarballs=True, **kw).app
50 return TestApp(HTTPExceptionHandler(branch_app))50 return TestApp(HTTPExceptionHandler(branch_app))
5151
52 def assertOkJsonResponse(self, app, env):52 def assertOkJsonResponse(self, app, env):

Subscribers

People subscribed via source and target branches