Allow trac.ini to include other files

[anonymous]

Support an include directive in the configuration file syntax.

include "trac-ldap.ini"

will recursively load the directives in trac-ldap.ini .

Rationale: this will make it possible to split the configuration into multiple files.

TracAdmin will be able to write to a tracadmin.ini, included by trac.ini (included eg at the bottom to give it higher priority).

This is necessary to manage trac configuration programmatically.

My use case is to install trac plugins with a plugin.ini snippet using puppet (this requires no manual intervention). Currently I am merging all these small .ini files into one big trac.ini, but this also means that any changes TracAdmin writes to the file are lost.

[Emmanuel Blot]

This is already implemented in 0.11, you can use the inherit section, such as in:

[inherit]
file = /another/path/trac.ini

[anonymous]

This is great.

Is it also possible to change the path TracAdmin writes to?

[Emmanuel Blot]

Replying to anonymous:

Is it also possible to change the path TracAdmin writes to?

Which one: .../conf/trac.ini?
No, it is not.

[haizaar@…]

Is globbing supported? I.e. can I do something like this:

[inherit]
file = /etc/trac/*.ini

[haizaar@…]

Problem: Can not include more then one file:

$>  cat /etc/trac/trac-graphviz.ini
[components]
graphviz.* = enabled
$>  cat /etc/trac/trac-toc-macro.ini
[components]
tractop.* = enabled
$> cat $PWD/conf/trac.ini
...
[inherit]
file = /etc/trac/trac-graphviz.ini
file = /etc/trac/trac-toc-macro.ini

Only the last mentioned file is included. Trac-0.11

[Emmanuel Blot]

Replying to haizaar@haizaar.com:

Problem: Can not include more then one file:

This is a limitation of the Ini file parser, and the .ini file format: you cannot use the same key more than once within the same section. I guess this limitation could be overcomed with a list of files, such as:

file = a.ini, b.ini

[Martijn Pieters <trac.edgewall.org@…>]

Replying to eblot:

I guess this limitation could be overcomed with a list of files, such as: file = a.ini, b.ini

The ​buildout convention is space separation:

    file = a.ini b.ini

This has the added advantage that it's easy to spread this across multiple lines:

    file =
        a.ini
        b.ini

Config-file editors don't have to remember to insert commas, and the code reading the option simply has to use .split() to turn this into a list.

[osimons]

Following #8290/[8243] for upcoming 0.11.5, the config parser supports chaining of config files. This should support much the same need as including more than one file in same ini file. The chaining also implies ordering, and it will use the option on a first-come-first-served basis as it traverses the chain up from project trac.ini and ending in the default provided component Option properties.

[Remy Blank]

Proof of concept for multiple file inclusion in trac.ini.

[Remy Blank]

The patch above is a proof of concept on trunk for multiple file inclusion in trac.ini via the [inherit] section. It should be completely backward-compatible with the current functionality, and all tests pass. But I should really add more tests to make sure everything works correctly.

Oh, there's already one issue: on initenv, the options that are in the inherited files are re-included. I'll have to look at that. But it still shows the general idea.

Thoughts?

[Christian Boos]

It would be cool to be able to trace for any given setting in which trac.ini file it was defined. That would help a lot for debugging if this information get shown in the /about page (as a tooltip). Maybe it would even help for the problem you mention above.

What about the PathOption concerns raised by osimons?

[Remy Blank]

Replying to cboos:

It would be cool to be able to trace for any given setting in which trac.ini file it was defined. That would help a lot for debugging if this information get shown in the /about page (as a tooltip). Maybe it would even help for the problem you mention above.

Good idea, I'll see how I can do that.

What about the PathOption concerns raised by osimons?

The current functionality always resolves PathOptions relative to the environment trac.ini (the _base_filename attribute). This patch does exactly the same. Did we want something else?

[osimons]

Replying to rblank:

Replying to cboos:

What about the PathOption concerns raised by osimons?

The current functionality always resolves PathOptions relative to the environment trac.ini (the _base_filename attribute). This patch does exactly the same. Did we want something else?

No it doesn't work like that. If the project config itself has it (or read from options registry), then yes. If a parent defines the option then the parent resolves it relative to its own location whereever that may be.

Also, the patch ignores templates_dir and plugins_dir that are also under [inherit]. A config section like the one in the test-case won't work, and using --inherit we won't have any way of naming them either. Nor ordering parents by indicating which is to be preferred if same setting is defined in more than one parent. If anything, the multiple parents should be a list like:

[inherit]
file = trac-site1.ini, trac-site2.ini

A change that just converts [inherit] file = from Option to ListOption is something that would work well - provided the rules for evaluating options don't change (or are at least discussed and handled).

[osimons]

BTW, my suggestion really requires a new PathListOption type.. - unless that is already available in trunk (that I still don't have running)… :-)

[Christian Boos]

Replying to osimons:

Replying to rblank:

Replying to cboos:

What about the PathOption concerns raised by osimons?

The current functionality always resolves PathOptions relative to the environment trac.ini (the _base_filename attribute). This patch does exactly the same. Did we want something else?

No it doesn't work like that. If the project config itself has it (or read from options registry), then yes. If a parent defines the option then the parent resolves it relative to its own location whereever that may be.

Well, then it should, I think. The behavior suggested by Remy is more flexible, as otherwise you won't be able to specify a project-relative path from a parent config file (e.g. specifying "the authz_policy.conf file is by default to one from the environment's config folder" in a global.ini file). I find the (current) behavior less useful, as in this case you could as well use an absolute path.

… the multiple parents should be a list like:

[inherit]
file = trac-site1.ini, trac-site2.ini

I would also prefer it this way, the ordering is made more intuitive, and this should avoid the problems with (or special casing) templates_dir and plugins_dir.

[Remy Blank]

Replying to osimons:

No it doesn't work like that. If the project config itself has it (or read from options registry), then yes. If a parent defines the option then the parent resolves it relative to its own location whereever that may be.

Ah, yes, by bad. Only default options are resolved relative to trac.ini in Section.getpath(), not the options actually parsed. I also prefer resolving relative to trac.ini in any case, as that's what we do for templates_dir and plugins_dir (well, actually they are resolved relative to the environment, not trac.ini).

Also, the patch ignores templates_dir and plugins_dir that are also under [inherit].

I have fixed that in my sandbox already.

A config section like the one in the test-case won't work

Can you explain that? The test case passes, so I'm missing something.

and using --inherit we won't have any way of naming them either.

That's true, I just generate keys with the names "file01", "file02", and so on.

Nor ordering parents by indicating which is to be preferred if same setting is defined in more than one parent.

This is handled by sorting the parent configs by their key in [inherit], so the "file01" parent will always be evaluated before the "file02" parent. That's also how Mercurial handles stuff in hgrc.

If anything, the multiple parents should be a list like:

That would be another option, yes. I'm ok with using that instead of alphabetically-ordered keys, let's just hope that nobody uses paths with a comma ;-)

[Remy Blank]

Use [inherit] file with a comma-separated list of paths

[Remy Blank]

The second patch uses [inherit] file with a comma-separated list of paths, and also fixes environment creation.

[osimons]

Just note that the docs for getpath() has more or less "forever" been:

Return the value of the specified option as a path name, relative to
the location of the configuration file the option is defined in.

The example cboos used can also be turned around, so that if I have a plugin that uses and refers to various shared resources, each project can just include the shared config and that configuration and its resources will be self-contained - regardless of where each project is located on disk. We can't make both examples work, so that leaves us to decide on one of them.

If we change that behaviour, we need to carefully consider the possible consequences for existing installations. I have no idea how widespread the use of relative paths are in shared configurations (I don't have any, so either way is fine by me personally). The good thing is that the inherited path option only really started working in 0.11.5dev….

[Remy Blank]

Replying to osimons:

If we change that behaviour, we need to carefully consider the possible consequences for existing installations. I have no idea how widespread the use of relative paths are in shared configurations (I don't have any, so either way is fine by me personally).

The truth is, relative paths specified with --inherit have never actually worked completely correctly, that's why I have added a call to os.path.abspath() in [8703] (see #8724).

[osimons]

Replying to rblank:

The truth is, relative paths specified with --inherit have never actually worked completely correctly, that's why I have added a call to os.path.abspath() in [8703] (see #8724).

Hmm. Haven't seen that changeset. I have close to 2000 projects running that inherits from relative locations, so that must obviously be wrong…. I suspect the mentioned ticket is not aware of the 'relativeness' being relative to the config and not to current location for running trac-admin or project.

I'm tempted to ask for that changeset to be reverted…

[Remy Blank]

Inheriting from relative locations worked, but specifying them in --inherit was very counter-intuitive, as the the path given to --inherit had to be specified relative to the not-yet created environment's trac.ini. The norm, when using various Unix tools, is that relative paths are specified relative to the current working directory. That's probably what happened to the reporter of #8724, and it also happened to me while trying to find out what the issue was.

I kept trying to do:

trac-admin $ENV my_env --inherit=global.ini

and was wondering why a [ticket-workflow] section kept appearing in my trac.ini. Finally I found out that the following worked:

trac-admin $ENV my_env --inherit=../../global.ini

and then I understood what happened.

I'm ok with reverting [8703], but we need to document the option better, preferably in the help text for trac-admin $ENV initenv.

[osimons]

Replying to rblank:

I'm ok with reverting [8703], but we need to document the option better, preferably in the help text for trac-admin $ENV initenv.

Thanks, and agreed on docs. Running trac-admin help I see the --inherit option isn't even mentioned at all. Likely my fault from back when I made it, but I blame the awkwardness of adding lengthy and informative help to the console application… :-)

[osimons]

Either that, or we can just look for the file at the obvious locations and calculate the new correct relative path ourselves (if not specified correctly and pointing to an existing file)?

[Remy Blank]

Replying to osimons:

Thanks, and agreed on docs. Running trac-admin help I see the --inherit option isn't even mentioned at all. Likely my fault from back when I made it, but I blame the awkwardness of adding lengthy and informative help to the console application… :-)

Oh, right, nice excuse. Ever looked at trac-admin in 0.12? All nice and clean ;-)

Either that, or we can just look for the file at the obvious locations and calculate the new correct relative path ourselves (if not specified correctly and pointing to an existing file)?

On second thought, changing the current behavior (like I did in [8703]) is a bad idea, as many people (and their scripts) are likely to depend on it. So I will revert the change and improve the documentation.

[Remy Blank]

The change has been reverted in [8740] (on 0.11-stable), and the resolution of relative paths has been added in [8741] (on trunk).

[Remy Blank]

Refreshed patch with better documentation.

[osimons]

Good going, Remy. Obviously still not able to test it, but hey… it reads well :-) I suppose the only open question is the change for how paths are resolved. I see the attraction of resolving all values relative to the environment and its config, but I'm not convinced we gain so much from it - other than breaking existing installations. I'll give in to the majority though, if you both think this needs to change.

[Remy Blank]

Replying to osimons:

I suppose the only open question is the change for how paths are resolved. I see the attraction of resolving all values relative to the environment and its config, but I'm not convinced we gain so much from it - other than breaking existing installations.

Will it break your 2000 installations?

I have no strong preference either, but always resolving from the primary trac.ini makes the code simpler, so +0.5 for the change.

Christian, your last word?

[osimons]

Replying to rblank:

Will it break your 2000 installations?

Perhaps, but easily fixable anyway. However, not so sure as it is mostly used for [inherit] file = for chainable configurations - and according to patch this still looks like it is resolved relative to the location of the currently parsed config and not the base config (ie resolved relative to location of self.filename, and no _base_filename is carried through anymore seeing that is no longer needed to resolve a PathOption). Seeing this option is special, I do actually think it makes sense that a path for this option is always evaluated relative to the location of the file where it is defined - and not by the varying locations of projects that consumes it. If so, I'll have no problem with changing the regular PathOption to always be project-config-relative.

[Remy Blank]

Replying to osimons:

and according to patch this still looks like it is resolved relative to the location of the currently parsed config and not the base config

Mmh, you're right, the patch is actually still "incorrect" in that respect. Well, let's wait for Christian's vote, then I'll implement the correct variant.

[Remy Blank]

Actually, I would prefer even another option: relative paths in config files are always resolved relative to the environment directory (instead of the directory containing the config file). I think that's the easiest to understand for the admin.

[Christian Boos]

I'd still want to think a bit more about this.

[Christian Boos]

So to sum up my point of view on this topic:

• my personal intuition would be to view [inherit] file = ... as a kind of content inclusion: it doesn't matter where the included files are located, everything works as if the content of the inherited files would be included textually in the current conf file. That means that relative paths would be relative to the current .ini or the current environment, whatever we decided for this point.
• now I'm sure the opposite position held by osimons is valid as well: consider that when inheriting files, we would inherit "self-contained" complex configurations, that could eventually be relocated. So relative paths would be relative to the file in which the setting is written. In his experience, that's useful for maintaining large sets of instances. This is not something I'm familiar with, and I can only rely on his advice here.

So while the first point of view is maybe the simplest and most easy to explain/implement, it's perhaps not the most useful in practice. We should perhaps stick with the second approach and document it better if needed.

[osimons]

Yup. Agree.

Not mentioned in the summary though is the messy upgrade and experiences of existing installations when new code runs and evaluates all inherited relative paths 'wrong'. Do we issue warnings? Do we try to detect 'wrong' configs? How do we explain and document the change to those potentially affected by the change? A new logic that don't know or care where a setting is read from will not likely be aware of any such potential problem anyway….

I just don't see a compelling reason to change the current way of evaluating. On the other hand, perhaps it is just me using relative paths in inherited config files, and if so there will be no better time to change than now… :-)

[Remy Blank]

Replying to cboos:

So while the first point of view is maybe the simplest and most easy to explain/implement, it's perhaps not the most useful in practice. We should perhaps stick with the second approach and document it better if needed.

I hear you. I suppose it depends what is done more often: referencing files relative to the environment (which will remain impossible with this approach) or moving configuration trees around.

Simon, do you use AccountManager in your installations? How do you reference the htpasswd files? I assume you set it in each individual trac.ini file. This could become a single entry in an included file if resolving was done relative to the environment.

Then again, anything that changes the current behavior will require work to migrate. Too bad for simplicity.

[Remy Blank]

Resolve relatve paths relative to the file that contains them.

[Remy Blank]

The patch above resolves relative paths in [inherit] file and in PathOptions relative to the directory containing the file. Testing welcome.

As for the result… Well, I'm not happy at all:

• It's not (reasonably) possible to use the cache for Section.getpath(), so PathOptions are uncached.

• The logic in Section.getpath() is ugly even without caching, especially compared to what it would be when resolving relative to the base file.

So we have a functionality that was introduced only recently, that is complicated to explain, and complicated to implement and maintain. I'll ask one last time if everyone is sure that's what we want. After that I'll shut up.

(Let's just say this path resolution functionality is not something I'll ever touch again, not even with a ten-foot pole. Simon, I hope you're ready to support it in the future.)

[osimons]

Replying to rblank:

• It's not (reasonably) possible to use the cache for Section.getpath(), so PathOptions are uncached.

As far as I can see, there is nothing related to how relative paths are evaluated that is affected by this. A setting is just as cacheable (or not) whether it evaluates to A or B. Seeing we check timestamps and reload if any files have changed, won't a path evaluation be cacheable regardless of old path-evaluation, or this move from simple chained configs to tree structure with multipe inherits at multiple levels?

• The logic in Section.getpath() is ugly even without caching, especially compared to what it would be when resolving relative to the base file.

I can see that it requires some more lines of code to evaluate in proper path context.

So we have a functionality that was introduced only recently, that is complicated to explain, and complicated to implement and maintain. I'll ask one last time if everyone is sure that's what we want. After that I'll shut up.

I'll survive either. I don't have any settings or structures that I can't make the transition - and also seeing I'm more-than-average aware of the change and its implications of course… I'll bend to majority vote if you and cboos feel strongly about this. If so though, note that any patch really should include an upgrade script to change any existing relative settings - or however we select to deal with the change for upgraded installs.

(Let's just say this path resolution functionality is not something I'll ever touch again, not even with a ten-foot pole. Simon, I hope you're ready to support it in the future.)

Right - and if you do change, I suppose you'll be doing all the support for the installations that break? :-)

[Remy Blank]

Replying to osimons:

As far as I can see, there is nothing related to how relative paths are evaluated that is affected by this. A setting is just as cacheable (or not) whether it evaluates to A or B.

Not quite. The issue is more subtle, and comes from the fact that using get() or getpath() on a setting will return different values for relative paths (the former will return the setting unchanged, the latter prefixes it with the base path). If you call get() first, the unchanged value will be cached. How would you retrieve the correct value in getpath() after that? (Note that we have exactly such a case in our unit tests.) You would have to cache not only the "raw" value, but also the base path, so that you can reconstruct the correct path in getpath().

That's why getpath() is currently uncached (in trunk), is uncached in the patch, and should probably stay so. This has nothing to do with multiple inclusion. It's not a big issue, but still, it adds complexity.

I'll bend to majority vote if you and cboos feel strongly about this.

I do feel somewhat strongly about this, for the following reason: I have the feeling that the current algorithm was a sub-optimal decision, making the functionality more difficult to understand and maintain. Waiting much longer will make it impossible to change at all, so we will be stuck with it. My question was a last "sanity" check, and as promised, I'll shut up afterward. (Note that I would have interpreted a lack of reply as "yes, that's what we want", as you were clear about it and Christian was only a little ambiguous :-)

If so though, note that any patch really should include an upgrade script to change any existing relative settings - or however we select to deal with the change for upgraded installs.

This could probably be done, yes. I'd have to think about it some more.

Right - and if you do change, I suppose you'll be doing all the support for the installations that break? :-)

Sure I will! Have been here for over a year and been supporting the consequences of my mistakes all along ;-)

Joking apart, this comment wasn't meant to sound aggressive, and I hope it didn't come across as such. The thing is, it takes me a tremendous amount of energy to implement something I'm not convinced of, because I keep seeing the (simpler|cooler|more useful|faster|whatever) alternative and have to resist the urge of doing that. That's why I would stay away from that code. It's just one of my bad character traits ;-)

[Christian Boos]

Well, thinking again about all this… attachment:5525-multiple-inherit-r8741.patch​ is:

• simpler than what we have in 0.11-stable
• simpler than attachment:5525-multiple-inherit-r8902.patch​
• doesn't break backward compatibility, as it keeps the <env>/conf folder as the base

[Christian Boos]

Replying to cboos:

Well, thinking again about all this… attachment:5525-multiple-inherit-r8741.patch​ is:

• simpler than what we have in 0.11-stable
• simpler than attachment:5525-multiple-inherit-r8902.patch​
• doesn't break backward compatibility, as it keeps the <env>/conf folder as the base

So let's go for this solution.

[Remy Blank]

Patch (with a few more fixes) applied in [9037].

[Ryan J Ollos]

It appears there is an unused protected attribute _cache in [9037#file11]: tags/trac-1.0.2/trac/config.py@:293-294#L264​. Could that be removed?

[Remy Blank]

Indeed, this looks unused. Sure, feel free to remove it, if all tests still pass.

[Peter Suter]

Perhaps some other variable was meant to be cleared, e.g. self._sections = {} or for k in self._sections: self._sections[k]._cache = {}?

# config inherits parent.ini containing [a] b=x
config.get('a', 'b') # caches 'x' in config._sections['a']._cache['b']
# change parent.ini to [a] b=y
config.parse_if_needed() # does not invalidate that cache
config.get('a', 'b') # should return 'y' but returns cached 'x'

[Remy Blank]

That's totally possible, but it's been too long for me to remember. The best would be to add a test case like you suggest, and fix the code if the test fails.

[Peter Suter]

Thanks. I created #11902 and will attach a patch.

[Ryan J Ollos]

Thanks for the feedback and additional investigation. I'll test the patch in #11902.