Merge lp:~thomir-deactivatedaccount/bzr/add-global-config into lp:bzr

[Thomi Richards]

Code not finished yet - working on making a system-wide config file (/etc/bazaar/bazaar.conf under Linux).

[Thomi Richards (thomir-deactivatedaccount)]

Things I know still need to be done:
1) to consider: rename GlobalStore to UserStore, and SystemGlobalStore to GlobalStore. no one outside bzrlib.config and associated unit tests use this class directly, so the rename should be trivial.

2) Test new windows path function.

3) System-wide config file path for Mac OS X? Haven't found any portable way to work out where this should go. Do macs have /etc/?

[lp:~thomir-deactivatedaccount/bzr/add-global-config]

6048. By Thomi Richards on 2011-07-28

Fixed docstring for config.system_config_dir

[Matthew Fuller (fullermd)]

> (/etc/bazaar/bazaar.conf under Linux).

On BSD, there's going to be a strong preference for /usr/local/etc/.
This probably calls for it being settable somehow in the install
process at least...

[Thomi Richards (thomir-deactivatedaccount)]

> On BSD, there's going to be a strong preference for /usr/local/etc/.
> This probably calls for it being settable somehow in the install
> process at least...

Okay, that's fine. What's the best way to do this?

[Martin Pool (mbp)]

On 29 July 2011 04:52, Matthew Fuller <email address hidden> wrote:
>> (/etc/bazaar/bazaar.conf under Linux).
>
> On BSD, there's going to be a strong preference for /usr/local/etc/.
> This probably calls for it being settable somehow in the install
> process at least...

If you use distutils.sysconfig or similar, it should use whatever
prefix Python was compiled with.

[lp:~thomir-deactivatedaccount/bzr/add-global-config]

6049. By Thomi Richards on 2011-07-30

Added unit tests for new method in win32utils.py

6050. By Thomi Richards on 2011-07-30

Added unit tests to test the new path functions for win32 and freebsd.

[Martin Pool (mbp)]

Thanks for this, Thomi.

+ elif sys.platform.startswith('freebsd'):
+ return osutils.pathjoin(sysconfig.PREFIX, 'etc', 'bazaar')
+ else:
+ return osutils.pathjoin('/etc', "bazaar")

Ideally we would get the whole path to '...etc' from Python, rather than special-casing on the platform.

But I don't actually see anything that points to /etc in sysconfig.get_config_vars() so we might need to either find some other module that tells us where it is, or just hardcode it, in which case I suppose your patch is the best thing.

The docstring also needs to be updated.

[Thomi Richards (thomir-deactivatedaccount)]

Hi Martin,

Thanks for your feedback - I'll update the docstring and try and find some programmatic way to get the /etc/ path.

I also need to work out if my unit tests pass under Windows. Is there a CI server that can run these tests for me?

[Martin Pool (mbp)]

+class SystemGlobalStore(LockableIniFileStore):
+
+ def __init__(self, possible_transports=None):
+ t = transport.get_transport_from_path(system_config_dir(),
+ possible_transports=possible_transports)
+ super(SystemGlobalStore, self).__init__(t, 'bazaar.conf')

The point of passing possible_transports is basically so that we can reuse some connection objects, and that's mostly important for remote transports, which this won't be at present. I think the current approach of manually passing it down is not working very well and we should probably look at putting it into the library state instead.

[tweak] However, for this patch, I don't think it's either used or particularly useful (since making new local transports is cheap), so you could just remove the parameter.

[suggestion] I'm not a big fan of creating new classes that only have different initial state, not different behaviour, so I would probably just make this a factory method.

Aside from that, this looks great. It could be good to get a review from vila too.

[Martin Pool (mbp)]

On 2 August 2011 12:51, Thomi Richards <email address hidden> wrote:
> Hi Martin,
>
> Thanks for your feedback - I'll update the docstring and try and find some programmatic way to get the /etc/ path.
>
> I also need to work out if my unit tests pass under Windows. Is there a CI server that can run these tests for me?

There is Babune <http://babune.ladeuil.net:24842/> which will catch
any post-landing regressions, but I don't think it is yet able to test
arbitrary unlanded branches.

