Merge lp:~lool/ubuntuone-client/tritcask-use-platform-api into lp:ubuntuone-client

[Loïc Minier]

Use platform API for os.path.* functions; fixes LP #1101344.

Use platform API for os.path.* functions; fixes bug #1101344.

[Loïc Minier (lool)]

Pushed some more revisions; sorry, there's some history garbage: r1387 to r1390 were attempts to fix the SetFileSecurity issue:
* it's a deprecated function, so I tried using the new SetNamedSecurityInfo function
* I also tried matching permissions with files on my system which have an ACL entry for sysops too
* I dropped some unused function (_get_group_sid)

but I didn't want these revs to end up in this mp, and I forgot to revert my tree to the right revno before committing the other fixes, so r1391 has these changed reverted.

Let me know if you'd like some of them in another mp, but they don't fix the orginal bug so I'm not adding them here.

[Loïc Minier (lool)]

I pushed a last rev 1397 which is untested; I was hoping using normpath would fix all issues without having to call into the os_helper API for everything, but it didn't help after all, so I'm reverting the normpath changes.

I didn't actually test tritcask.py after reverting though.

[dobey (dobey)]

44 +is_dir = unit.is_dir

Typo here.

Have you not done ./autogen.sh && make check in the tree?

[Loïc Minier (lool)]

Sorry I hadn't (I thought I had noted that, but apparently I forgot)

I've pushed r1398 which fixes this particular issue, and then make check exposed another issue which I've fixed in r1399; this should hopefully be good to merge now. NB: some tests were actually skipped, not sure why:
PASSED (skips=12, successes=2759)

Also didn't run some part of the tests it seems:
ubuntuone.devtools.errors.TestError: The Python package providing the requested reactor is not installed. You can find it here: https://github.com/ghtdak/qtreactor

[ Would you mind adding some advice in HACKING on the packages that one should install? I had to install:
  xutils-dev gobject-introspection python-mocker
to run ./autogen.sh && make check, and I can see that other bits like gnome-common are needed. ]

[dobey (dobey)]

You can use "apt-get build-dep ubuntuone-client" for the most part. python-qt4reactor is packaged, but not in main, so it's not in the build-depends of ubuntuone-client in the main archive. But adding the nightlies ppa (ppa:ubuntuone/nightlies) and then apt-get update && apt-get build-dep ubuntuone-client will pull everything needed.

Also, can you please do a "bzr commit --fixes=lp:1101344 --unchanged" to link the bug into the branch? Thanks.

[Loïc Minier (lool)]

Done in r1400.

[dobey (dobey)]

review: Approve

[Mike McCracken (mikemc)]

looks good. tests pass on darwin.

Thanks Loïc!

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

There was a problem validating some authors of the branch. Authors must be either one of the listed Launchpad users, or a member of one of the listed teams on Launchpad.

Persons or Teams:

    contributor-agreement-canonical
    ubuntuone-hackers

Unaccepted Authors:

    Loïc Minier <email address hidden>

=== modified file 'ubuntuone/platform/__init__.py'
--- ubuntuone/platform/__init__.py	2012-09-25 22:51:42 +0000
+++ ubuntuone/platform/__init__.py	2013-02-13 18:06:22 +0000
@@ -76,6 +76,7 @@
 allow_writes = os_helper.allow_writes
 can_write = os_helper.can_write
 get_path_list = os_helper.get_path_list
+is_dir = os_helper.is_dir
 is_link = os_helper.is_link
 is_root = os_helper.is_root
 listdir = os_helper.listdir

=== modified file 'ubuntuone/platform/os_helper/__init__.py'
--- ubuntuone/platform/os_helper/__init__.py	2012-08-07 13:29:42 +0000
+++ ubuntuone/platform/os_helper/__init__.py	2013-02-13 18:06:22 +0000
@@ -51,6 +51,7 @@
 remove_tree = source.remove_tree
 remove_dir = source.remove_dir
 path_exists = source.path_exists
+is_dir = source.is_dir
 make_dir = source.make_dir
 open_file = source.open_file
 rename = source.rename

