forrest-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ross Gardler <rgard...@apache.org>
Subject Re: adding site-wide configuration files (was: [jira] Closed: (FOR-913) failure when ${project.home}/forrest.properties does not exist)
Date Mon, 07 Aug 2006 09:45:37 GMT
Mathieu Champlon wrote:
> Ross Gardler a écrit :
> 
>> Mathieu Champlon wrote:

...

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

OK, I get it. Well I kind of got it before, hence my patch.

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

Well, like you said it was real simple, almost as easy to do the change 
as it was to open the issue again. Sorry I missed this in your post 
though, help is always appreciated, I should have left it for you to do ;-)

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

OK.

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

That all sounds fine by me.

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

I have nothing to add to this. I didn't realise it was called twice. 
Since this is a Cocoon input module, then this is a Cocoon thing. So we 
need to take it up with them once we have a little more understanding of 
why it is happening.

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

This is not on purpose. The forrest.properties.xml was added for the 
specific purpose of allowing plugins to extend the configuration options 
available in the sitemap without having to touch core.

It is still experimental (i.e. incomplete) code, so adding this feature 
would be a good idea.

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

Sounds fine.

We look forward to your patches. Thanks for your contribuion.

Ross


Mime
View raw message