forrest-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mathieu Champlon <m.champ...@free.fr>
Subject adding site-wide configuration files (was: [jira] Closed: (FOR-913) failure when ${project.home}/forrest.properties does not exist)
Date Mon, 07 Aug 2006 08:07:01 GMT
Ross Gardler a écrit :
> Mathieu Champlon wrote:
>>>
>>> Ross Gardler closed FOR-913.
>>> ----------------------------
>>>
>>>     Resolution: Invalid
>>>
>>> forrest.properties has nothing to do with ant. It provides 
>>> configuration values for forrest.
>>>
>>> A forrest.properties file is required.
> ...
>> What I was trying to explain is that a forrest.properties file seems 
>> indeed required while its content isn't.
>> If you seed a new project and remove everything from the 
>> forrest.properties file, it builds fine (using default configuration).
>> But if you remove the (now empty) file, the build breaks.
>>
>> In the context of several projects with an automated/centralized 
>> build process, it isn't very convenient to maintain a 
>> forrest.properties file for each project.
> ...
>> For example if the proxy configuration must be changed, it's much 
>> easier to change it in one place rather than going through every 
>> project.
> ...
>
> OK, that all makes sense. But how is this changed by having a blank 
> forrest.properties file?

It isn't.
The point was more something like : if the file can be blank then it
should probably be made optional.

> Then why not provide us with a patch? (again a rhetorical question, 
> keep reading, I just want to point out we need your help)
(...)
> I've applied a guard around the necessary code. It should now work 
> without a forrest.properties file.

Thanks !
I was actually hoping/waiting for the re-opening of the issue so I could
attach the patch.

> Perhaps your use case requries a new site-wide properties file. It 
> would be easier to manage than setting properties in your ant build 
> file and is easy to implement.

Yes, good point !

(...)
> It is also worth noting that in a future version of Forrest (probably 
> 0.9) we will be moving to a new XML property configuration system. 
> This provides much more flexibility than the existing system. If you 
> add a site-wide file please be sure to add an equivalent XML conf file 
> (loaded in the same code).

I took a look at the configuration management before adding this new
feature and I have a few refactoring suggestions :

1/ ForrestConfModule#getSystemProperty is never used and therefore
neither is defaultHome
2/ import java.io.FileNotFoundException; is not needed anymore

3/ configuration variables precedence order

It seems from reading ForrestConfModule#initialize that configuration
variables come from four different sources, in this order (the first
defined wins) :
. system properties
. xml configuration files
. property configuration files
. plugin xml configuration files

System properties are defined in the ant scripts as the properties
starting with either 'forrest.' or 'project.', including -D properties
from command-line and properties loaded from files with <property
file=.../>.
Therefore all configuration variables defined in property files loaded
by forrest.build.xml are accessible as system properties in
ForrestConfModule (well actually not all of them, because 'proxy' isn't,
but it's used only in ant scripts).
So there is no need to reload ${project.home}/forrest.properties in
ForrestConfModule#initialize.

Moreover, as the system properties are loaded at the end but replace all
other properties (in ForrestConfModule#loadSystemProperties),
configuration values from xml files are actually replaced by values from
property files.
So the actual precedence order is :
. command-line properties
. property configuration files
. xml configuration files
. plugin xml configuration files

What I suggest is to refactor ForrestConfModule#initialize so that it
clearly reflects all this, simplifying it at the same time, mostly by
removing the unnecessary code and re-arranging the loading order.
Among other benefits it also removes ant property files from
ForrestConfModule.

4/ ForrestConfModule#initialize is called twice

I don't exactly know why, it seems to be called from avalon, because
this method is actually an implementation of
org.apache.avalon.framework.activity.Initializable#initialize().
I haven't really looked further...

5/ there is no user.home forrest.properties.xml

There is a ${user.home}/forrest.properties loaded from the ant scripts,
but there is no corresponding xml configuration file. Is it on purpose ?
The user.home property from ant is not passed as system property because
it does not start with neither 'forrest.' nor 'project.', but it can be
specifically added.

6/ finally : adding site-wide properties files

Not much to say, only that the configuration files are only looked for
if a property named global.home exists (naming it site.home can be
confusing because 'site' has a special meaning in forrest already, so I
picked 'global').


Please tell me which modifications you think are worthwhile (and
possibly create an issue for this ?) and I will upload them for a step
by step patching.

MAT.



Mime
View raw message