I have Wine, Python installed under that, and then just a script
'winepython' that does

exec wine ~/.wine/drive_c/Python26/python.exe "$@"

and then I can say 'winepython ./bzr selftest config' etc.

[Thomi Richards (thomir-deactivatedaccount)]

> +class SystemGlobalStore(LockableIniFileStore):
> +
> + def __init__(self, possible_transports=None):
> + t = transport.get_transport_from_path(system_config_dir(),
> + possible_transports=possible_transports)
> + super(SystemGlobalStore, self).__init__(t, 'bazaar.conf')
>
> The point of passing possible_transports is basically so that we can reuse
> some connection objects, and that's mostly important for remote transports,
> which this won't be at present. I think the current approach of manually
> passing it down is not working very well and we should probably look at
> putting it into the library state instead.
>
> [tweak] However, for this patch, I don't think it's either used or
> particularly useful (since making new local transports is cheap), so you could
> just remove the parameter.

That sounds sensible. In fact, there's not a single place inside all of bzrlib where a transport is passed in. I've taken the liberty of removing that parameter from all the Store classes. If that was the wrong thing to do, please let me know.

>
> [suggestion] I'm not a big fan of creating new classes that only have
> different initial state, not different behaviour, so I would probably just
> make this a factory method.
>

That also sounds sensible. Since the Stores are an internal API (we want people to be using the Stack classes, right?), I guess I should follow naming convention and call these "_get_global_store" etc?

Also, I've renamed the GlobalStore to UserStore, and the "SystemGlobalStore" to just "GlobalStore", except that after the change above none of these will be visible, since they'll all come out of factory functions.

BranchStore is a bit more difficult, as it uses the branch lock for locking. I could keep the BranchStore class as-is, or I could wrap that up inside a factory function as well, which would make everything look a lot neater, but I'm not sure what your coding standards are regarding nested classes....

Finally, the win32utils unit tests fail, so I have some work to do there...

[lp:~thomir-deactivatedaccount/bzr/add-global-config]

6051. By Thomi Richards on 2011-08-03

Several changes to clean up global config work:
 * Store classes (GlobalStore et al) are now factory functions.
 * What was previously called a "Global store" is now called a "User Store".
 * similarly, what was the "System Global Store" is now just the "Global Store"

6052. By Thomi Richards on 2011-08-03

Forgot to update docstring for system_config_dir()

6053. By Thomi Richards on 2011-08-03

Windows version 6 (WIndows 7, Server 2008) puts the common app data folder in a different place. Updated docstrings and fixed unit test.

[Thomi Richards (thomir-deactivatedaccount)]

Unit tests now pass on Windows 7. I don't have access to anything older. However, it's a little inconvenient that the common appdata folder changes between windows versions. Perhaps a viable alternative would be to put the global app config file in the bzr installation directory under windows?

I'm not sure how that would work with non-standalone installations...

[Martin Pool (mbp)]

> That also sounds sensible. Since the Stores are an internal API (we want people to be using the Stack classes, right?), I guess I should follow naming convention and call these "_get_global_store" etc?
>
> Also, I've renamed the GlobalStore to UserStore, and the "SystemGlobalStore" to just "GlobalStore", except that after the change above none of these will be visible, since they'll all come out of factory functions.
>
> BranchStore is a bit more difficult, as it uses the branch lock for locking. I could keep the BranchStore class as-is, or I could wrap that up inside a factory function as well, which would make everything look a lot neater, but I'm not sure what your coding standards are regarding nested classes....
>
> Finally, the win32utils unit tests fail, so I have some work to do there...

On naming:

We definitely shouldn't change the meaning of an existing name like
GlobalStore, even if it's not intended to be exposed. It's much
better for out-of-date code to get eg a NameError than to run the
wrong thing.

If the class really has actually different behaviour, like
BranchStore, it deserves to be a real different class. But if it's
basically the same class just with different constructor parameters, I
wonder if it's worthwhile. Perhaps for simplicity it is better just
to leave them all as classes. I may be getting a bit bikesheddy here
so don't block on it.

m

[Thomi Richards (thomir-deactivatedaccount)]

Hi Michael,

I'd like to get this right, so I'll probably bug you later this evening if I get time...