=== modified file 'ubuntuone/platform/os_helper/darwin.py'
--- ubuntuone/platform/os_helper/darwin.py	2012-08-01 11:34:37 +0000
+++ ubuntuone/platform/os_helper/darwin.py	2013-02-13 18:06:22 +0000
@@ -56,6 +56,7 @@
 remove_tree = unix.remove_tree
 remove_dir = unix.remove_dir
 path_exists = unix.path_exists
+is_dir = unix.is_dir
 make_dir = unix.make_dir
 open_file = unix.open_file
 rename = unix.rename

=== modified file 'ubuntuone/platform/os_helper/linux.py'
--- ubuntuone/platform/os_helper/linux.py	2013-01-28 20:17:45 +0000
+++ ubuntuone/platform/os_helper/linux.py	2013-02-13 18:06:22 +0000
@@ -101,6 +101,7 @@
 remove_tree = unix.remove_tree
 remove_dir = unix.remove_dir
 path_exists = unix.path_exists
+is_dir = unix.is_dir
 make_dir = unix.make_dir
 open_file = unix.open_file
 rename = unix.rename

=== modified file 'ubuntuone/platform/os_helper/unix.py'
--- ubuntuone/platform/os_helper/unix.py	2012-08-07 13:29:42 +0000
+++ ubuntuone/platform/os_helper/unix.py	2013-02-13 18:06:22 +0000
@@ -98,6 +98,11 @@
     return os.path.exists(path)
 
 
+def is_dir(path):
+    """Return if the path is an existing directory."""
+    return os.path.isdir(path)
+
+
 def make_dir(path, recursive=False):
     """Make a dir, optionally creating all the middle ones."""
     if recursive:

=== modified file 'ubuntuone/platform/os_helper/windows.py'
--- ubuntuone/platform/os_helper/windows.py	2012-08-07 13:29:42 +0000
+++ ubuntuone/platform/os_helper/windows.py	2013-02-13 18:06:22 +0000
@@ -591,6 +591,12 @@
 
 
 @windowspath()
+def is_dir(path):
+    """Return if the path is an existing directory."""
+    return os.path.isdir(path)
+
+
+@windowspath()
 def make_dir(path, recursive=False):
     """Make a dir, optionally creating all the middle ones."""
     if recursive:

=== modified file 'ubuntuone/syncdaemon/tritcask.py'
--- ubuntuone/syncdaemon/tritcask.py	2013-01-25 20:38:01 +0000
+++ ubuntuone/syncdaemon/tritcask.py	2013-02-13 18:06:22 +0000
@@ -49,6 +49,17 @@
 from collections import namedtuple
 from UserDict import DictMixin
 
+from ubuntuone.platform import (
+    is_dir,
+    listdir,
+    make_dir,
+    open_file,
+    path_exists,
+    remove_file,
+    rename,
+    stat_path,
+)
+
 
 crc32_fmt = '>i'
 crc32_size = struct.calcsize(crc32_fmt)
@@ -200,7 +211,7 @@
 
     @property
     def size(self):
-        return os.stat(self.filename).st_size
+        return stat_path(self.filename).st_size
 
     @classmethod
     def _get_next_file_id(cls):
@@ -216,30 +227,30 @@
     @property
     def has_hint(self):
         """Return true if there is a hint file on-disk."""
-        return os.path.exists(self.hint_filename)
+        return path_exists(self.hint_filename)
 
     @property
     def hint_size(self):
         """Return the hint file size."""
         if self.has_hint:
-            return os.stat(self.hint_filename).st_size
+            return stat_path(self.hint_filename).st_size
         return 0
 
     def exists(self):
-        return os.path.exists(self.filename)
+        return path_exists(self.filename)
 
     def make_immutable(self):
         """Make this data file immutable."""
         self.close()
         new_name = self.filename.replace(LIVE, INACTIVE)
-        os.rename(self.filename, new_name)
+        rename(self.filename, new_name)
         return ImmutableDataFile(*os.path.split(new_name))
 
     def _open(self):
-        if not os.path.exists(self.filename):
+        if not path_exists(self.filename):
             # create the file and open it for append
-            open(self.filename, 'wb').close()
-        self.fd = open(self.filename, 'a+b')
+            open_file(self.filename, 'wb').close()
+        self.fd = open_file(self.filename, 'a+b')
 
     def close(self):
         """Close the file descriptor"""
