Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 328929 - [target] Would you like source with that?
Summary: [target] Would you like source with that?
Status: VERIFIED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.7   Edit
Hardware: PC Mac OS X - Carbon (unsup.)
: P3 enhancement (vote)
Target Milestone: 3.7 M4   Edit
Assignee: Curtis Windatt CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed, noteworthy
Depends on:
Blocks:
 
Reported: 2010-10-28 10:08 EDT by Jeff McAffer CLA
Modified: 2010-12-07 16:13 EST (History)
7 users (show)

See Also:


Attachments
patch to include source (12.13 KB, patch)
2010-10-28 13:25 EDT, Jeff McAffer CLA
no flags Details | Diff
Updated patch (27.25 KB, patch)
2010-10-28 23:39 EDT, Jeff McAffer CLA
no flags Details | Diff
Even better version, now with slicing... (28.87 KB, patch)
2010-10-29 00:17 EDT, Jeff McAffer CLA
no flags Details | Diff
consolidated patch (101.90 KB, patch)
2010-11-01 15:56 EDT, Jeff McAffer CLA
no flags Details | Diff
Consolidated now with update (113.51 KB, patch)
2010-11-01 20:59 EDT, Jeff McAffer CLA
no flags Details | Diff
Reviewed Mass Patch (108.85 KB, patch)
2010-11-05 11:53 EDT, Curtis Windatt CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeff McAffer CLA 2010-10-28 10:08:00 EDT
it would be awesome if users could pick a feature, add it ot the target and say they want source with that.  Currently the producing team has to create and publish source features for this purpose.  This can be a problem as they factor their world into interesting runtime pieces.  They might make 100 features to suit different runtime scenarios.  making 100 source features is clutter.  Rather, PDE could have a check box (whatever) that asked the user "Would you like source with that?".

I'll work on a patch to support this capability and attach it here presently.
Comment 1 Hugues Malphettes CLA 2010-10-28 10:49:56 EDT
+1.
In fact we have been doing this with our custom packaging of p2-director for the command-line.
I would like it even more if this was something available out of the box with the bare bone p2-director application.

Scott asked for this back in June on p2-dev.
Pascal answered it was not done and as the p2-repo don't explicitly publish the relationship between a bundle and a source bundle we would have to use naming conventions to identify those.

Those ideas put into code (all EPL, forked from eclipse code and coded by myself cleanly)
http://github.com/intalio/org.eclipse.equinox.p2.director.extended/blob/master/plugins/org.eclipse.equinox.p2.director.extended/src/org/eclipse/equinox/p2/director/extended/internal/AddSourcesRequirementsHelper.java
.... there is a trivial bug just now : not checking the actual version on the source match the runtime's version and it has created some surprises.

And here is where it is 'inserted' in the plan:
http://github.com/intalio/org.eclipse.equinox.p2.director.extended/blob/master/plugins/org.eclipse.equinox.p2.director.extended/src/org/eclipse/equinox/p2/director/extended/internal/ForkedSimplePlanner.java#L382

I can look into PDE's use of the Planner and Slicer and figure out if something similar would work there.
Let me know if that is worth putting into a patch here.
Comment 2 Jeff McAffer CLA 2010-10-28 11:11:22 EDT
this bug relates to the PDE UI workflows.  the p2 director changes you propose should go in a p2-related bug.  I can both see the usefulness of what you want but can also see the p2 team not wanting to encode OSGi and Eclipse specific things in the relatively generic director app.

I'll have a patch for the PDE UI function hopefully later today.
Comment 3 Jeff McAffer CLA 2010-10-28 13:25:59 EDT
Created attachment 181966 [details]
patch to include source

this patch adds a new fIncludeSource field to the IUBundleContainer along with the corresponding setters/getter.  If set, then the available source bundles corresponding to the indicated binary bundles are added to the target.  

