Community
Participate
Working Groups
The Region.installBundle(String url) method works only for file URLs. It should support any URL string for which there is a registered URL Handler. The current workaround is to use the installBundle(String, InputStream) method and create the InputStream yourself from the URL.
Here I think it is a mistake to build the policy into Region only to support file: or reference: protocols. The code seems to try and convert a file: URL into a reference: URL and then fail if the URL is not a file: or reference: URL to begin with. Personally I would remove this auto-magical conversion to reference: URL altogether and leave this policy decision in the hands of the installer.
The point of this restriction was so that Virgo could prepend the region name (plus "@") to the bundle location to ensure that the same bundle installed in distinct regions didn't clash bundle locations. The only way we found to do this was to open a stream and call: BundleContext.installBundle(String location, InputStream input) In order to open a stream, it appeared that we needed to require a file: or reference: URL - see the openBundleStream method. Is there a way to lift this restriction while still preventing bundle location clashes?
Created attachment 195980 [details] Proposed Patch This patch removes the policy of converting all URLs to reference: URLs when using the Region.installBundle(String) method. Any URL for which there is a registered URL Handler is now supported. This patch also adds a new test. I also tested this against the subsystems implementation where Maven URLs are used.
I reviewed the patch and I'm not sure I follow the new implementation of openBundleStream. Previously it was careful to produce a reference: URL before opening the stream. Will the new implementation work for all the URL schemes that the previous one coped with, particularly file:? Also the exception message below is not quite right: throw new BundleException("Location '" + location + "' resulted in an invalid bundle URI '" + location + "'", e); There's no point inserting location twice as that will only confuse.
(In reply to comment #4) > I reviewed the patch and I'm not sure I follow the new implementation of > openBundleStream. Previously it was careful to produce a reference: URL before > opening the stream. Will the new implementation work for all the URL schemes > that the previous one coped with, particularly file:? Perhaps I'm missing some history here because I don't understand the concern this question reflects. Is there something funky about the file: URL in conjunction with Region Digraph that required special consideration? Previously, only file: URLs were supported because everything was converted to reference: which only works with file: URLs. The patch should work with any URL for which a URL Handler exists, including reference: and file:. > Also the exception message below is not quite right: > throw new BundleException("Location '" + location + "' resulted in an invalid > bundle URI '" + location + "'", e); > There's no point inserting location twice as that will only confuse. I'll take care of this.
(In reply to comment #4) > I reviewed the patch and I'm not sure I follow the new implementation of > openBundleStream. Previously it was careful to produce a reference: URL before > opening the stream. Will the new implementation work for all the URL schemes > that the previous one coped with, particularly file:? Perhaps you were referring to the handling of file: URLs that represent directories? Unfortunately, the current patch will not handle this. Given a file: URL location string pointing to a directory, the patch will create a URL input stream from it and pass it along to bundleContext.installBundle(String InputStream), which is a no-no. In this case, the location string should be passed without creating a URL input stream. If an input stream must be passed, the file: URL must be converted into a reference: URL first, which is what the previous code did but with the unintended consequence that only file: URLs were supported. I think what we want to do is handle the file directory case but also support other protocols.
Yes, directories need to work otherwise Virgo will fail. If you want to build this fix on a branch with the directory support, I'll happily try it out with Virgo.
Created attachment 196153 [details] patch + test (In reply to comment #7) > Yes, directories need to work otherwise Virgo will fail. If you want to build > this fix on a branch with the directory support, I'll happily try it out with > Virgo. Glyn, if directories need to work shouldn't virgo use reference: URLs itself when installing bundles? Using reference under the covers builds the policy to not copy the jars from disk for all file: URLs. This is wrong. The decision to copy the content of the bundles into the framework cache or not should be a decision left in the hands of the installer IMO. At any rate, here is an alternative. I would prefer to encode the region identifier into the location in such a way that the location used to install the bundle at is still a valid URL. This patch uses an approach to place the region name as a fragment at the end of the original URL. For example, if you install the following location into a region named "regionX": http://server.com/bundles/thisRocks.jar Would translate into a location of: http://server.com/bundles/thisRocks.jar#regionX This way we can choose to simply pass the file: URLs through to the framework with the fragment #regionName tacked onto the end. The file: protocol handler ignores fragments and will allow the file to be opened as is. This allows the framework to handle the directory case correctly.
Hi Tom. I don't disagree with the proposed change, but since it affects the contract, we need to make a pre-req. change in Virgo so we don't cause Virgo to fail when we pick up this change. It looks as though we could make the pre-req. change early so I've raised Virgo bug 346625, which I will handle, to track this.
I went ahead and released the fix. I assume that you can delay picking up the new region impl until you have fixed Virgo. Let me know if that is not the case and I can back it out.
(In reply to comment #10) > I went ahead and released the fix. I assume that you can delay picking up the > new region impl until you have fixed Virgo. Let me know if that is not the > case and I can back it out. That should be fine. Thanks.