> On naming:
>
> We definitely shouldn't change the meaning of an existing name like
> GlobalStore, even if it's not intended to be exposed. It's much
> better for out-of-date code to get eg a NameError than to run the
> wrong thing.
>

The branch as it currently stands will give you that behavior. Nobody should be creating the Store classes directly (nobody is within bzrlib, I don't know about external projects), and now if they try they'll get a NameError.

> If the class really has actually different behaviour, like
> BranchStore, it deserves to be a real different class. But if it's
> basically the same class just with different constructor parameters, I
> wonder if it's worthwhile.
So how about this, for the branch store: replace the BranchStore class with:

def _get_branch_store(branch):
    class BranchStore(IniStore)
        # otherstuff in here from BranchStore class
    return BranchStore(branch)

This has the advantage that: A) it keeps it inline with the other factory functions. B) it hides the different implementation of the BranchStore class, and marks the BranchStore object as being a private thing that other code shouldn't use.

> Perhaps for simplicity it is better just
> to leave them all as classes. I may be getting a bit bikesheddy here
> so don't block on it.
>
Well, I'd like to get it right - I'm preparing to do some more things with sloecode & Bazaar, so learning the bzr code is helpful to me.

What do you think?

[lp:~thomir-deactivatedaccount/bzr/add-global-config]

6054. By Thomi Richards on 2011-08-04

Hid BranchStore class behind _get_branch_store() factory function.

[Vincent Ladeuil (vila)]

Thanks for working on this !

Did you read http://doc.bazaar.canonical.com/devnotes/configuration.html which describes the current implementation, the stack-based implementation and the steps needed to fully deploy it and then the additional features we want to take into account ? This will probably clarifies some of the issues your encountered so far.

> Things I know still need to be done:
> 1) to consider: rename GlobalStore to UserStore, and SystemGlobalStore to
> GlobalStore. no one outside bzrlib.config and associated unit tests use this
> class directly, so the rename should be trivial.

Two remarks:
1 - No one use them because these classes are there to help the transition to stack based configs which is not finished yet.
2 - GlobalStore is targeted at bazaar.conf, UserStore will probably use user.conf and SystemStore may use /etc/bazaar.conf or whatever we decide.

I didn't introduce the system-wide config file because I wanted to minimize the migration issues (section names, section matching, etc), see the Compatibility section in the URL above.

>
> 2) Test new windows path function.
>
> 3) System-wide config file path for Mac OS X? Haven't found any portable way
> to work out where this should go. Do macs have /etc/?

[Vincent Ladeuil (vila)]

>
> > Perhaps for simplicity it is better just
> > to leave them all as classes. I may be getting a bit bikesheddy here
> > so don't block on it.

+1 on leaving them as classes. I think the plan is to use a registry rather than factory methods which is also why I made all the classes use transports whether they were using local files or not.

[Thomi Richards (thomir-deactivatedaccount)]

Hi Vincent,

On 5 August 2011 04:13, Vincent Ladeuil <email address hidden> wrote:
>
> +1 on leaving them as classes. I think the plan is to use a registry rather than factory methods which is also why I made all the classes use transports whether they were using local files or not.
>

I'll revert them back to classes, and re-instate the
possible_transports parameter.

--
Thomi Richards

[Thomi Richards (thomir-deactivatedaccount)]

Hi Vincent,

On 5 August 2011 04:09, Vincent Ladeuil <email address hidden> wrote:
> Did you read http://doc.bazaar.canonical.com/devnotes/configuration.html which describes the current implementation, the stack-based implementation and the steps needed to fully deploy it and then the additional features we want to take into account ? This will probably clarifies some of the issues your encountered so far.

No, I didn't see that. It certainly clears up a lot of questions I had ;)

> 2 - GlobalStore is targeted at bazaar.conf, UserStore will probably use user.conf and SystemStore may use /etc/bazaar.conf or whatever we decide.

...just to be clear, GlobarStore looks at ~/.bazaar/bazaar.conf? So
what does UserStore do?

Cheers,

--
Thomi Richards

[lp:~thomir-deactivatedaccount/bzr/add-global-config]

6055. By Thomi Richards on 2011-08-05

Reverted changes after feedback from Vila.

