Bug 356193 - Make config.ini properties configurable for surefire test
Summary: Make config.ini properties configurable for surefire test
Status: NEW
Alias: None
Product: z_Archived
Classification: Eclipse Foundation
Component: Tycho (show other bugs)
Version: unspecified   Edit
Hardware: All All
: P3 enhancement with 1 vote (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-30 09:06 EDT by Steffen Grunwald CLA
Modified: 2021-04-28 16:55 EDT (History)
5 users (show)

See Also:


Attachments
Patch as created from https://github.com/steffeng/sonatype-tycho/commit/edf17fcec9fcb62088a5ff931bd109edbf3e6ec2 (5.91 KB, application/octet-stream)
2011-10-14 17:56 EDT, Steffen Grunwald CLA
no flags Details
Commit based on previous patch but with JUnit tests (18.43 KB, patch)
2011-12-08 10:30 EST, Steffen Grunwald CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steffen Grunwald CLA 2011-08-30 09:06:14 EDT
I would like to run UI tests for an application that starts with
non-default startLevels.
My product's config.ini provides these lines as described in the
"system properties" section in [1]
osgi.startLevel=20
osgi.bundles.defaultStartLevel=20

During the surefire test, the config.ini is created with
defaultStartLevel 4, although I configured the system properties with:

<systemProperties>
       <osgi.startLevel>20</osgi.startLevel>
       <osgi.bundles.defaultStartLevel>20</osgi.bundles.defaultStartLevel>
</systemProperties>

The tycho code in [2] does not consider the system property in the
config.ini but sets the default startlevel to 4 and just appends the
system properties with -D[...].
The command line properties do neither take precendence nor are they
considered at all, if they don't have a command line equivalent in
[2].
It should be possible to configure these system properties in the config.ini.

As proposed in the mailing list I see these options:
a) put _known_ (runtime option) system properties (as described in [1]) not to the command line but to the config.ini
b) introduce a new configuration parameter (e.g. <configIni>...</configIni>) which allows to set anything in the config ini
c) like the p2 director does: reuse an existing config.ini and put all other parameters/defaults/bundles to it

[1] http://help.eclipse.org/indigo/topic/org.eclipse.platform.doc.isv/reference/misc/runtime-options.html
[2] https://github.com/sonatype/sonatype-tycho/blob/master/tycho-equinox-launching/src/main/java/org/eclipse/tycho/equinox/launching/internal/DefaultEquinoxInstallationFactory.java#L102
Comment 1 Steffen Grunwald CLA 2011-09-06 10:25:49 EDT
I created a branch [1] that makes the config.ini parameter configurable.
I've chosen the <configIniProperties> element to override/set properties in the resulting config.ini. Thus, it is easy to specify variables and parameters in the pom and very similar to the existing <systemProperties> configuration.
I was not fond of option a) as this would somehow involve hardcoding all runtime option system properties in the plugin.

[1] https://github.com/steffeng/sonatype-tycho/commits/eclipse-356193
Comment 2 Tobias Oberlies CLA 2011-10-11 04:32:41 EDT
Steffen, for legal reasons we are not allowed to take your patch from github. Could you please instead attach it as (git) patch to this bug report?
Comment 4 Tobias Oberlies CLA 2011-11-28 10:23:13 EST
Thank you for attaching the patch - it is pretty good.

I just have concerns about the semantics of the new interface method EquinoxInstallationDescription.addConfigIniProperties. In your patch, it allows to override any config.ini value, including the ones which are supposed to be controlled through the other interface methods. This either needs to be documented (and IMHO this doesn't work without repeating the implementation of DefaultEquinoxInstallationFactory) or changed.

Would it be sufficient for you, if addConfigIniProperties would only override values which are not set in DefaultEquinoxInstallationFactory (like "osgi.startLevel") or just have default values (like "osgi.bundles.defaultStartLevel")?
Comment 5 Steffen Grunwald CLA 2011-11-28 17:29:08 EST
Thanks for checking the patch.

> In your patch, it allows to override any config.ini value,

Yes, it is my interpretation of your idea to "allow to set anything in the config ini" [1] :-)

However, the following parameters are sufficient (among custom properties specific to my application):
- "osgi.startLevel"
- "osgi.bundles.defaultStartLevel"

Should I modify the patch or will you do it?

[1] http://dev.eclipse.org/mhonarc/lists/tycho-user/msg00705.html
Comment 6 Tobias Oberlies CLA 2011-11-30 04:45:32 EST
(In reply to comment #5)
> Yes, it is my interpretation of your idea to "allow to set anything in the
> config ini" [1] :-)
Seems like I had not thought this through fully. I only noticed that this is a bad idea when looking at the new method in EquinoxInstallationDescription

> However, the following parameters are sufficient (among custom properties
> specific to my application):
> - "osgi.startLevel"
> - "osgi.bundles.defaultStartLevel"
> 
> Should I modify the patch or will you do it?
I probably won't have time for this soon, so any help is appreciated. In particular, it would be good to have tests for the computation of the effective config.ini in DefaultEquinoxInstallationFactory to make sure the precedence (1st: specific methods like addBundle, 2nd: general method addConfigIniProperties, 3rd: default values) is done correctly.
Comment 7 Steffen Grunwald CLA 2011-12-08 10:30:06 EST
Created attachment 208097 [details]
Commit based on previous patch but with JUnit tests

Added JUnit Tests but had to restructure the InstallationFactory a bit to do it.
Patch is based on https://github.com/steffeng/sonatype-tycho/tree/eclipse-356193-2.
Comment 8 Tobias Oberlies CLA 2011-12-13 11:42:32 EST
(In reply to comment #7)
> Created attachment 208097 [details]
> Commit based on previous patch but with JUnit tests
This is very good - we really need to get this integrated now. Unfortunately, I still need you to answer the following legal-related questions for this:

1. Did you author 100% of the code (excluding refactorings of course)?
2. Who owns the copyright of new code? (This is typically your employer.)
3. Is the contributed code licensed under the EPL? (The test file didn't have a license header.)
4. Do you have the right to contribute the content to Eclipse? (You need to confirm this with the copyright owner, if this isn't yourself.)
Comment 9 Tobias Oberlies CLA 2014-12-15 05:32:01 EST
If someone should decide to pick up this bug, it is important to note that it must not used any of the code that we don't have the copyright clearance for.
So unfortunately we must not use anything of Steffen's patches but rewrite a fix from scratch.
Comment 10 Mickael Istria CLA 2021-04-08 18:14:05 EDT
Eclipse Tycho is moving away from this bugs.eclipse.org issue tracker to https://github.com/eclipse/tycho/issues/ instead. If this issue is relevant to you, your action is required.
0. Verify this issue is still happening with latest Tycho 2.4.0-SNAPSHOT
  if issue has disappeared, please change status of this issue to "CLOSED WORKFORME" with some details about your testing environment and how you did verify the issue; and you're done
  if issue is still present when latest release:
* Create a new issue at https://github.com/eclipse/tycho/issues/
  ** Use as title in GitHub the title of this Bugzilla ticket (may include the bug number or not, at your own convenience)
  ** In the GitHub description, start with a link to this bugzilla ticket
  ** Optionally add new content to the description if it can helps towards resolution
  ** Submit GitHub issue
* Update bugzilla ticket
  ** Add to "See also" property (up right column) the link to the newly created GitHub issue
  ** Add a comment "Migrated to <link-to-newly-created-GitHub-issue>"
  ** Set status as CLOSED MOVED
  ** Submit

All issues that remain open will be automatically closed next week or so. Then the Bugzilla component for Tycho will be archived and made read-only.