@@ -361,16 +372,16 @@
         new_name = self.filename.replace(INACTIVE, DEAD)
         new_hint_name = self.hint_filename.replace(INACTIVE, DEAD)
         self.close()
-        os.rename(self.filename, new_name)
+        rename(self.filename, new_name)
         self.filename = new_name
         if self.has_hint:
-            os.rename(self.hint_filename, new_hint_name)
+            rename(self.hint_filename, new_hint_name)
             self.hint_filename = new_hint_name
         self._open()
         return self
 
     def _open(self):
-        self.fd = open(self.filename, 'rb')
+        self.fd = open_file(self.filename, 'rb')
         fmmap = mmap.mmap(self.fd.fileno(), 0, access=mmap.ACCESS_READ)
         self.fmmap = fmmap
 
@@ -410,9 +421,9 @@
 
     def delete(self):
         """Delete this file and the hint if exists."""
-        os.unlink(self.filename)
+        remove_file(self.filename)
         if self.has_hint:
-            os.unlink(self.hint_filename)
+            remove_file(self.hint_filename)
 
     # make all inherited methods to fail
     def _not_implemented(self, *args, **kwargs):
@@ -436,19 +447,19 @@
         """Make this data file immutable."""
         self.close()
         new_name = self.filename.replace(self.temp_name, INACTIVE)
-        os.rename(self.filename, new_name)
+        rename(self.filename, new_name)
         if self.has_hint:
             new_hint_name = self.hint_filename.replace(self.temp_name,
                 INACTIVE)
-            os.rename(self.hint_filename, new_hint_name)
+            rename(self.hint_filename, new_hint_name)
         return ImmutableDataFile(*os.path.split(new_name))
 
     def delete(self):
         """Delete this file and the hint if exists."""
         self.close()
-        os.unlink(self.filename)
+        remove_file(self.filename)
         if self.has_hint:
-            os.unlink(self.hint_filename)
+            remove_file(self.hint_filename)
 
 
 class HintFile(object):
@@ -458,13 +469,13 @@
         """Create the instance."""
         self.path = path
         self.tempfile = None
-        if os.path.exists(self.path) and os.stat(self.path).st_size > 0:
+        if path_exists(self.path) and stat_path(self.path).st_size > 0:
             # if it's there and size > 0, open only for read
-            self.fd = open(self.path, 'rb')
+            self.fd = open_file(self.path, 'rb')
         else:
             # this is a new hint file, lets create it as a tempfile.
             self.tempfile = tempfile.mktemp(dir=os.path.dirname(self.path))
-            self.fd = open(self.tempfile, 'w+b')
+            self.fd = open_file(self.tempfile, 'w+b')
 
     def iter_entries(self):
         """Return a generator over the hint entries."""
@@ -508,7 +519,7 @@
             self.fd.close()
         # if this is a new hint file, rename it to the real path.
         if self.tempfile:
-            os.rename(self.tempfile, self.path)
+            rename(self.tempfile, self.path)
 
 
 class Keydir(dict):
@@ -627,9 +638,9 @@
         self.dead_bytes_threshold = dead_bytes_threshold
         self.max_immutable_files = max_immutable_files
         self.auto_merge = auto_merge
-        if not os.path.exists(self.base_path):
-            os.makedirs(self.base_path)
-        elif not os.path.isdir(self.base_path):
+        if not path_exists(self.base_path):
+            make_dir(self.base_path, recursive=True)
+        elif not is_dir(self.base_path):
             raise ValueError('path must be a directory.')
         self.live_file = None
         self._immutable = {}
@@ -719,7 +730,7 @@
         base_path = self.base_path
         if not base_path.endswith(os.path.sep):
             base_path += os.path.sep
-        files = os.listdir(self.base_path)
+        files = listdir(self.base_path)
         for filename in files:
             # first check for hint and dead
             if is_hint(filename):
@@ -748,7 +759,7 @@
                 logger.warning("Failed to open %s, renaming it to: %s - "
                                "error: %s", orig, dest, e)
                 # rename it to "broken"
-                os.rename(orig, dest)
+                rename(orig, dest)
         # immutable files + live
         logger.info("found %s data files, %s dead and %s broken files",
                     len(self._immutable) + 1, dead_files, broken_files)