Bug 183924 - baseLocation should also support configuration with platform.xml
Summary: baseLocation should also support configuration with platform.xml
Status: RESOLVED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: Build (show other bugs)
Version: 3.3   Edit
Hardware: PC Windows XP
: P3 enhancement (vote)
Target Milestone: 3.3 M7   Edit
Assignee: pde-build-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2007-04-25 02:08 EDT by Conan Chan CLA
Modified: 2007-04-27 16:52 EDT (History)
1 user (show)

See Also:


Attachments
Patc for org.eclipse.pde.build (6.41 KB, patch)
2007-04-25 02:16 EDT, Conan Chan CLA
no flags Details | Diff
org.eclipse.pde.build patch based on HEAD (6.37 KB, patch)
2007-04-25 13:19 EDT, Conan Chan CLA
no flags Details | Diff
Correct format of the patch from HEAD (8.90 KB, patch)
2007-04-26 17:49 EDT, Conan Chan CLA
no flags Details | Diff
Patch including the changes proposed above (16.93 KB, patch)
2007-04-27 10:04 EDT, Pascal Rapicault CLA
no flags Details | Diff
Extra Patch on top of Pascal's change (3.71 KB, patch)
2007-04-27 12:54 EDT, Conan Chan CLA
no flags Details | Diff
New patch addressing some concerns with looking too deep (17.02 KB, patch)
2007-04-27 13:14 EDT, Pascal Rapicault CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Conan Chan CLA 2007-04-25 02:08:53 EDT
Supporting just features/plugins and links directory is not flexible enought.
with Eclipse 3.x uses platform.xml to control what plugin to be loaded.

baseLocation should also support configuration with platform.xml.
Comment 1 Conan Chan CLA 2007-04-25 02:16:15 EDT
Created attachment 64829 [details]
Patc for org.eclipse.pde.build

3 files that I change to have it support..
Please review and see if it can be commited into the base.

I have ran test against my product build to confirm the old ways  still worked, as well as using platform.xml
Comment 2 Pascal Rapicault CLA 2007-04-25 08:27:25 EDT
This is a fine idea. However before tackling this we should see how the new provisioning work from equinox evolve.
Comment 3 David Olsen CLA 2007-04-25 10:20:02 EDT
I don't know if it should be covered by this bug or a new one, but the IDE should also support a target (Preferences > Plug-in Development > Target Platform) that uses plaform.xml.  I don't know whether this patch addresses that situation, but I assume not since the files changed have "build" in their name.

Why is this affected by the ongoing Equinox provisioning work?
Comment 4 Pascal Rapicault CLA 2007-04-25 11:56:15 EDT
David, I do believe that when pointing the target platform to an eclipse install containing a platform.xml PDE does the proper thing. Some work went into this in 3.2 to enable scenarios where pools of plug-ins are being shared across multiple configurations. See Nalini's bug in PDE UI.

This would be affected by the provisioning work because the platform.xml is an update artifact that will unlikely be carried on into the new story with its current shape.
Comment 5 Pascal Rapicault CLA 2007-04-25 12:00:08 EDT
Hum... I had missed the fact that there was a patch attached to this bug.... I will take a look. Conan, could you please attach an actual patch against HEAD? That would greatly facilitate the review and gaging the scope of the change.
Comment 6 Conan Chan CLA 2007-04-25 12:08:50 EDT
will do.
Comment 7 Conan Chan CLA 2007-04-25 13:19:04 EDT
Created attachment 64896 [details]
org.eclipse.pde.build patch based on HEAD
Comment 8 Conan Chan CLA 2007-04-26 17:49:57 EDT
Created attachment 65120 [details]
Correct format of the patch from HEAD
Comment 9 Pascal Rapicault CLA 2007-04-26 19:31:55 EDT
The patch looks good and the fact that most of the code is coming from PDE Core makes me confident that it could be released for 3.3.

There is just one thing that bugs me: given that all the callers of PluginPathFinder.getPluginPaths() eventually convert the returned URLs into Files, why can't getPluginPaths directly return a File[].
By doing this change, it would avoid to have two findPluginXML in BuildTimeSiteContentProvider (note that findPluginXML(String[]) can be changed to findPluginXML(File[]).

Conan could you please do those change and attach a new patch for tomorrow friday noon?
Comment 10 Pascal Rapicault CLA 2007-04-26 19:32:21 EDT
Tentatively marking for 3.3.
Comment 11 Conan Chan CLA 2007-04-26 22:50:13 EDT
No, Can be done but it is not an easy.
I have to make a different handling of the URL[] in BuildTimeSiteContentProvider because getPlugsPath  in URL[] is different then String[]..
You request basically required me to start to the beginning to do all the testing again..

Beside, keeping the PluginPathFinder close to the pde.core one make it easier to move up with the pde.core version I would think..
Comment 12 Conan Chan CLA 2007-04-26 22:56:06 EDT
I tried.. but I don't think it is as simple as making a converter from URL[] to String[]..  And I think what I have changed in the patch is way better solution..  

You to decide..
Comment 13 Pascal Rapicault CLA 2007-04-27 10:04:45 EDT
Created attachment 65214 [details]
Patch including the changes proposed above

Conan, I have made the changes. Here is a patch, please verify that it works in your environment. Thx.
Comment 14 Conan Chan CLA 2007-04-27 12:54:52 EDT
Created attachment 65230 [details]
Extra Patch on top of Pascal's change
Comment 15 Pascal Rapicault CLA 2007-04-27 13:14:34 EDT
Created attachment 65236 [details]
New patch addressing some concerns with looking too deep
Comment 16 Pascal Rapicault CLA 2007-04-27 16:08:41 EDT
Patch released in HEAD.