Merge lp:~nataliabidart/ubuntuone-client/xdg-return-bytes into lp:ubuntuone-client

[Natalia Bidart]

- XDG data dirs must return bytes (LP: #818030).

[Guillermo Gonzalez (verterok)]

looks good.

review: Approve

[lp:~nataliabidart/ubuntuone-client/xdg-return-bytes]

1080. By Natalia Bidart on 2011-07-29

Typo!

[Alejandro J. Cura (alecu)]

Waiting for a few style changes, but basically "Approved!!!!!"

review: Approve

[lp:~nataliabidart/ubuntuone-client/xdg-return-bytes]

1081. By Natalia Bidart on 2011-07-29

Merged trunk in.

1082. By Natalia Bidart on 2011-07-29

Style fixes.

=== modified file 'bin/ubuntuone-syncdaemon'
--- bin/ubuntuone-syncdaemon	2011-07-28 14:36:35 +0000
+++ bin/ubuntuone-syncdaemon	2011-07-29 19:11:49 +0000
@@ -37,7 +37,15 @@
 import sys
 import shutil
 
-from ubuntuone.platform import is_already_running, set_application_name, is_root
+from ubuntuone.platform import (
+    can_write,
+    set_dir_readwrite,
+    is_already_running,
+    is_root,
+    make_dir,
+    path_exists,
+    set_application_name,
+)
 from ubuntuone.syncdaemon import logger, config
 from ubuntuone.syncdaemon.config import (
     get_config_files,
@@ -127,31 +135,36 @@
 
     # check if we are using xdg_data_home and it doesn't exists
     if xdg_data_home in options.data_dir and \
-       not os.path.exists(options.data_dir):
+       not path_exists(options.data_dir):
         # if we have metadata in the old xdg_cache, move it!
         old_data_dir = options.data_dir.replace(xdg_data_home, xdg_cache_home)
-        if os.path.exists(old_data_dir):
+        if path_exists(old_data_dir):
             parent = os.path.dirname(options.data_dir)
-            if os.path.exists(parent) and not os.access(parent, os.W_OK):
+            if path_exists(parent) and not can_write(parent):
                 # make the parent dir writable
-                os.chmod(parent, 0775)
-            elif not os.path.exists(parent):
+                set_dir_readwrite(parent)
+            elif not path_exists(parent):
                 # if it don't exits
-                os.makedirs(parent)
+                make_dir(parent, recursive=True)
             shutil.move(old_data_dir, options.data_dir)
-    if not os.path.exists(options.data_dir):
+    if not path_exists(options.data_dir):
         parent = os.path.dirname(options.data_dir)
-        if os.path.exists(parent) and not os.access(parent, os.W_OK):
+        if path_exists(parent) and not can_write(parent):
             # make the parent dir writable
-            os.chmod(parent, 0775)
-        os.makedirs(options.data_dir)
+            set_dir_readwrite(parent)
+        make_dir(options.data_dir, recursive=True)
 
     # create the partials_dir
     partials_dir = os.path.join(xdg_cache_home, 'ubuntuone', 'partials')
-    if not os.path.exists(partials_dir):
-        os.makedirs(partials_dir)
+    if not path_exists(partials_dir):
+        make_dir(partials_dir, recursive=True)
 
     logger.rotate_logs()
+
+    assert isinstance(options.root_dir, str)
+    assert isinstance(options.shares_dir, str)
+    assert isinstance(options.data_dir, str)
+
     main = Main(options.root_dir, options.shares_dir, options.data_dir,
                 partials_dir, host=options.host, port=int(options.port),
                 dns_srv=options.dns_srv, ssl=True,

=== modified file 'tests/platform/test_xdg_base_directory.py'
--- tests/platform/test_xdg_base_directory.py	2011-07-26 16:07:08 +0000
+++ tests/platform/test_xdg_base_directory.py	2011-07-29 19:11:49 +0000
@@ -34,3 +34,13 @@
                                 'ubuntuone', 'log')
         self.assertEqual(expected, xdg_base_directory.ubuntuone_log_dir)
         self.assertTrue(os.path.exists(expected))
+
+    def test_xdg_cache_home_is_bytes(self):
+        """The returned path is bytes."""
+        actual = xdg_base_directory.xdg_cache_home
+        self.assertIsInstance(actual, str)
+
+    def test_xdg_data_home_is_bytes(self):
+        """The returned path is bytes."""
+        actual = xdg_base_directory.xdg_data_home
+        self.assertIsInstance(actual, str)

=== modified file 'tests/platform/windows/test_xdg_base_directory.py'
--- tests/platform/windows/test_xdg_base_directory.py	2011-07-28 12:51:07 +0000
+++ tests/platform/windows/test_xdg_base_directory.py	2011-07-29 19:11:49 +0000
@@ -17,12 +17,11 @@
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 """Platform independent tests for the credentials management."""
 
-import os
-
 import win32com.shell
 
 from twisted.trial.unittest import TestCase
 
+from ubuntuone.platform.windows.os_helper import assert_syncdaemon_path
 from ubuntuone.platform.xdg_base_directory.windows import get_special_folders
 
 
@@ -38,8 +37,8 @@
     def __init__(self):
         """Set the proper mapping between CSIDL_ consts."""
         self.values = {
-            0: r'c:\path\to\users\home',
-            1: r'c:\path\to\users\home\appData\local'}
+            0: u'c:\\path\\to\\users\\home',
+            1: u'c:\\path\\to\\users\\home\\appData\\local'}
 
     def SHGetFolderPath(self, dummy0, shellconValue, dummy2, dummy3):
         """Override SHGetFolderPath functionality."""
@@ -67,5 +66,5 @@
         self.assertTrue(special_folders['Local AppData'].startswith(
             special_folders['AppData']))
 
-        for k in special_folders:
-            os.access(special_folders[k], os.W_OK)
+        for val in special_folders.itervalues():
+            assert_syncdaemon_path(val)

=== modified file 'tests/syncdaemon/test_config.py'
--- tests/syncdaemon/test_config.py	2011-07-27 13:27:22 +0000
+++ tests/syncdaemon/test_config.py	2011-07-29 19:11:49 +0000
@@ -471,7 +471,7 @@
 
 
 class ConfigglueParsersTests(BaseTwistedTestCase):
-    """Tests for our custom configglue parsers"""
+    """Tests for our custom configglue parsers."""
 
     def test_throttling_limit_parser(self):
         """Test throttling_limit_parser"""
@@ -497,39 +497,55 @@
         self.assertEqual(logging.DEBUG, parser(bad_value))
         self.assertEqual(logging.DEBUG, parser(invalid_value))
 
-    def test_home_dir_parser(self):
-        """Test home_dir_parser"""
-        good_value = '~/hola'
-        bad_value = 'hola'
-        invalid_value = None
-        parser = config.home_dir_parser
-        homedir = os.path.join('/', 'home', 'fake')
+
+class XdgHomeParsersTests(BaseTwistedTestCase):
+    """Tests for our custom xdg parsers."""
+
+    good_value = '~/hola/mundo'
+    name = 'home'
+    xdg_dir = os.path.join('', 'home', 'fake')
+
+    @defer.inlineCallbacks
+    def setUp(self):
+        yield super(XdgHomeParsersTests, self).setUp()
+        self.parser = getattr(config, '%s_dir_parser' % self.name)
+
+    def test_good_value(self):
+        """Test the parser using a good value."""
+        homedir = os.path.join('', 'home', 'fake')
         with environ('HOME', homedir):
-            self.assertEqual(os.path.join(homedir, 'hola'), parser(good_value))
-        self.assertEqual('hola', parser(bad_value))
-        self.assertRaises(AttributeError, parser, invalid_value)
-
-    def test_xdg_cache_dir_parser(self):
-        """Test xdg_cache_dir_parser"""
-        good_value = 'hola'
-        bad_value = '/hola'
-        invalid_value = None
-        parser = config.xdg_cache_dir_parser
-        self.assertEquals(os.path.join(xdg_cache_home, 'hola'),
-                          parser(good_value))
-        self.assertEquals('/hola', parser(bad_value))
-        self.assertRaises(AttributeError, parser, invalid_value)
-
-    def test_xdg_data_dir_parser(self):
-        """Test xdg_data_dir_parser"""
-        good_value = 'hola'
-        bad_value = '/hola'
-        invalid_value = None
-        parser = config.xdg_data_dir_parser
-        self.assertEquals(os.path.join(xdg_data_home, 'hola'),
-                          parser(good_value))
-        self.assertEquals('/hola', parser(bad_value))
-        self.assertRaises(AttributeError, parser, invalid_value)
+            expected = os.path.join(self.xdg_dir, 'hola', 'mundo')
+            actual = self.parser(self.good_value)
+            self.assertEqual(expected, actual)
+            self.assertIsInstance(actual, str)
+            self.assertNotIsInstance(actual, unicode)
+
+    def test_bad_value(self):
+        """Test the parser using a bad value."""
+        bad_value = '/hola'
+        self.assertEqual(config.path_from_unix(bad_value),
+                         self.parser(bad_value))
+
+    def test_invalid_value(self):
+        """Test the parser using an invalid value."""
+        invalid_value = None
+        self.assertRaises(AttributeError, self.parser, invalid_value)
+
+
+class XdgCacheParsersTests(XdgHomeParsersTests):
+    """Tests for our custom xdg parsers."""
+
+    good_value = 'hola/mundo'
+    name = 'xdg_cache'
+    xdg_dir = xdg_cache_home
+
+
+class XdgDataParsersTests(XdgCacheParsersTests):
+    """Tests for our custom xdg parsers."""
+
+    good_value = 'hola/mundo'
+    name = 'xdg_data'
+    xdg_dir = xdg_data_home
 
 
 class SyncDaemonConfigParserTests(BaseTwistedTestCase):

=== modified file 'ubuntuone/platform/linux/__init__.py'
--- ubuntuone/platform/linux/__init__.py	2011-07-28 17:32:56 +0000
+++ ubuntuone/platform/linux/__init__.py	2011-07-29 19:11:49 +0000
@@ -52,13 +52,13 @@
     set_file_readwrite,
     set_no_rights,
     stat_path,
-    validate_path_from_unix,
 )
 from ubuntuone.platform.linux.credentials import CredentialsManagementTool
 from ubuntuone.platform.linux.logger import setup_filesystem_logging, get_filesystem_logger
 from ubuntuone.platform.linux.filesystem_notifications import FilesystemMonitor
 from ubuntuone.platform.linux.notification import Notification
 