[Thomi Richards (thomir-deactivatedaccount)]

Hi,

After reading your devnotes it's all a lot clearer.

I think the branch now contains everything as it should be. I'm happy to keep tweaking things, if you can see something that's not quite correct.

Cheers,

[Vincent Ladeuil (vila)]

> No, I didn't see that. It certainly clears up a lot of questions I had ;)

Cool.

>
> > 2 - GlobalStore is targeted at bazaar.conf, UserStore will probably use
> user.conf and SystemStore may use /etc/bazaar.conf or whatever we decide.
>
> ...just to be clear, GlobarStore looks at ~/.bazaar/bazaar.conf?

Yes and is currently defined to accept the [DEFAULT] section and any arbitrary ones used by various plugins and provides no way to match section names as paths.

> So
> what does UserStore do?

UserStore is planned to be associated with ~/.bazaar/user.conf (or defaults.conf) and allows section names to be matched against paths.

Going from one to the other will require some migration path.

[Martin Pool (mbp)]

That hack for /etc is a bit icky, but (modulo just matching all bsds, which I will tweak) I can't think of a cleaner way, and we shouldn't block on it. If someone comes up with a better api or complains that on some other systems it ought to be elsewhere, we can tweak it.

[Martin Pool (mbp)]

So I'm just trying to test this interactively, and it's not obviously hooking in to the things I might like to configure, such as locks.steal_dead, debug_flags, 'config --scope global' etc. Perhaps we should merge it and hook it up later? I would be reluctant to actually document this as a new feature until it's at least mostly working.

[Thomi Richards (thomir-deactivatedaccount)]

> So I'm just trying to test this interactively, and it's not obviously hooking
> in to the things I might like to configure, such as locks.steal_dead,
> debug_flags, 'config --scope global' etc. Perhaps we should merge it and hook
> it up later? I would be reluctant to actually document this as a new feature
> until it's at least mostly working.

The last time I checked the only config option that used the new stack-based API was the "editor" option.

Unfortunately I've been busy with work & real life, but I intend to get back to this at some point in the future. However, I'm not totally clear on what the future direction is, so if you're happy with the code as-is, the best option might be to merge and work from there.

Cheers,

[Jelmer Vernooij (jelmer)]

I'd be interested to hear how this fits in with Vincent's current plans for config, but other than that I think we should just land it.

It seems reasonable enough in general, and we can always tweak it further.

review: Approve

[Vincent Ladeuil (vila)]

Roughly speaking (summarising my previous remarks here): it's a bit early to land as is.

