Comment 1 Sebastien Angers 2011-03-25 15:40:12 EDT

Bonjour,

got feedback from Anthony from his visit to Econ :-)  (I've asked him to bug anybody that could provide feedback on this).

So, while having interest to move to Tycho, some current Builds are based on PDE-Build and needs this to be fixed; so we can work on a contribution! but we are looking for feedback.

Since filtering out the optional dependencies would work for our use case, we would suggest to enable all the slicing options in the build.properties used by the PDE-Build process.  So the slicing options would not be hardcoded anymore in 
/org.eclipse.pde.build/pdebuildsrc/org/eclipse/pde/internal/build/P2ConfigScriptGenerator.java line 303 and would allow all the options.

What do you think about that suggestions?  Any comments/suggestions/guidance welcome!

Comment 2 Andrew Niefer 2011-03-25 16:11:54 EDT

(In reply to comment #1)
Hi Sébastien, adding options to control the slicing sounds reasonable to me.
There is PDE/Build bug 313334 that covers this topic, but it was just turning optional off.

We could add new properties like
p2.slicing.greedy
p2.slicing.optional

The generated slicing xml would be something like 
<slicing includeNonGreedy="${p2.slicing.greedy}" includeOptional="${p2.slicing.optional}" />
and we would need to generate some
<property name="p2.slicing.greedy" value="false" />
<property name="p2.slicing.option" value="true" />
with the default values for when the user does not set these properties.

Comment 3 Sebastien Angers 2011-03-25 17:07:16 EDT

(In reply to comment #2)

Great, thanks for the feedback, we will look into that direction and provide a contribution.

Regards,

Sébastien

Comment 4 Sebastien Angers 2011-04-20 20:09:00 EDT

Created attachment 193769 [details]
Proposed patch to add more (all) slicingOptions to Headless PDE Build

Bonjour,

as suggested in Comment 2, I'm submitting a patch that is enabling all the p2 mirror slicingOptions (customizable) the Headless PDE Build.

- Note 1)
    o From the code analysis, I've seen that slicingOptions support another option: latestVersionOnly (private boolean latestVersion = false;)
        - See /org.eclipse.equinox.p2.repository.tools/src/org/eclipse/equinox/p2/internal/repository/tools/SlicingOptions.java 
        - Should it be made publicly available?  If yes, 
            - I could add it to http://wiki.eclipse.org/Equinox/p2/Ant_Tasks#SlicingOptions
            - I have already added support to it in the proposed patch for the headless mode.

- Note 2)
    o From the code analysis, I've also seen that slicingOptions support another option: installTimeLikeResolution (private boolean resolve = false;)
        - See /org.eclipse.equinox.p2.repository.tools/src/org/eclipse/equinox/p2/internal/repository/tools/SlicingOptions.java 
        - Should it be made publicly available?  If yes, 
            - We should add it to http://wiki.eclipse.org/Equinox/p2/Ant_Tasks#SlicingOptions
            - There's no Bugzilla reference in the CVS commit...
            - I have NOT added support to it in the proposed patch for the headless mode.

- Note 3) 
    o Please review with attention the way the slicingOptions are transferred from "Ant" level Properties to "Java" level Properties (a bit verbose :-()
        -  In org\eclipse\pde\internal\build\tasks\BuildScriptGeneratorTask.java, they are added to Java level Properties.  They are only added to Java Properties if not null.  getImmutableAntProperty is used instead of getPropertyAsBoolean so the value is not set if null (otherwise, by using getPropertyAsBoolean it would be set to false when the slicingOption is null in build.properties, which could be different than the default value we want).     
        -  In org\eclipse\pde\internal\build\P2ConfigScriptGenerator.java, they are consumed and if they were null in build.properties, default values are set here.

- Note 4)
    o The set of options supported by slicingOptions have been added to the build.properties
    o Default values have been set trying to fit most common PDE Build scenario:
	p2.mirror.slicing.filter=
	p2.mirror.slicing.followOnlyFilteredRequirements=false
	p2.mirror.slicing.followStrict=false
	p2.mirror.slicing.includeFeatures=true
	p2.mirror.slicing.includeNonGreedy=false
	p2.mirror.slicing.includeOptional=false
	p2.mirror.slicing.platformFilter=
	p2.mirror.slicing.latestVersionOnly=false
    o These default values are different from the ones for the slicingOptions (documented http://wiki.eclipse.org/Equinox/p2/Ant_Tasks#SlicingOptions) but they are the ones that (should) fit in a common PDE Build scenario.  
    o Please comment on these default values and I will change them accordingly (or change them directly while applying the patch :-))  They must be changed in the build.properties and in the org/eclipse/pde/internal/build/P2ConfigScriptGenerator.java


- Note 5)
    o Notes on SlicingOptions during Product Export from GUI vs Headless:
        - Note that the Product Export from GUI does not include any GUI options to set the slicingOptions (as it will now be possible with headless/Ant build.properties)
            • Because of this and based on Bug 313334 Comment 4 & reuse of the patch Attachment 169899 [details], it is proposed in the attached patch to hardcode the value to false for the following 2 slicingOptions:
                o p2.mirror.slicing.includeNonGreedy
                o p2.mirror.slicing.includeOptional
            • The other 6 slicingOptions are not set in the case of an Export from GUI (they are using their default value).
            • A better solution would be to add these options to the PDE Product Export GUI for more control of these options (not covered by this current Bug/Patch).


