Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 330133

Summary:

[launcher] Add mechanism for ignoring user specified config.ini values

Product:

[Eclipse Project] Equinox

Reporter:

DJ Houghton <dj.houghton>

Component:

Launcher

Assignee:

DJ Houghton <dj.houghton>

Status:

RESOLVED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

john.arthorne, tjwatson

Version:

3.7

Target Milestone:

3.7 M4

Hardware:

OS:

Mac OS X - Carbon (unsup.)

Whiteboard:

Bug Depends on:

Bug Blocks:

330611

Attachments:

Description	Flags
patch	none
patch	none
Alternate patch	none

Description DJ Houghton

2010-11-12 14:48:09 EST

Tom and I were talking about the problem where we have a shared install and the user has values in their config.ini which cause the install to not load. (for instance after the root user has updated the shared install)

One approach to this would be to implement a mechanism where by the user config.ini values for certain properties could be ignored. 

We've agreed to the following approach:
- we will look for <propertyname>.override.user
- it can be set either in the config.ini from the shared location or by a -D parm on the command line
- if the override property is set (to be any value: getProperty() != null) then the value from the shared install area will override any value from the user config.ini
- if a property value is set via the command-line (e.g. -Dkey=value) then that value will not be over-written

Comment 1 DJ Houghton

2010-11-15 10:10:42 EST

Created attachment 183124 [details]
patch

Comment 2 Thomas Watson

2010-11-15 10:24:32 EST

DJ, when we discussed this approach there was a question on whether the *.override.user properties should show up in the merged configuration (destination).  The patch appears to not place the *.override.user properties into the destination.  Is that the intention?  I thought you were leaning towards including the *.override.user props in the destination properties.

Comment 3 DJ Houghton

2010-11-15 10:26:32 EST

Hmm... yeah I forgot about that. I'll change it. My argument at the time was that it will be easier to debug down the road if we have customer problems and we receive a system properties dump.

Comment 4 Thomas Watson

2010-11-15 10:51:20 EST

yeah, the other argument to keep the *.override.user properties is in case someone defined their own property that ended with .override.user.  The current patch would ignore them altogether.

Comment 5 DJ Houghton

2010-11-15 10:52:20 EST

Why would someone do that?? Serves them right! ;-)

Comment 6 DJ Houghton

2010-11-15 11:09:38 EST

I think another change to the patch should be to always let the command-line args win. Tom, what do you think?

With the current patch:
- user calls exe with -Dfoo=bar
- user config.ini has no entry for foo
- shared config has entry foo=anotherBar and foo.override.user=true

In this case the result will be foo=anotherBar.

Comment 7 Thomas Watson

2010-11-15 11:55:50 EST

(In reply to comment #6)
> I think another change to the patch should be to always let the command-line
> args win. Tom, what do you think?
> 
> With the current patch:
> - user calls exe with -Dfoo=bar
> - user config.ini has no entry for foo
> - shared config has entry foo=anotherBar and foo.override.user=true
> 
> In this case the result will be foo=anotherBar.

Yes, I think -D options should win.

Comment 8 DJ Houghton

2010-11-15 17:14:58 EST

Created attachment 183174 [details]
patch

Updated patch which keeps track of all the keys in the System properties before we start our modifications so we can check later to ensure we don't overwrite any values set on the command-line.

Comment 9 Thomas Watson

2010-11-16 11:20:01 EST

Created attachment 183238 [details]
Alternate patch

This patch avoids taking a snapshot of system property keys by passing the userConfiguration properties object to the mergeProperties for the override case (null is passed for the non-override case instead of false).

Comment 10 DJ Houghton

2010-11-16 15:16:03 EST

Comment on attachment 183174 [details]
patch

Ok, your patch is good. Thanks Tom.

Comment 11 Thomas Watson

2010-11-19 16:34:41 EST

I went ahead and released the latest patch to HEAD to get more testing before M4 is final.

Comment 12 Thomas Watson

2010-11-19 16:35:07 EST

set wrong milestone, sorry for the spam.