Doesn't support uploading symlinks

[Jelmer Vernooij]

  affects bzr-upload

It would be nice if bzr-upload supported uploading symlinks.
--
Jelmer Vernooij <email address hidden> - http://samba.org/~jelmer/
Jabber: <email address hidden>

[Vincent Ladeuil (vila)]

1) No need to report it twice
2) No need to report it twice
3) That can work only if the protocol used allows creating symlinks
4) That can work only if the server support symlinks

But yes, that would be nice were applicable :)

[Andrew Schulman (andrex)]

I can live with upload not supporting symlinks, because I can work around that. But what's really a problem is that upload crashes when it encounters one:

$ bzr upload
Using saved location: sftp://ck/vservers/iponly167882/trunk/
Uploading .htaccess
Uploading .project
Uploading CHANGELOG.txt
Uploading COPYRIGHT.txt
Uploading INSTALL.mysql.txt
Uploading INSTALL.pgsql.txt
Uploading INSTALL.txt
Uploading LICENSE.txt
Uploading MAINTAINERS.txt
Uploading UPGRADE.txt
Uploading cron.php
bzr: ERROR: exceptions.NotImplementedError: don't known how to upload 'symlink'

Traceback (most recent call last):
  File "/usr/lib/python2.5/site-packages/bzrlib/commands.py", line 846, in run_bzr_catch_errors
    return run_bzr(argv)
  File "/usr/lib/python2.5/site-packages/bzrlib/commands.py", line 797, in run_bzr
    ret = run(*run_argv)
  File "/usr/lib/python2.5/site-packages/bzrlib/commands.py", line 499, in run_argv_aliases
    return self.run(**all_cmd_args)
  File "/usr/lib/python2.5/site-packages/bzrlib/plugins/upload/__init__.py", line 121, in run
    self.upload_full_tree()
  File "/usr/lib/python2.5/site-packages/bzrlib/plugins/upload/__init__.py", line 234, in upload_full_tree
    raise NotImplementedError("don't known how to upload %r" % ie.kind)
NotImplementedError: don't known how to upload 'symlink'

bzr 1.5 on python 2.5.2 (linux2)
arguments: ['/usr/bin/bzr', 'upload', '--full']
encoding: 'ISO-8859-1', fsenc: 'ISO-8859-1', lang: 'en_US'
plugins:
  bzrtools /usr/lib/python2.5/site-packages/bzrlib/plugins/bzrtools [1.5.0]
  lastlog /home/andrex/.bazaar/plugins/lastlog [unknown]
  launchpad /usr/lib/python2.5/site-packages/bzrlib/plugins/launchpad [unknown]
  push_and_update /home/andrex/.bazaar/plugins/push_and_update [0.2.0dev0]
  upload /usr/lib/python2.5/site-packages/bzrlib/plugins/upload [0.1.0]
  xmloutput /home/andrex/.bazaar/plugins/xmloutput [0.8.0]
*** Bazaar has encountered an internal error.
    Please report a bug at https://bugs.launchpad.net/bzr/+filebug
    including this traceback, and a description of what you
    were doing when the error occurred.

Thanks,
Andrew.

[Anthony Bush (awbush)]

When the protocol or server does not support symlinks, why not just make a copy of the file that the symlink is pointing to?

[Neil Santos (rls)]

Out of curiousity, what would be needed for bzr-upload to support symlinks? I've created a hack that handles symlinks locally, but it's ugly (even for a hack), merely a way to shoehorn support in because I really need it.

From what I've learned in implementing my hack, it seems Bzr's Transports would need to implement it on a as-supported basis. Or is this overkill, and would it be enough to localize the work inside bzr-upload?

[Martin Pool (mbp)]

On 17 February 2010 01:24, Neil Santos <email address hidden> wrote:
> Out of curiousity, what would be needed for bzr-upload to support
> symlinks?  I've created a hack that handles symlinks locally, but it's
> ugly (even for a hack), merely a way to shoehorn support in because I
> really need it.
>
> >From what I've learned in implementing my hack, it seems Bzr's
> Transports would need to implement it on a as-supported basis.  Or is
> this overkill, and would it be enough to localize the work inside bzr-
> upload?
>
> ** Patch added: "Quick and dirty hack to support for dealing with symlinks using LocalTransport on *nix systems"
>   http://launchpadlibrarian.net/39268272/bzr-upload_unix-symlink-support.patch

That patch looks basically reasonable. You should put up a branch and
propose a merge.

A few comments:

+ def upload_symlink_robustly(self, relpath, symlink_target):
+ if not isinstance(self.to_transport, transport.local.LocalTransport):
+ raise NotImplementedError

I don't see why you can't do it on non-local transports as long as
they support symlinks? Similarly there's no reason why it should
matter whether the local os supports them or not. Instead just try it
and check for TransportNotPossible.

+ try:
+ st = self._up_stat(relpath)
+ if stat.S_ISDIR(st.st_mode):
+ # A simple rmdir may not be enough
+ if not self.quiet:
+ self.outf.write('Clearing %s/%s\n' % (
+ self.to_transport.external_url(), relpath))
+ self._up_delete_tree(relpath)
+ elif stat.S_ISLNK(st.st_mode):
+ if not self.quiet:
+ self.outf.write('Clearing %s/%s\n' % (
+ self.to_transport.external_url(), relpath))
+ self._up_delete(relpath)
+ except errors.PathError:
+ pass
+
+ if osutils.has_symlinks():
+ # Target might not be there at this time; dummy file should be
+ # overwritten at some point, possibly by another upload.
+ target_path = osutils.normpath(osutils.pathjoin(
+ osutils.dirname(relpath),
+ symlink_target))