+
 class ExternalInterface(object):
     """An ExternalInterface implemented with a DBus interface."""
 

=== modified file 'ubuntuone/platform/linux/os_helper.py'
--- ubuntuone/platform/linux/os_helper.py	2011-07-19 21:59:27 +0000
+++ ubuntuone/platform/linux/os_helper.py	2011-07-29 19:11:49 +0000
@@ -154,12 +154,7 @@
 
 def is_root():
     """Return if the user is running as root."""
-    return  not os.geteuid()
-
-
-def validate_path_from_unix(path):
-    """Get a unix path and return it so that it cna be used."""
-    return path
+    return not os.geteuid()
 
 
 def get_path_list(path):

=== modified file 'ubuntuone/platform/windows/__init__.py'
--- ubuntuone/platform/windows/__init__.py	2011-07-28 17:32:56 +0000
+++ ubuntuone/platform/windows/__init__.py	2011-07-29 19:11:49 +0000
@@ -51,7 +51,6 @@
     set_file_readwrite,
     set_no_rights,
     stat_path,
-    validate_path_from_unix,
 )
 from ubuntuone.platform.windows.credentials import CredentialsManagementTool
 from ubuntuone.platform.windows import event_logging

=== modified file 'ubuntuone/platform/windows/os_helper.py'
--- ubuntuone/platform/windows/os_helper.py	2011-07-28 15:43:05 +0000
+++ ubuntuone/platform/windows/os_helper.py	2011-07-29 19:11:49 +0000
@@ -716,12 +716,6 @@
     return False
 
 