Remaining issues:
- UI positioning. This should likely surface as a "Automatically add available source" check box on the container page as it is not really a target-wide concept (dir containers don't have this option for example).

- when the planner is being used we end up two whole provisioning operations.  Once to get the real bundles that have been requested into the profile, then again add the corresponding source bundles.  It may be possible to walk the profile and the plan and figure this out without running the planner again.  I'm not smart enough.

- similarly, in the slicer case, we run the slicer twice. Note here we do not run the engine twice so the cost is not that high.

- Progress monitoring is not the best
Comment 4 Jeff McAffer CLA 2010-10-28 23:39:50 EDT
Created attachment 182007 [details]
Updated patch

New patch, now with UI integration. There is a new check box on the Edit UI page.  Its setting is persisted withthe dialog setting an the Target Persistence helper for 36 has been updated to persist the includeSource setting.  Not sure if we have to make a new persistence helper for 3.7...

Looking at how this all worked there appears to be an underlying issue in the IUBundleContainer. It looks like nothing is ever removed from the profiles. In fact, the engine is run withOUT the UNINSTALL phase.  There must be something I'm missing from the profile lifecycle.  The workaround in the code is to always slice the profile at the end based on the feature root set. This causes problems for the source IU as mentioned earlier.  

In this patch I changed all the resolve* methods to use profile queries to get the set of resolved features and bundles.  The net effect is that the profiles need to accurately model the container.  This is, IMHO, a good thing and enables future enhancements like having products as roots in targets (why not?!).  

In any event, I managed the removal of root IUs in the planner case but have not yet done it in the slicer case (more manual).

One thing I did notice is that the p2 GC potentially runs at the end of every provisioning operation.  That means that as you click around in the target editor and add/remove things from your list of features in a site, the GC is merrily running and removing things.  The net effect is that you have to redownload them.  grrrr.  We can control the GC but since we're using the global p2 agent, turning it off (for example) turns it off for everyone.  Another reason to use have a PDE p2 Agent.
Comment 5 Jeff McAffer CLA 2010-10-29 00:17:54 EDT
Created attachment 182009 [details]
Even better version, now with slicing...

OK, so I couldn't stand slicing not working properly.  Here is a new patch that includes proper profile management with planning and slicing.  The GC comments still remain...
Comment 6 Curtis Windatt CLA 2010-10-29 10:23:59 EDT
(In reply to comment #4)
> New patch, now with UI integration. There is a new check box on the Edit UI
> page.  Its setting is persisted withthe dialog setting an the Target
> Persistence helper for 36 has been updated to persist the includeSource
> setting.  Not sure if we have to make a new persistence helper for 3.7...

I haven't looked at the patch yet, but I assume that the the setting is getting stored as an xml attribute on the container element (Same as include_mode and include_all_platforms).  I'm tempted to avoid making another version of the file for a new attribute.  While the behaviour will differ between 3.6 and 3.7 for the same file, using a new file with the new attribute in Eclipse 3.6 will not break anything as the attributes are looked up directly. i.e. the 3.6 persistence helper won't get confused by the extra attribute.

> Looking at how this all worked there appears to be an underlying issue in the
> IUBundleContainer. It looks like nothing is ever removed from the profiles. In
> fact, the engine is run withOUT the UNINSTALL phase.  

The profiles started as a temporary place to put the IUs (the planner needed a profile).  We didn't want to manage the contents of the profile, instead we just threw it away on each resolve.  There is more use for a managed profile now.  I'll have to look at your patch in more detail to see what changes were made and how they might affect things.

> One thing I did notice is that the p2 GC potentially runs at the end of every
> provisioning operation.

I updated bug 328928.
Comment 7 Jeff McAffer CLA 2010-10-29 11:44:32 EDT
(In reply to comment #6)
> I haven't looked at the patch yet, but I assume that the the setting is getting
> stored as an xml attribute on the container element (Same as include_mode and
> include_all_platforms).  I'm tempted to avoid making another version of the
> file for a new attribute. 

Exactly.  From the user point of view the only thing will be their source will not appear if using a "new" target in 3.6.  But there's nothing we can really do about that anyway.  Unfortunately if you open a new target with 3.6 and save, the attribute is likely lost.

> The profiles started as a temporary place to put the IUs (the planner needed a
> profile).  We didn't want to manage the contents of the profile, instead we
> just threw it away on each resolve.  There is more use for a managed profile
> now.  I'll have to look at your patch in more detail to see what changes were
> made and how they might affect things.

Figured as much.  With the most recent patch a profile, if there and up to date, should match the target exactly.  This will also enable the GC to do a better job.
Comment 8 Jeff McAffer CLA 2010-10-31 12:25:35 EDT
See a related though not prerequisite enhancement in PDE Build-land (bug 329162) to have source bundles generated automatically.  That enhancement makes it easier for people to get the source in the repos which makes this enhancement more interesting.
Comment 9 Jeff McAffer CLA 2010-11-01 15:56:21 EDT
Created attachment 182171 [details]
consolidated patch

This is a consolidated patch of bugs 

Bug 327668	target provisioning should use local artifact repos
Bug 328928	[target] use a unique p2 agent
Bug 328929	[target] Would you like source with that?

which all overlap.  The bugs are independent and their respective patches can be applied (or not) as the PDE team chooses.  

This patch does have a few fixes and improvements that were discovered while doing the consolidation so if all three bugs are going to be done, this is the best patch to use.  In particular the following things were found/changed.
- updated P2TargetUtils to correct some issues in the way the agent was being referenced
- fixed problem in IUBundleContainer in profile artifact repo discovery
- Turned off GC and added a preference button to "cleanup" (i.e., call GC)
- updated the test suite 
- bit o' doc/comments here and there.
Comment 10 Jeff McAffer CLA 2010-11-01 20:59:01 EDT
Created attachment 182185 [details]
Consolidated now with update

I forgot a patch in the consolidation process. 
Bug 274212	[target] Update button in the target provisioner

This new version of the consolidated patch has support for an Update button on the Target location list.  Users can select either a container or individual (or group of) IUs to be updated.  See Bug 274212 for remaining of topics.  The consolidation was largely a straight application of the original patch but a few improvements were made along the way.
Comment 11 Curtis Windatt CLA 2010-11-02 14:29:57 EDT
Still just starting to review.  I tweaked the UI a bit, but the feature is pretty sweet.
Comment 12 Curtis Windatt CLA 2010-11-05 11:53:49 EDT
Created attachment 182492 [details]
Reviewed Mass Patch
Comment 13 Curtis Windatt CLA 2010-11-05 12:34:20 EDT
Fixed in HEAD.  Thanks for all the great work Jeff!
Comment 14 Alfred Schmid CLA 2010-11-16 04:57:12 EST
Is it possible to provide this fix to Eclipse 3.6.2? I have the problem, that the target-platform cache bloats my disk drive and I don't find an easy way to cleanup this cache.
Comment 15 Curtis Windatt CLA 2010-11-16 10:25:09 EST
(In reply to comment #14)
> Is it possible to provide this fix to Eclipse 3.6.2? I have the problem, that
> the target-platform cache bloats my disk drive and I don't find an easy way to
> cleanup this cache.

The majority of the target changes are new enhancements and will not be put into a maintenance release.  There were not any significant changes to garbage collection in this patch.  Can you point to the specific change that fixes your issue.
Comment 16 Alfred Schmid CLA 2010-11-17 04:26:59 EST
I'm referencing to Jeff McAffers point:

- Turned off GC and added a preference button to "cleanup" (i.e., call GC)

As far as I understand this adds a button to preferences to cleanup the 

'Z:\Entwicklung\eclipse\workspace\eclipse_3.6.1\.metadata\.plugins\org.eclipse.pde.core\.bundle_pool'

In my case this directory contains gigabytes of old bundles and I'm searching for an easy cleanup for them.
Comment 17 Jeff McAffer CLA 2010-11-17 04:55:04 EST
You can trigger a GC by adding a target platform definition and then removing it.  The act of removing a def on the prefs page triggers the GC.  Try that then look at bundlepool.  should be smaller.
Comment 18 Alfred Schmid CLA 2010-11-17 05:29:14 EST
Yes, I know, that removing of target in prefs triggers a GC. But then I've the problem, that my target definition xxx.target get's deleted also. I tried also to unselect the checkbox item of the target definition in the preferences and apply these settings, but this doesn't seem to trigger the GC. In my case the directory content stays the same :(
Comment 19 Curtis Windatt CLA 2010-12-07 16:13:03 EST
Cool new feature.  Added to N&N.  Verified in I20101206-1800