Configuration sections from plugins should be written to trac.ini

[Ryan J Ollos]

When a Trac environment is first created the default data is written to trac.ini​. However, when installing a plugin the default data is not written to trac.ini.

When the plugin enabled it would be nice to have the default data added to trac.ini. Currently the user must manually type the section and option name if they wish to change any configuration values. I think that most users would expect that the defaults for all configuration values be found in trac.ini.

A quick implementation that is not ideal, but demonstrates the idea is found in log:rjollos.git:t11437.

One issue with the quick implementation is that all of defaults are written to trac.ini, not just those of the plugin being installed. Maybe this is okay, but if a user likes to remove a lot default options to simplify the file, they may be surprised to have their changes undone (#7378).

#8290 had me thinking of a different solution, that plugins could add the information through existing IEnvironmentParticipant method, or new methods or helper functions that we add. The downside of course is that every plugin then needs to be modified to implement the behavior.

Side note: it might be useful to have a trac-admin $ENV config clean command to remove options from trac.ini that are no longer defined in an enabled Component. For example, with ​th:AccountManagerPlugin, many options were renamed going from 0.3 → 0.5, and the users must manually clean up these sections. This comes up rather frequently when users post their configuration to tickets and it contains a mess of old and new configuration options. Of course, it causes no harm to leave them, it is just more confusing and error prone to manage the trac.ini file. We could, by default, create a backup of the trac.ini file whenever clean is run.

[Jun Omae]

A quick implementation that is not ideal, but demonstrates the idea is found in log:rjollos.git:t11437.

Another PoC: jomae.git@t11437.1.

[Ryan J Ollos]

The changes in comment:1 look good to me. I'm testing them along with the changes in #11520 and I'll make some comments in that ticket.

[anonymous]

FWIW, Please keep in mind the use of the "global configuration". I use this extensively, and I keep most settings in the global configuration file.

For instance, if updating some setting in a Trac instance introduced a lot of other kinds of settings into the local configuration file, it could be then necessary to clean out all the defaults overriding the global configuration.

(I mainly mention this due to the comment about writing all defaults to trac.ini, not just those of the plugin being installed.)

[Ryan J Ollos]

The latest changes after #11520 can be found in log:rjollos.git:t11437.2. However, I plan to do more work before committing.

[S.N.Maijers@…]

I am not convinced of the usefulness of this. What happens if the configuration format for a plugin changes? Why would an administrator expect default options to be explicitly in the Trac configuration? The current system, where the administrator may set/override (default) configuration options works fine and is more robust in my opinion.

[Ryan J Ollos]

Replying to S.N.Maijers@…:

What happens if the configuration format for a plugin changes?

The same thing that would happen if the option has been added by editing the config file. The extra entries would be harmless and are the use case for the config clean command that I describe in comment:description.

Why would an administrator expect default options to be explicitly in the Trac configuration?

Less typing, less possibility of mistakes in entering the option name and less need to look up what options are available in the documentation. Every default option for Trac is written to the config file, so adding plugin defaults would be consistent.

The current system, where the administrator may set/override (default) configuration options works fine and is more robust in my opinion.

You've not made an argument for why the change would not be robust.

[jholg@…]

Replying to anonymous:

FWIW, Please keep in mind the use of the "global configuration". I use this extensively, and I keep most settings in the global configuration file.

+1 Please take into account multi-project trac installations where

• there's one "master" project and all the "slave" projects inherit its global configuration, overriding only those config options that differ
• plugins are installed globally i.e. the Python installation or virtualenv used for running Trac, not in a project's plugins dir

Changes introduced with this ticket should not lead to writing defaults to "slave" trac.ini files

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t11437.4. The following behaviors have been implemented:

• When a plugin is enabled through TracAdmin or the Web administration page, the component options are written to trac.ini if they don't already exists in trac.ini or a parent configuration.
• The # option-name = <inherited> markers are no longer written to trac.ini, but still if the value is defined in a parent configuration, the default value is not written to trac.ini when enabling a component or creating an environment.

[Jun Omae]

Reviewing.

• If possible, I think we should add unit tests for Configuration.set_defaults(component=...) with and without [inherit] file = ....
• If the component argument is tracopt.ticket.commit_updater.Commit, the method would wrongly set defaults of options in CommitTicketUpdater. My PoC is wrong.

  trac/config.py

  @@ -314 +314 @@
   
           if component:
               if component.endswith('.*'):
  -                component = component[:-1]
  -            component = component.lower()
  +                component = component[:-2]
  +            component = component.lower().split('.')
               from trac.core import ComponentMeta
               for cls in ComponentMeta._components:
  -                clsname = (cls.__module__ + '.' + cls.__name__).lower()
  -                if not clsname.startswith(component) and clsname != component:
  +                clsname = (cls.__module__ + '.' + cls.__name__).lower() \
  +                                                               .split('.')
  +                if clsname[:len(component)] != component and \
  +                        clsname != component:
                       continue
                   for option in cls.__dict__.itervalues():
                       if not isinstance(option, Option):

[Ryan J Ollos]

Additional refactoring and tests added in log:rjollos.git:t11437.4. There are some test failures when running $ make test=trac/tests/__init__.py, which are due to the other Components defined in trac.tests.config. I'm thinking about how to modify or restructure the tests to avoid the failures, but I welcome ideas on that.

[Jun Omae]

The Configuration.set_defaults method uses registered classes in ComponentMeta._components. I think we should clean up the class instances to remove test classes and interfaces in the tearDown of unit tests. ConfigSection.registry and ComponentMeta._registry have the same issue. See [0ba577eb4/jomae.git].

Minor thing, component.rstrip('.*') is used, Environment class strips only the trailing .* of component name in tags/trac-1.1.3/trac/env.py@:385#L382​. I think we should use the same logic.

[Ryan J Ollos]

Replying to jomae:

The Configuration.set_defaults method uses registered classes in ComponentMeta._components. I think we should clean up the class instances to remove test classes and interfaces in the tearDown of unit tests. ConfigSection.registry and ComponentMeta._registry have the same issue. See [0ba577eb4/jomae.git].

Thanks, I was trying to accomplish that but having no success.

If I run the tests using either of these two commands, ConfigurationSetDefaultsTestCase is skipped:

$ PYTHONPATH=. make test=trac/tests/config.py
$ PYTHONPATH=. python test/tests/config.py

SKIP: trac.tests.config.ConfigurationSetDefaultsTestCase (__name__ is not trac.tests.config)

Those two are the same of course because Makefile runs python test/tests/config.py. The tests get run for this command though:

$ python setup.py test -s trac.tests.config

Is that an expected limitation? If so, maybe we should modify how our Makefile runs the tests.

Minor thing, component.rstrip('.*') is used, Environment class strips only the trailing .* of component name in tags/trac-1.1.3/trac/env.py@:385#L382​. I think we should use the same logic.

Does that mean that trac.tests.config. should not cause the components in the trac.tests.config to be enabled? I suspect yes, since Trac enables the components in module1 with the following rules:

module1 = enabled

module1.* = enabled

but not with:

module1. = enabled

I guess that was probably your point though in saying that we should replicate the rule that exists in trac.env.

[Jun Omae]

Is that an expected limitation? If so, maybe we should modify how our Makefile runs the tests.

Yes. Running python trac/tests/config.py, CompA and CompB classes are defined under __main__ package. Then, all tests of ConfigurationSetDefaultsTestCase would fail. Similar discussion in comment:11:ticket:11312.

Minor thing, component.rstrip('.*') is used, Environment class strips only the trailing .* of component name in tags/trac-1.1.3/trac/env.py@:385#L382​. I think we should use the same logic.

Does that mean that trac.tests.config. should not cause the components in the trac.tests.config to be enabled? I suspect yes, since Trac enables the components in module1 with the following rules:

Right. modname1. = enabled wouldn't enable components in modname1 package.

[Jun Omae]

Those two are the same of course because Makefile runs python test/tests/config.py. The tests get run for this command though:

$ python setup.py test -s trac.tests.config

After the following patch, the Makefile would convert the file name to module name. We could run unit tests with make test=trac/tests/config.py and make trac/tests/config.py.

Makefile

@@ -124 +124 @@
 
 .PHONY: all help status clean clean-bytecode clean-mo
 
+%.py : status
+        python -m $(subst /,.,$(@:.py=)) $(testopts)
+
 ifdef test
 all: status
-        python $(test) $(testopts)
+        python -m $(subst /,.,$(test:.py=)) $(testopts)
 else
 all: help
 endif

[Ryan J Ollos]

Replying to jomae:

After the following patch, the Makefile would convert the file name to module name. We could run unit tests with make test=trac/tests/config.py and make trac/tests/config.py.

The ConfigurationSetDefaultsTestCase tests are still skipped since __name__ == __main__ in the module. Is there another complementary change that I should make?

With the following variation of your patch the ConfigurationSetDefaultsTestCase are executed using make test=trac/tests/config.py and make trac/tests/config.py.

Makefile

@@ -125 +125 @@
 .PHONY: all help status clean clean-bytecode clean-mo
 
 %.py : status
-       python setup.py -q test -s $(subst /,.,$(@:.py=)) $(testopts)
+       python setup.py -q test -s $(subst /,.,$(@:.py=)).suite $(testopts)
 
 ifdef test
 all: status
-       python setup.py -q test -s $(subst /,.,$(test:.py=)) $(testopts)
+       python setup.py -q test -s $(subst /,.,$(test:.py=)).suite $(testopts)
 else
 all: help
 endif

I haven't done enough testing yet to be confident of the change.

[Jun Omae]

Replying to rjollos:

The ConfigurationSetDefaultsTestCase tests are still skipped since __name__ == __main__ in the module. Is there another complementary change that I should make?

Sorry. I was confused with something else.

With the following variation of your patch the ConfigurationSetDefaultsTestCase are executed using make test=trac/tests/config.py and make trac/tests/config.py.

[…]

I haven't done enough testing yet to be confident of the change.

That patch seems to be good to me.

[Ryan J Ollos]

Thanks for the review. I committed against #11316 since the change fixes the issue in that ticket as well. I'm thinking we should suggest in TracDev/UnitTests to use the Makefile for test execution rather than suggest so many different ways to execute the tests, some of which can fail for some modules.

I'm also wondering if there might be a better way yet. If we use python -m unittest to run the tests, we could directly specify a single class or method to execute.

$ PYTHONPATH=. python -m unittest trac.tests.config
.............................................
----------------------------------------------------------------------
Ran 45 tests in 1.621s

OK
$ PYTHONPATH=. python -m unittest trac.tests.config.ConfigurationTestCase
......................................
----------------------------------------------------------------------
Ran 38 tests in 2.068s

OK
$ PYTHONPATH=. python -m unittest trac.tests.config.ConfigurationTestCase.test_repr
.
----------------------------------------------------------------------
Ran 1 test in 0.017s

OK

Makefile

@@ -125 +125 @@
 .PHONY: all help status clean clean-bytecode clean-mo
 
 %.py : status
-       python setup.py -q test -s $(subst /,.,$(@:.py=)).suite $(testopts)
+       python -m unittest $(subst /,.,$(@:.py=)) $(testopts)
 
 ifdef test
 all: status
-       python setup.py -q test -s $(subst /,.,$(test:.py=)).suite $(testopts)
+       python -m unittest $(subst /,.,$(test:.py=)) $(testopts)
 else
 all: help
 endif

However, there are test failures when running trac/wiki/tests/formatter.py, and no tests run when specifying trac/wiki/tests/__init__.py. Perhaps that is worth investigating more down the road.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t11437.7.

[Ryan J Ollos]

Committed to trunk in [13859:13860].

[Ryan J Ollos]

Replying to rjollos:

Side note: it might be useful to have a trac-admin $ENV config clean command to remove options from trac.ini that are no longer defined in an enabled Component.

Related idea in comment:15:ticket:6551.