-def validate_path_from_unix(path):
-    """Get a unix path and return it so that it can be used."""
-    path = path.replace('/', '\\')
-    return path
-
-
 @windowspath()
 def get_path_list(path):
     """Return a list with the diff components of the path."""

=== modified file 'ubuntuone/platform/xdg_base_directory/windows.py'
--- ubuntuone/platform/xdg_base_directory/windows.py	2011-07-27 21:09:30 +0000
+++ ubuntuone/platform/xdg_base_directory/windows.py	2011-07-29 19:11:49 +0000
@@ -31,11 +31,12 @@
     # CSIDL_LOCAL_APPDATA = C:\Users\<username>\AppData\Local
     # CSIDL_PROFILE = C:\Users\<username>
     path = shell.SHGetFolderPath(0, shellcon.CSIDL_PROFILE, None, 0)
-    special_folders['Personal'] = path
+    special_folders['Personal'] = path.encode('utf8')
 
     path = shell.SHGetFolderPath(0, shellcon.CSIDL_LOCAL_APPDATA, None, 0)
-    special_folders['Local AppData'] = path
-    special_folders['AppData'] = os.path.dirname(path)
+    special_folders['Local AppData'] = path.encode('utf8')
+    path = os.path.dirname(path)
+    special_folders['AppData'] = path.encode('utf8')
 
     return special_folders
 

=== modified file 'ubuntuone/syncdaemon/config.py'
--- ubuntuone/syncdaemon/config.py	2011-07-28 15:43:05 +0000
+++ ubuntuone/syncdaemon/config.py	2011-07-29 19:11:49 +0000
@@ -25,6 +25,7 @@
 
 from ConfigParser import NoOptionError, NoSectionError
 from optparse import OptionParser