- Notes 6)
    o This proposed patch will provide better control on the Headless PDE Build.
    o We have tested it on few Builds already and works fine.  Since it filters out the optional dependencies, we don't get the problem with "folders" generated in the p2 repo anymore (reason of this Bug entry).
    o This patch is based on trunk, but has also been tested successfully with tag R3_6_1 (only the build.properties that needed some tuning).


Thanks for your review & feedback.

Comment 5 Andrew Niefer 2011-04-25 16:47:55 EDT

Created attachment 194023 [details]
Modified patch

For this patch, I see two main questions:

1) How to apply defaults.  
If the user does not specify a value, should PDE (a) not set a value and let p2 choose the default, or (b) always set a value and PDE chooses the default.  

In the proposed patch we are doing (b), PDE is setting a value and the default may or may not match p2's default.  I think I agree with this choice.  It lets PDE choose defaults that make sense for it, and it insulates us in case p2 decides to change its defaults in the future.

2) How to generate the actual script.  
There are two choices here, (a) generate the actual value directly into the script (value is resolved at script generation time), or (b) generate a property into the script (value is resolved at ant runtime)  ie:
(a)  followStrict="true" 
(b)  followStrict="${p2.mirror.slicing.followStrict}"

Method (a) requires knowing the value and is a bit verbose in the code changes.  The proposed patch kind of mixes (a) and (b).

I suggest method (b), we don't need to read the properties from the ant project, instead we can just generate <property> elements with default values (since ant properties don't change, the user's settings override these defaults).



RE Note 1: We should include latestVersionOnly, it is listed in the docs at  http://help.eclipse.org/topic//org.eclipse.platform.doc.isv/guide/p2_repositorytasks.htm

RE Note 2: I am in favour of omitting installTimeLikeResolution, it appears to invoke the planner instead of the slicer.  This could be a major change for build, I'm not sure what the implications would be.

RE Note 3: This goes away with point (2) above, we don't need to read the properties at all.

RE Note 4 & 5: 
We differ from p2 for the default value of includeNonGreedy.  
Regarding includeOptional, previous headless PDE behaviour is includeOptional=true.  Bug 313334 proposed setting UI export to use false.
The new patch here has "true" for headless builds and "false" for UI export.


These changes all seem to me to address Comment #0 "Question 2)" which, while certainly useful, seems like a workaround for "Question 1)".
I think we need to consider the <p2.mirror> "raw" attribute. PDE uses the default which is "true".  I think that setting this to "false" will result in the folders being mirrored into jars. (Although we should verify this).

I think that in addition to the changes we already have here, we should do another patch to allow setting the raw attribute, and also we should consider changing PDE's default for this to "false".

Comment 6 Sebastien Angers 2011-05-02 16:07:17 EDT

Created attachment 194539 [details]
Modified patch to add more (all) slicingOptions and raw attribute to Headless PDE Build

(In reply to comment #5)

Thanks Andrew for the -quick- review/feedback!

Regarding your 2 Questions:
- 1) How to apply defaults
        -> I agree with (b): always set a value and PDE chooses the default.  For the same reasons you mentioned.
- 2) How to generate the actual script. 
        -> I agree with your modified patch where we just generate <property> elements with default values.  In fact, I had implemented it that way at some point, but wasn't sure what was making it most clean/clear; so good modification!


Regarding your answers to the Notes:
- RE Note 1: I'll add latestVersionOnly option to the following wiki page http://wiki.eclipse.org/Equinox/p2/Ant_Tasks#SlicingOptions

- RE Note 2: Will keep installTimeLikeResolution option out for now.

- RE Note 3: Agreed.

- RE Note 4 & 5: Agreed.


Regarding your last notes on using -raw attribute to fix the "folder" problem at the root, that's a great suggestion; I did not know about the behavior of this property, but it works as you mentioned.  So the new suggested patch contains that new attribute ("raw") in addition to the previous patch.  

***Note that I've defaulted the value of the 'raw' attribute to 'false', since it seems to make sense for the default mirror behavior***  Should we keep default to 'true' since that's the current behavior/default?

By the way, there seems to be a bug in org/eclipse/equinox/p2/internal/repository/tools/MirrorApplication.java since the value of the "raw" attribute can't be changed; default value is "true" and in initializeFromArguments(String[] args) we have

   if (args[i].equalsIgnoreCase("-raw")) //$NON-NLS-1$
				raw = true;

so, it can only (not) change from "true" to "true".  So, it seems that there's no way to use "false" from MirrorApplication at command line. (maybe there should be property -falseRaw (just kidding)).

Thanks again for review and constructive feedback!

Comment 7 Sebastien Angers 2011-05-06 14:42:17 EDT

As indicated in Comment 6, I've updated the wiki page http://wiki.eclipse.org/Equinox/p2/Ant_Tasks#SlicingOptions to include the "latestVersionOnly" slicing option that was already available.

That option was not documented on the wiki but was documented in the help docs (http://help.eclipse.org/helios/index.jsp?topic=/org.eclipse.platform.doc.isv/guide/p2_repositorytasks.htm)

Comment 8 Andrew Niefer 2011-05-06 16:11:36 EDT

Patch released.  Thank you Sébastien.  I raised bug 345013 regarding setting raw=false on the command line mirror application.

Comment 9 Sebastien Angers 2011-05-06 16:13:10 EDT

(In reply to comment #8)

Great!, Thanks Andrew for the review & release.

Comment 10 Andrew Niefer 2011-05-24 10:52:37 EDT

*** Bug 313334 has been marked as a duplicate of this bug. ***