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

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/?

> (/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...

> 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?

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.

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.

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?

+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.

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.

> +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...

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...

> 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

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?

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/?

>
> > 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.

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

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

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,

> 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.

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.

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.

> 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,

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.

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.

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?

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

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.

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.