+from ubuntuone.platform import path_exists, rename
 from ubuntuone.platform.xdg_base_directory import (
     load_config_paths,
     save_config_path,
@@ -56,7 +57,6 @@
     TypedConfigParser = new_tcp
     del new_tcp
 # end of naming shenanigans
-from ubuntuone.platform import validate_path_from_unix
 
 CONFIG_FILE = 'syncdaemon.conf'
 CONFIG_LOGS = 'logging.conf'
@@ -76,6 +76,8 @@
 # this object is the shared config
 _user_config = None
 
+path_from_unix = lambda path: path.replace('/', os.path.sep)
+
 
 def home_dir_parser(value):
     """Parser for the root_dir and shares_dir options.
@@ -83,21 +85,31 @@
     Return the path using user home + value.
 
     """
-    return os.path.expanduser(validate_path_from_unix(value))
+    result = os.path.expanduser(path_from_unix(value))
+    assert isinstance(result, str)
+    return result
 
 
 def xdg_cache_dir_parser(value):
-    """ Parser for the data_dir option.
-    returns the path using xdg_cache_home + value.
+    """Parser for the data_dir option.
+
+    Return the path using xdg_cache_home + value.
+
     """
-    return os.path.join(xdg_cache_home, validate_path_from_unix(value))
+    result = os.path.join(xdg_cache_home, path_from_unix(value))
+    assert isinstance(result, str)
+    return result
 
 
 def xdg_data_dir_parser(value):
-    """ Parser for the data_dir option.
-    returns the path using xdg_data_home + value.
+    """Parser for the data_dir option.
+
+    Return the path using xdg_data_home + value.
+
     """
-    return os.path.join(xdg_data_home, validate_path_from_unix(value))
+    result = os.path.join(xdg_data_home, path_from_unix(value))
+    assert isinstance(result, str)
+    return result
 
 
 def log_level_parser(value):
@@ -140,11 +152,11 @@
     config_files = []
     for xdg_config_dir in load_config_paths('ubuntuone'):
         config_file = os.path.join(xdg_config_dir, CONFIG_FILE)
-        if os.path.exists(config_file):
+        if path_exists(config_file):
             config_files.append(config_file)
 
         config_logs = os.path.join(xdg_config_dir, CONFIG_LOGS)
-        if os.path.exists(config_logs):
+        if path_exists(config_logs):
             config_files.append(config_logs)
 
     # reverse the list as load_config_paths returns the user dir first
@@ -152,11 +164,11 @@
     # if we are running from a branch, get the config files from it too
     config_file = os.path.join(os.path.dirname(__file__), os.path.pardir,
                                os.path.pardir, 'data', CONFIG_FILE)
-    if os.path.exists(config_file):
+    if path_exists(config_file):
         config_files.append(config_file)
 
     config_logs = os.path.join('data', CONFIG_LOGS)
-    if os.path.exists(config_logs):
+    if path_exists(config_logs):
         config_files.append(config_logs)
 
     return config_files
@@ -279,11 +291,11 @@
         for section in [MAIN, THROTTLING, NOTIFICATIONS]:
             if self.has_section(section) and not self.options(section):
                 self.remove_section(section)
-        with open(self.config_file+'.new', 'w') as fp:
+        with open(self.config_file + '.new', 'w') as fp:
             self.write(fp)
-        if os.path.exists(self.config_file):
-            os.rename(self.config_file, self.config_file+'.old')
-        os.rename(self.config_file+'.new', self.config_file)
+        if path_exists(self.config_file):
+            rename(self.config_file, self.config_file + '.old')
+        rename(self.config_file + '.new', self.config_file)
 
     def get_parsed(self, section, option):
         """get that fallbacks to our custom defaults"""

=== modified file 'ubuntuone/syncdaemon/main.py'
--- ubuntuone/syncdaemon/main.py	2011-06-23 13:10:19 +0000
+++ ubuntuone/syncdaemon/main.py	2011-07-29 19:11:49 +0000
@@ -98,6 +98,9 @@
 
         self.logger.info("Starting Ubuntu One client version %s",
                          clientdefs.VERSION)
+        self.logger.info("Using %r as root dir", self.root_dir)
+        self.logger.info("Using %r as data dir", self.data_dir)
+        self.logger.info("Using %r as shares root dir", self.shares_dir)
         self.db = tritcask.Tritcask(self.tritcask_dir)
         self.vm = volume_manager.VolumeManager(self)
         self.fs = filesystem_manager.FileSystemManager(
@@ -135,9 +138,6 @@
         self.eventlog_listener = None
         self.start_event_logger()
         self.start_status_listener()
-        self.logger.info("Using %r as root dir", self.root_dir)
-        self.logger.info("Using %r as data dir", self.data_dir)
-        self.logger.info("Using %r as shares root dir", self.shares_dir)
         self.mark = task.LoopingCall(self.log_mark)
         self.mark.start(mark_interval)