Community
Participate
Working Groups
Support has been added to PDE Build to list the source roots of a source bundle. A new attribute 'roots' can be added to the Eclipse-SourceBundle property in the manifest file of source bundles. This attribute will contain a CSV list of roots where source is stored. '.' is the root of the bundle jar can be included in the list. If no roots are specified, '.' is the default. PDE UI needs to add the ability to read and store the explicit roots. This information can be used to improve importing of plugins by listing which folders need to extract from the jarred bundle. In addition we should make this information available to JDT so source bundles could contain overlapping namespaces. See bug 208171.
Created attachment 84101 [details] Work in progress
Created attachment 84240 [details] Work in progress II
Created attachment 84577 [details] Patch This patch needs a little bit more testing, but things are looking good so far.
Adding Brian for review so he has enough time before the milestone. I will test the patch some more tomorrow.
Created attachment 84681 [details] Patch for review This patch deals with all outstanding issues, import seems to work very well. I also added javadoc to a number of methods to improve usability. Brian, please review the patch and get it in before the milestone if possible.
We need to be a bit more conservative on caching in the BundleManifestSourceLocationManager. First, I believe fPluginToRootsMapping is not necessary. It seems the values are only referenced in getSourceRoots(..) which does not seem to be referenced by any code. Therefore, we can simplify the class by removing the global variable, the extra function, and reduce memory consumption by not creating a large HashMap. Also, we probably should not be storing the fLocationsToRootsMapping. This map is created when you parse a plug-ins source header. Since many people may never import the bundle, we should parse these headers lazily. This parsing should be fairly quick since the String value is stored in memory and we simply need to run it through ManifestElement. Since this operation should be short and the # of bundles which we are caching will be large, caching this information will not buy us much. To remove this, you have two functions which reference this map. getAllSourceRoots can be modified to take an IPluginModelBase so you can easily get the source header. The same can go for isValidSourceLocation, if you make it take a IPluginModelBase you can do a quick look up in fPluginToLocationMapping to see if it is in the cache. This will trickle down to PluginImportOperation, but I don't see a problem with getting the source location then calling manager.isBundleManifestLocation(IPluginModelBase) and then manager.getSourceRootsForBundleManifestLocation(IPluginModelBase). If you don't have time to make the changes, let me know and I will tweak it so we can get it into the build first thing next week. I am also reassigning the bug to you so you get credit for it when you go back through the bug reports :) I am adding myself to the CC list so I will be able to see updates on the bug.
(In reply to comment #6) > First, I believe fPluginToRootsMapping is not necessary. It seems the values > are only referenced in getSourceRoots(..) which does not seem to be referenced > by any code. Therefore, we can simplify the class by removing the global > variable, the extra function, and reduce memory consumption by not creating a > large HashMap. Oops, this is a mistake on my part. I am calling the wrong method when importing source archives. It currently does not make any difference to the result because there are no source bundles with multiple manifest entries (i.e. a bundle that provides source for multiple plugins or plugin versions). One method gets the source roots for the specific plugin/version. The other method is to provide all source roots in a source bundle. I should try and make this clearer in the method names. This information will also be passed to JDT once they have support for it. > Also, we probably should not be storing the fLocationsToRootsMapping. You are right, there is no advantage in caching it. > If you don't have time to make the changes, let me know and I will tweak it so > we can get it into the build first thing next week. I'm cc'ing Darin so he knows what's going on with this. I have one other bug I was looking at, but this needs to get in for the milestone. Not sure if it is better for you to work on it or me.
Created attachment 84807 [details] Patch for review Updated patch only has the one mapping, refactored a lot of the code, including method names and comments to make them more clear. Brian, please review as soon as possible.
Nice job Curtis. The last patch looked much better than the one prior, less code and more elegant. I applied the patch with a few changes. The first being that when importing with a binary link resource, the source was not attached correctly. Since the classpath code still expected a separate jar for each library, it was not able to find the one jar which should have attached to all libraries. The second modification was only importing source code that is referenced in the Manifest's Bundle-Classpath. I talked to Andrew and for the time being we are going to keep it this same way as we always have. If we want to change the way we import things, we can do that later. I am glad you were able to get this in for 3.4M4, it really finishes our new source bundle story. Thanks for doing all the work to move PDE up to this new source model!
Verified. I'm happy with the changes you made and every bundle I tested worked well.