It introduces one more config file that will be accessed for each option query (http://pad.lv/832042).

It introduces a new file with a semantic which is not the one we want (regarding section filtering/ordering http://pad.lv/832046).

It doesn't address the defaults/overrides distinction we want to add there (which will require two distinct files, adding another queried config file for each option)

The rest seems fine, in particular:

We need to have platform specific paths for the system-wide config file.

We need to add this system-wide file at the right place in the stack definitions.

The new store *is* hooked in the right places but since this mp is based on an old trunk version, this can observed only for already migrated options. As such, we can't even measure its impact with -Econfig_stats.

So I'd say, either we land the path handling stuff only or we wait for the bugs mentioned above to be fixed before reconsidering this mp.

review: Needs Fixing

[Martin Pool (mbp)]

I'd really like to get this feature in, and to not have this work stalled.

Vincent's away at the moment, but what do you think, Thomi, do you want to have a go at those changes suggested by Vincent or should someone pick it up?

> It introduces one more config file that will be accessed for each option query (http://pad.lv/832042).

That bug's important to fix, but I don't think it needs to strictly serialize landing this. Reading a global file will make the penalty for multiple lookups linearly worse, but hopefully it doesn't block it entirely.

> It introduces a new file with a semantic which is not the one we want (regarding section filtering/ordering http://pad.lv/832046).

Right, but again it doesn't seem wrong so much as just not yet fixing things we'd like to fix in existing files.

> It doesn't address the defaults/overrides distinction we want to add there (which will require two distinct files, adding another queried config file for each option)

I'm not sure I understand that. Is this to say you want one global configuration that is a low priority, and another that overrides everything else?

[Thomi Richards (thomir-deactivatedaccount)]

Hi Martin,

On 14 October 2011 20:03, Martin Pool <email address hidden> wrote:

> I'd really like to get this feature in, and to not have this work stalled.
>
> Vincent's away at the moment, but what do you think, Thomi, do you want to
> have a go at those changes suggested by Vincent or should someone pick it
> up?
>
>
My day-job is getting pretty busy this time of year (just a few weeks until
the end of semester), but I'll see what I can do. The biggest issue stopping
this work going ahead right now is that I don't have a clear idea of what
needs to happen to get it merged. I don't understand the desired config file
semantic, and that seems like a separate piece of work to me.

If you know what needs to be done, please let me know and I'll try and find
an evening or two in which to clean up the code.

Cheers,

--
Thomi Richards

[Martin Packman (gz)]

I think underlying config changes that are confusing the status of this proposal are gradually getting resolved. For now though, I'm going to move this off the review queue, but will poke Vincent later to make sure we know what else we need before global configs can move forward.

[Vincent Ladeuil (vila)]

For the record, I've proposed https://code.launchpad.net/~vila/bzr/832046-globs-store-ordered/+merge/86734 which address one of the pre-requisites.

Unmerged revisions

6055. By Thomi Richards on 2011-08-05

Reverted changes after feedback from Vila.

6054. By Thomi Richards on 2011-08-04

Hid BranchStore class behind _get_branch_store() factory function.

6053. By Thomi Richards on 2011-08-03

Windows version 6 (WIndows 7, Server 2008) puts the common app data folder in a different place. Updated docstrings and fixed unit test.

6052. By Thomi Richards on 2011-08-03

Forgot to update docstring for system_config_dir()

6051. By Thomi Richards on 2011-08-03

Several changes to clean up global config work:
 * Store classes (GlobalStore et al) are now factory functions.
 * What was previously called a "Global store" is now called a "User Store".
 * similarly, what was the "System Global Store" is now just the "Global Store"

6050. By Thomi Richards on 2011-07-30

Added unit tests to test the new path functions for win32 and freebsd.

6049. By Thomi Richards on 2011-07-30

Added unit tests for new method in win32utils.py

6048. By Thomi Richards on 2011-07-28

Fixed docstring for config.system_config_dir

6047. By Thomi Richards on 2011-07-28

LocationStack and BranchStack now use system global store as well.

6046. By Thomi Richards on 2011-07-26

First pass attempt at global config file implementation.

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2011-08-02 01:10:27 +0000
+++ bzrlib/config.py	2011-08-05 04:17:29 +0000
@@ -55,7 +55,7 @@
                    turns on create_signatures.
 create_signatures - this option controls whether bzr will always create
                     gpg signatures or not on commits.  There is an unused
-                    option which in future is expected to work if               
+                    option which in future is expected to work if
                     branch settings require signatures.
 log_format - this option sets the default log format.  Possible values are
              long, short, line, or a plugin can register new formats.
@@ -72,6 +72,7 @@
 up=pull
 """
 
+from distutils import sysconfig
 import os
 import string
 import sys
@@ -452,7 +453,7 @@
         return None
 
     def acceptable_keys(self):
-        """Comma separated list of key patterns acceptable to 
+        """Comma separated list of key patterns acceptable to
         verify-signatures command"""
         result = self._acceptable_keys()
         return result
@@ -1533,6 +1534,26 @@
             base = os.path.expanduser("~")
         return osutils.pathjoin(base, ".bazaar")
 
+def system_config_dir():
+    """Return system-wide configuration directory.
+
+    By default this is %ALLUSERSAPPDATA%/bazaar/2.0 on Windows, /etc/bazaar
+    on Mac OS X and Linux, and {PREFIX}/etc/bazaar on BSD, where "{PREFIX}" is
+    the path prefix python was installed with (usually "/usr/local").
+    """
+    if sys.platform == 'win32':
+        # environ variables on Windows are in user encoding/mbcs. So decode
+        # before using one
+        base = win32utils.get_system_appdata_location()
+        if base is None:
+            raise errors.BzrError('Could not determine system-wide application'
+                                  'data directory. Do you have ALLUSERSAPPDATA'
+                                  'set?')
+        return osutils.pathjoin(base, 'bazaar', '2.0')
+    elif sys.platform.startswith('freebsd'):
+        return osutils.pathjoin(sysconfig.PREFIX, 'etc', 'bazaar')
+    else:
+        return osutils.pathjoin('/etc', "bazaar")
 
 def config_filename():
     """Return per-user configuration ini file filename."""
@@ -1608,11 +1629,11 @@
 
     Only used when none is set in the environment or the id file.
 
-    This only returns an email address if we can be fairly sure the 
+    This only returns an email address if we can be fairly sure the
     address is reasonable, ie if /etc/mailname is set on unix.
 
-    This doesn't use the FQDN as the default domain because that may be 
-    slow, and it doesn't use the hostname alone because that's not normally 
+    This doesn't use the FQDN as the default domain because that may be
+    slow, and it doesn't use the hostname alone because that's not normally
     a reasonable address.
     """
     if sys.platform == 'win32':
@@ -1794,7 +1815,7 @@
         :param user: login (optional)
 
         :param path: the absolute path on the server (optional)
-        
+
         :param realm: the http authentication realm (optional)
 
         :return: A dict containing the matching credentials or None.
@@ -1938,7 +1959,7 @@
 
         :param path: the absolute path on the server (optional)
 
-        :param ask: Ask the user if there is no explicitly configured username 
+        :param ask: Ask the user if there is no explicitly configured username
                     (optional)
 
         :param default: The username returned if none is defined (optional).
@@ -2074,7 +2095,7 @@
         :param override_existing: Raise KeyErorr if False and something has
                 already been registered for that key. If True, ignore if there
                 is an existing key (always register the new value).
-        :param fallback: Whether this credential store should be 
+        :param fallback: Whether this credential store should be
                 used as fallback.
         """
         return super(CredentialStoreRegistry,
@@ -2094,7 +2115,7 @@
         :param override_existing: If True, replace the existing object
                 with the new one. If False, if there is already something
                 registered with the same key, raise a KeyError
-        :param fallback: Whether this credential store should be 
+        :param fallback: Whether this credential store should be
                 used as fallback.
         """
         return super(CredentialStoreRegistry, self).register_lazy(
@@ -2575,6 +2596,12 @@
                                     possible_transports=possible_transports)
         super(GlobalStore, self).__init__(t, 'bazaar.conf')
 
+class SystemGlobalStore(LockableIniFileStore):
+
+    def __init__(self, possible_transports=None):
+        t = transport.get_transport_from_path(system_config_dir(),
+                                    possible_transports=possible_transports)
+        super(SystemGlobalStore, self).__init__(t, 'bazaar.conf')
 
 class LocationStore(LockableIniFileStore):
 
@@ -2822,9 +2849,11 @@
 class GlobalStack(_CompatibleStack):
 
     def __init__(self):
-        # Get a GlobalStore
+        # Get a GlobalStore and a system-wide global store:
         gstore = GlobalStore()
-        super(GlobalStack, self).__init__([gstore.get_sections], gstore)
+        swgstore = SystemGlobalStore()
+        super(GlobalStack, self)\
+            .__init__([gstore.get_sections, swgstore.get_sections], gstore)
 
 
 class LocationStack(_CompatibleStack):
@@ -2836,8 +2865,10 @@
         lstore = LocationStore()
         matcher = LocationMatcher(lstore, location)
         gstore = GlobalStore()
+        swgstore = SystemGlobalStore()
         super(LocationStack, self).__init__(
-            [matcher.get_sections, gstore.get_sections], lstore)
+            [matcher.get_sections, gstore.get_sections, swgstore.get_sections],
+            lstore)
 
 class BranchStack(_CompatibleStack):
 
@@ -2846,8 +2877,10 @@
         lstore = LocationStore()
         matcher = LocationMatcher(lstore, branch.base)
         gstore = GlobalStore()
+        swgstore = SystemGlobalStore()
         super(BranchStack, self).__init__(
-            [matcher.get_sections, bstore.get_sections, gstore.get_sections],
+            [matcher.get_sections, bstore.get_sections, gstore.get_sections,
+             swgstore.get_sections],
             bstore)
         self.branch = branch
 

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2011-07-11 10:53:46 +0000
+++ bzrlib/tests/test_config.py	2011-08-05 04:17:29 +0000
@@ -17,6 +17,7 @@
 """Tests for finding and reading the bzr config file[s]."""
 # import system imports here
 from cStringIO import StringIO
+from distutils import sysconfig
 import os
 import sys
 import threading
@@ -545,8 +546,20 @@
                 'BZR_HOME', r'C:\Documents and Settings\bogus\Application Data')
             self.bzr_home = \
                 'C:/Documents and Settings/bogus/Application Data/bazaar/2.0'
+            # Windows ver 6 (Windows 7, Server 2008) uses a different location
+            # from earlier versions:
+            if sys.getwindowsversion().major == 6:
+                self.sys_config_dir = r'C:/ProgramData/bazaar/2.0'
+            else:
+                self.sys_config_dir = \
+                    r'C:/Documents and Settings/All Users/Application Data'
         else:
             self.bzr_home = '/home/bogus/.bazaar'
+            if sys.platform.startswith('freebsd'):
+                self.sys_config_dir = osutils.pathjoin(sysconfig.PREFIX, 'etc',
+                    'bazaar')
+            else:
+                self.sys_config_dir = '/etc/bazaar'
 
     def test_config_dir(self):
         self.assertEqual(config.config_dir(), self.bzr_home)
@@ -567,6 +580,9 @@
         self.assertEqual(config.xdg_cache_dir(),
             '/home/bogus/.cache')
 
+    def test_sys_config_dir(self):
+        self.assertEqual(config.system_config_dir(), self.sys_config_dir)
+
 
 class TestXDGConfigDir(tests.TestCaseInTempDir):
     # must be in temp dir because config tests for the existence of the bazaar
@@ -3175,7 +3191,7 @@
         conf = config.AuthenticationConfig(_file=StringIO(
                 'foo = bar\xff'))
         self.assertRaises(errors.ConfigContentError, conf._get_config)
-        
+
     def test_missing_auth_section_header(self):
         conf = config.AuthenticationConfig(_file=StringIO('foo = bar'))
         self.assertRaises(ValueError, conf.get_credentials, 'ftp', 'foo.net')
@@ -3371,8 +3387,8 @@
                                            port=99, path='/foo',
                                            realm='realm')
         CREDENTIALS = {'name': 'name', 'user': 'user', 'password': 'password',
-                       'verify_certificates': False, 'scheme': 'scheme', 
-                       'host': 'host', 'port': 99, 'path': '/foo', 
+                       'verify_certificates': False, 'scheme': 'scheme',
+                       'host': 'host', 'port': 99, 'path': '/foo',
                        'realm': 'realm'}
         self.assertEqual(CREDENTIALS, credentials)
         credentials_from_disk = config.AuthenticationConfig().get_credentials(
@@ -3386,8 +3402,8 @@
         self.assertIs(None, conf._get_config().get('name'))
         credentials = conf.get_credentials(host='host', scheme='scheme')
         CREDENTIALS = {'name': 'name2', 'user': 'user2', 'password':
-                       'password', 'verify_certificates': True, 
-                       'scheme': 'scheme', 'host': 'host', 'port': None, 
+                       'password', 'verify_certificates': True,
+                       'scheme': 'scheme', 'host': 'host', 'port': None,
                        'path': None, 'realm': None}
         self.assertEqual(CREDENTIALS, credentials)
 
@@ -3662,7 +3678,7 @@
 
     def test_auto_user_id(self):
         """Automatic inference of user name.
-        
+
         This is a bit hard to test in an isolated way, because it depends on
         system functions that go direct to /etc or perhaps somewhere else.
         But it's reasonable to say that on Unix, with an /etc/mailname, we ought

=== modified file 'bzrlib/tests/test_win32utils.py'
--- bzrlib/tests/test_win32utils.py	2011-06-14 01:26:41 +0000
+++ bzrlib/tests/test_win32utils.py	2011-08-05 04:17:29 +0000
@@ -38,7 +38,7 @@
 Win32RegistryFeature = features.ModuleAvailableFeature('_winreg')
 CtypesFeature = features.ModuleAvailableFeature('ctypes')
 Win32comShellFeature = features.ModuleAvailableFeature('win32com.shell')
-Win32ApiFeature = features.ModuleAvailableFeature('win32api') 
+Win32ApiFeature = features.ModuleAvailableFeature('win32api')
 
 
 # Tests
@@ -231,6 +231,22 @@
             encoding = osutils.get_user_encoding()
             self.assertPathsEqual(lad, env.decode(encoding))
 
+    def test_system_appdata_not_using_environment(self):
+        # Test that we aren't falling back to the environment
+        first = win32utils.get_system_appdata_location()
+        self.overrideEnv("ALLUSERSAPPDATA", None)
+        self.assertPathsEqual(first, win32utils.get_system_appdata_location())
+
+    def test_system_appdata_matches_environment(self):
+        # ALLUSERSAPPDATA typically only exists on Vista, so we only attempt to
+        # compare when it exists.
+        auad = win32utils.get_system_appdata_location()
+        env = os.environ.get("ALLUSERSAPPDATA")
+        if env:
+            # XXX - See bug 262874, which asserts the correct encoding is 'mbcs'
+            encoding = osutils.get_user_encoding()
+            self.assertPathsEqual(auad, env.decode(encoding))
+
 
 class TestLocationsPywin32(TestLocationsCtypes):
 

=== modified file 'bzrlib/win32utils.py'
--- bzrlib/win32utils.py	2011-06-11 01:11:43 +0000
+++ bzrlib/win32utils.py	2011-08-05 04:17:29 +0000
@@ -93,6 +93,7 @@
 CSIDL_APPDATA = 0x001A      # Application Data folder
 CSIDL_LOCAL_APPDATA = 0x001c# <user name>\Local Settings\Application Data (non roaming)
 CSIDL_PERSONAL = 0x0005     # My Documents folder
+CSIDL_COMMON_APPDATA = 0x0023 # C:\Documents and Settings\All Users\Application Data
 
 # from winapi C headers
 MAX_PATH = 260
@@ -291,6 +292,25 @@
     return get_appdata_location()
 
 
+def get_system_appdata_location():
+    """Return system-wide local Application Data location.
+
+    This returns the location of the system wide (i.e.- not per-user)
+    application data folder. This can be one of several locations, depending on
+    your platform. Common locations include "C:/ProgramData" and
+    "C:/Documents and Settings/All Users/Application Data".
+
+    Returned value can be unicode or plain string.
+    To convert plain string to unicode use
+    s.decode(osutils.get_user_encoding())
+    (XXX - but see bug 262874, which asserts the correct encoding is 'mbcs')
+    """
+    system_appdata = _get_sh_special_folder_path(CSIDL_COMMON_APPDATA)
+    if system_appdata:
+        return system_appdata
+    return os.environ.get('ALLUSERSAPPDATA')
+
+
 def get_home_location():
     """Return user's home location.
     Assume on win32 it's the <My Documents> folder.
@@ -537,19 +557,19 @@
     """
     # First, spit the command line
     s = cmdline.Splitter(command_line, single_quotes_allowed=single_quotes_allowed)
-    
-    # Bug #587868 Now make sure that the length of s agrees with sys.argv 
-    # we do this by simply counting the number of arguments in each. The counts should 
-    # agree no matter what encoding sys.argv is in (AFAIK) 
-    # len(arguments) < len(sys.argv) should be an impossibility since python gets 
+
+    # Bug #587868 Now make sure that the length of s agrees with sys.argv
+    # we do this by simply counting the number of arguments in each. The counts should
+    # agree no matter what encoding sys.argv is in (AFAIK)
+    # len(arguments) < len(sys.argv) should be an impossibility since python gets
     # args from the very same PEB as does GetCommandLineW
     arguments = list(s)
-    
+
     # Now shorten the command line we get from GetCommandLineW to match sys.argv
     if len(arguments) < len(argv):
         raise AssertionError("Split command line can't be shorter than argv")
     arguments = arguments[len(arguments) - len(argv):]
-    
+
     # Carry on to process globs (metachars) in the command line
     # expand globs if necessary
     # TODO: Use 'globbing' instead of 'glob.glob', this gives us stuff like