It seems like this should be done by url manipulation not os paths, or
it will break on windows.

+ self.make_remote_dir_robustly(osutils.dirname(target_path))
+ self._up_put_bytes(target_path, "", None)
+
+ os.symlink(symlink_target,
+ self.to_transport.local_abspath(relpath))
+ else:
+ raise NotImplementedError
+
     def make_remote_dir(self, relpath, mode=None):
         if mode is None:

A test for this would be nice, if bzr-upload has enough test framework
to support it.

Thanks!
--
Martin <http://launchpad.net/~mbp/>

[Neil Santos (rls)]

> That patch looks basically reasonable. You should put up a branch and propose a merge.

I did that, except for the merge proposal. I'm not comfortable asking the devs to merge something this hacky, so I'll try to work on it a bit more. Luckily, I need bzr-upload to support symlinks for non-local transports, as well, so I can pretty well justify working on this.

> I don't see why you can't do it on non-local transports as long as
> they support symlinks? Similarly there's no reason why it should
> matter whether the local os supports them or not. Instead just try it
> and check for TransportNotPossible.

My only objection is that I don't know how to do just that (not yet, in any case). My Google-fu seems to be amiss when trying to figure out how bzrlib works, especially the Transport classes-- I would appreciate pointers towards the right direction.

At first, I thought I should give the Transport classes the ability to do symlinks by themselves, since, as I implied earlier, they don't seem to know how to do just that. I still think this would be the best way to handle this, but I know so little about bzrlib and bzr-upload that I won't bet on it.

> A test for this would be nice, if bzr-upload has enough test framework
> to support it.

I believe it does; I'm looking at the test suite right now and trying to figure out how to add tests for symlink support in. I'm not really used to working with other people's code, let alone test suites.

[Martin Pool (mbp)]

On 17 February 2010 16:32, Neil Santos <email address hidden> wrote:
>> That patch looks basically reasonable. You should put up a branch and
> propose a merge.
>
> I did that, except for the merge proposal.  I'm not comfortable asking
> the devs to merge something this hacky, so I'll try to work on it a bit
> more.  Luckily, I need bzr-upload to support symlinks for non-local
> transports, as well, so I can pretty well justify working on this.
>
>> I don't see why you can't do it on non-local transports as long as
>> they support symlinks? Similarly there's no reason why it should
>> matter whether the local os supports them or not. Instead just try it
>> and check for TransportNotPossible.
>
> My only objection is that I don't know how to do just that (not yet, in
> any case).  My Google-fu seems to be amiss when trying to figure out how
> bzrlib works, especially the Transport classes-- I would appreciate
> pointers towards the right direction.

There is some description in http://doc.bazaar.canonical.com/bzr.dev/developers/

However I'm sure there are good questions it doesn't answer, and we
would appreciate them being pointed out.

> At first, I thought I should give the Transport classes the ability to
> do symlinks by themselves, since, as I implied earlier, they don't seem
> to know how to do just that.  I still think this would be the best way
> to handle this, but I know so little about bzrlib and bzr-upload that I
> won't bet on it.

Yes, we should. In fact I thought they already did but I was totally wrong.

To do that:

1. add a method in the base Transport class that raises TransportNotPossible
2. implement it in LocalTransport by calling os.symlink
3. add a test in per_transport that tries to call it and if it
succeeds stats the file and checks its a symlink
4. implement in SftpTransport
5. probably add readlink() too

>
>> A test for this would be nice, if bzr-upload has enough test framework
>> to support it.
>
> I believe it does; I'm looking at the test suite right now and trying to
> figure out how to add tests for symlink support in.  I'm not really used
> to working with other people's code, let alone test suites.

Feel free to ask, we try to help

--
Martin <http://launchpad.net/~mbp/>

[Alexander Menk (alex-menk)]

I understand that it is not easy to support upload of symlinks --- but until this is implemented (or if it might not be implemented ever), this error should be handled properly.

Especially this should only be a warning and not a error (i.e. the upload should continue even if there is a symlink)

Such as a final message:

 The following symlinks were not be uploaded --- you might have to create them on your own:

   1. foo ---> /usr/share/foo
   2. bar.conf ---> /etc/bar.conf
   3. ....

[Neil Santos (rls)]

I've put up a branch of the bzr mainline with changes in the Transport classes to try and support creating hard- and symlinks in bzr itself. I've also put up a merge proposal.

The branch still needs a *lot* of work, though, so it might be a while before it gets into the mainline, if at all. I'll be updating my bzr-upload branch with changes to follow suit, as appropriate.

[Neil Santos (rls)]

The fix I'm working on depends on the patch I submitted, which was included in bzr 2.2b1, but I'm going to try and work in some sort of backwards compatibility. Please let me know if you're stuck using an even older version (I'm targeting 2.0.3), so I could try to make it work there somehow (in case the aforementioned attempts at backwards compat work).