Community
Participate
Working Groups
Historically, we have had problems with the handling of UNC paths. We should ensure that these are properly supported in various places of p2.
I had a problem with UNC paths. I started my workspace with a UNC path and launched the director app that generated the following config.ini: #Configuration File #Tue May 13 15:25:28 EDT 2008 osgi.bundles=reference\:file\://.PSF/.Home/workspace/Code9/workspaces/p2/org.eclipse.equinox.simpleconfigurator@start osgi.bundles.defaultStartLevel=4 osgi.install.area=file\:C\:\\EclipseWindows osgi.framework=file\:C\:/EclipseWindows/plugins/org.eclipse.osgi_3.4.0.v20080427-0830.jar org.eclipse.equinox.simpleconfigurator.configUrl=file\://.PSF/.Home/workspace/Code9/workspaces/p2/.metadata/.plugins/org.eclipse.pde.core/director app/org.eclipse.equinox.simpleconfigurator/bundles.info osgi.configuration.cascaded=false the simpleconfigurator failed to install any bundles (ss at the osgi console showed only the osgi and simpleconfigurator installed). I ran eclipse with the same workspace but with a normal windows path for the workspace. Launching the director app generated the same config.ini but with normal windows paths for the configUrl property (e.g. file:z:\...). This configuration worked.
Created attachment 101600 [details] patch The following coding pattern is bad for UNC paths String uncPath = "//remote/directory/file"; URL url = new URL(uncPath); File uncFile = new File(url.getFile()); When a UNC file URL is constructed this way the URL.getFile() will return a String which is missing an important beginning '/'. In the above example "/remote/directory/file" is returned from url.getFile(). In the framework we construct file: URLs like this to work around this issue if (spec.startsWith("file:")) // need to do this for UNC paths return new File(spec.substring(5)).toURL(); We use the File.toURL to "correctly" construct the UNC URL. Some will say it is incorrect because this method does not encode the resulting URL path, but simple configurator was not encoding the file URLs anyway and AFAICT none of the code would handle the URLs being encoded anyway. This patch adds a new method to Utils to build file URLs. I noticed that this class says it was copied from frameworkadmin. I'm not sure how many other places p2 has the above coding pattern, but framework admin probably should be investigated.
*sigh* In the above bad code pattern the string should have had a "file:" on it String uncPath = "file://remote/directory/file"; URL url = new URL(uncPath); File uncFile = new File(url.getFile()); I think this fix is pretty important for 3.4.
I agree that the fix is important for 3.4 and also agree that we should push it out to RC3. We just dont' have enough time to get enough eyes on it for RC2. Tom, do you have time to look at creating a patch that covers all the areas needed?
(In reply to comment #4) > Tom, do you have time to look at creating a patch that covers all the areas > needed? > I will do a scan on the p2 projects to see how bad it is. I am going to add a new method to org.eclipse.equinox.internal.p2.core.helpers.URLUtil to use instead of new URL(String)
Created attachment 101816 [details] simpleconfigurator patch This is an updated patch to the previous one. I updated the buildURL method to throw an NPE when the spec==null just like new URL(String) does.
Created attachment 101818 [details] frameworkadmin patch This patch adds the same buildURL method to the frameworkadmin code to replace the new URL(String) calls. This patch also includes changes to org.eclipse.equinox.simpleconfigurator.manipulator which has depends on org.eclipse.equinox.frameworkadmin and not org.eclipse.equinox.simpleconfigurator (kinda strange).
Created attachment 101821 [details] p2 patch Here is the big one. This patch adds the same buildURL method to the org.eclipse.equinox.internal.p2.core.helpers.URLUtil class. This method should be used instead of any call to new URL(String) where the string may be a file: URL. It also fixes URLUtil.toURI(URL) and URLUtil.toFile(URL) to properly handle UNC paths. This is where things get even more tricky. URLs that represent UNC paths generally have a proceding '//' (e.g. file://machine/shared/file.txt) when converted to external form. But this form does not work well for URIs. A UNI that represents UNC paths must have a proceeding '////' (e.g. file:////machine/shared/file.txt). Otherwise the URI will use the first segment (machine) as the authority/host. This causes something like the following to fail. File uncFile = new File("//machine/shared/file.txt"); URL uncURL = uncFile.toURL(); File fileFromURI = new File(new URI(uncURL.toExternalForm())); This is because the external form of the URL will only have two '/' chars proceeding the path causing the URI to use the first segment as the authority/host. The new File call will fail because it only will accept URIs that have the file protocol and have no authority or host. To fix this I added a new method in URLUtil to correctly externalize URLs representing UNC paths when using them to contruct URIs. Note that using the multi-argument URI constructor seems to work fine when passed all the information from a URL object representing a UNC path. We may want to consider always using that approach instead of converting the URL to an external form to create a URI out of it. Something like this ... new URI(url.getProtocol(), url.getUserInfo(), url.getHost(), url.getPort(), url.getPath(), url.getQuery(), url.getRef()); Currently we do this in the exception case when the url has unencoded characters. It may be better to just always use this approach instead.
Thanks Tom. All, what are we going to do with this? 3.4? UNC is, unfortunately used by quite a few people as we have seen in the past. Is there a minimal set of changes that we can do to enable the base usecase? we were able to find a workaround in the case of Parallels on the Mac but I suspect that that is not the mainline usecase that people have problems with...
(In reply to comment #9) I'm not sure how isolated of a fix we can do. We will likely leave out some path that we did not think of if we try to cover some well known code paths. My 2 cents: - I am most confident in the simpleconfigurator patch. This patch enables self-hosting when the workspace is located at a UNC path. I think this is a pretty common usecase and one that we can fix by only releasing the simpleconfigurator patch. - The frameworkadmin patch is pretty isolated. But I am not as familiar with this code or all the usecases. I suspect if you try to manage an install located at a UNC path with frameworkadmin then it will fail without the fix. - The general p2 patch is large and not isolated. This patch needs lots of review and testing. I am not confident that all UNC cases are fixed, but I know that there are a large number of places where UNC paths are busted in the code. The fixes are not that risky but the quantity is large an pervasive.
I have been running with the support in simple configurator and it is fine. However once I tried the fwk admin and got unlucky with it. Therefore for 3.4 I propose that we only apply the simpleconfigurator change and review the other patches later.
The questin would be, what issues remain if we do only the SimpleConfigurator fix?
(In reply to comment #11) > I have been running with the support in simple configurator and it is fine. > However once I tried the fwk admin and got unlucky with it. Can you provide more details on what was failing? I tried this too but ran into general issues self-hosting, even without the patch. > Therefore for 3.4 I propose that we only apply the simpleconfigurator change > and review the other patches later. > I agree, this is getting very risky at this point.
(In reply to comment #12) > The questin would be, what issues remain if we do only the SimpleConfigurator > fix? > Take a look at the patch to framework admin. Code like this is broken if location==<URL with UNC path> fwJar = new File(new URL(location).getFile()); launcherData.setFwJar(fwJar); I'm not sure how likely this situation is to occur. I think if you have your bundle pool located on a UNC path but you install the configuration locally then that would cause this code to fail. For example if location="file://machine/eclipse/bundlepool/org.eclipse.osgi.jar" then the the result will be fwJar= new File("/machine/eclipse/bundlepool/org.eclipse.osgi.jar"); On windows that would look for the file /machine/eclipse/bundlepool/org.eclipse.osgi.jar on the same drive as the user.dir (I'm not sure what that path means if the user.dir is a UNC path). Until we actually test the basic scenarios with UNC paths I'm worried that we will not really know what is broken.
I take my +1 back. The patch in simple configurator introduces a severe regression since the bundles.info file is simply not being found! The problem comes from the call to buildURL(file) that replaces the call to new URL(file) in the method SimpleConfiguratorImpl#getConfigurationURL() (line 69). In the original code, the URL being passed in will cause a MalformedURLException which will then cause the searching of the file relatively to the configuration area. With the new code, the buildURL(file) eventually call new File(file) which will succeed and therefore cause the bundles.info file to be discovered relatively to the current working dir.
In answer to "what else is broken" , it has been reported that it is not possible to install from a repository located on a UNC drive.
Created attachment 102895 [details] simpleconfigurator patch 2 (In reply to comment #15) > I take my +1 back. The patch in simple configurator introduces a severe > regression since the bundles.info file is simply not being found! > > The problem comes from the call to buildURL(file) that replaces the call to new > URL(file) in the method SimpleConfiguratorImpl#getConfigurationURL() (line 69). > > In the original code, the URL being passed in will cause a > MalformedURLException which will then cause the searching of the file > relatively to the configuration area. With the new code, the buildURL(file) > eventually call new File(file) which will succeed and therefore cause the > bundles.info file to be discovered relatively to the current working dir. > This is dicey code. I'm not sure what the point is of the while (!done) loop in getConfigurationURL(). The code constructs a file URL and then calls getFile on the URL and then attempts to construct another URL from that. It does this in a loop until a MalformedURLException is thrown. Why is that needed? Do we have cases with nested file URLs file:file:file: ? The other strange thing is when we construct relative URLs we do not really mean relative according to the File definition (from user.dir). We mean relative to the configuration location. The problem with the buildURL is that we construct a File object from some relative spec and then call toURL which returns an absolute file URL relative to the user.dir. This patch changes buildURL to check that the file isAbsolute() before returning the result of File.toURL(). This should keep the behavior of new URL("file:org.eclipse.equinox.simpleconfigurator/bundles.info"); which keeps the getPath() as relative. I will also attach the same fix for frameworkadmin and the rest of p2.
Created attachment 102898 [details] frameworkadmin patch 2 Same buildURL fix for relative URLs for frameworkadmin.
Created attachment 102899 [details] p2 patch 2 Same buildURL fix for p2 patch.
Reevaluate in RC4.
Please note that we are only interested in reviewing the patch to simpleconfigurator (attachmment https://bugs.eclipse.org/bugs/attachment.cgi?id=102895).
Reviewed simple configurator patch with Pascal. Seems good. +1
For testing purposes I setup my machine to have a "local" UNC path. Here is how I set this up on Windows XP: 1) From File Explorer select a folder you would like to use as your UNC path. Right-Click this folder and bring up the Properties dialog. 2) Select the "Sharing" tab and then select the "Share this folder" option. Note that you can change the name under which you share this folder 3) If you are going to test the ability to write to the UNC path then you must also click the "Permissions" button on the "Sharing" tab. 4) For "Group or user names" there should be the user "Everyone". If not then click "Add..." and enter the name "Everyone" (you can click the "Check Names" to ensure the user exists on your system). Then click OK. 5) For the Everyone user check the "Full Control" permission under the "Allow" column. Click OK until the folder properties dialog exits. To test your local UNC path do the following: 1) Find out the host name of your machine (from a dos prompt execute "hostname") 2) In File Explorer enter in your UNC path \\<hostname>\<share folder name> 3) This should allow you to browse to the shared location. Ensure you can modify the folder if you set it up to be fully controlled by Everyone. Now you should be able to use this UNC path locally to test Eclipse. For example: 1) Launch eclipse and point it to the UNC path for a workspace location. Then you can test the self-hosting scenario from comment 1. 2) Extract an eclipse build to the UNC path and then launch it using the UNC path to ensure eclipse can launch. 3) Attempt to do a build to build upgrade of Eclipse while launched from the UNC path to ensure p2 can provision to a UNC path. other scenarios involving the director? ...
From a visual inspection the patch makes sense but I am unable to set up a UNC location on my machine to test it thoroughly. Added Jeff as an additional reviewer. It would be helpful to see if the patch fixes your/Ben's original problem.
Ok, we'll take a look tomorrorow. I know that Ben was messing around with UNC options today but not sure that he came up with anything conclusive. As a side note, it did appear that we had some bad behavior just with the workspace on a UNC drive (quite independent of p2). Tomorrow will bring more investigation and perhpas more bug reports :-(
Created attachment 103577 [details] p2 patch 3 I could not apply the p2 patch to HEAD any more because of conflicts. Here is an updated patch against HEAD. I found some new places where new URL(String) was being called (not sure if these were cases I missed before or new code that got released since my last patch).
I think for RC4 we are only considering the simpleconfigurator patch. Were there any changes that affect that patch? It seems to still apply ok but are there som eadditional URL(String) uses that need to be fixed?
(In reply to comment #27) > I think for RC4 we are only considering the simpleconfigurator patch. Were > there any changes that affect that patch? It seems to still apply ok but are > there som eadditional URL(String) uses that need to be fixed? > The simpleconfigurator patch is fine. It was only the big p2 patch that was out of sync. I was not intending for the p2 patch to be reviewed, I am only trying to keep it up to date.
+1 for the simpleconfigurator patch
This seems to have enough reviewers so I'm removing my review flag.
Simple configurator patch released. I'm leaving the bug open and unsetting the milestone as there are more problems to be looked at.
Along the fixes in bug 235629 and the patches in this bug I was able to do the following: 1) Unzip a build into a UNC directory and launch eclipse. 2) Point Eclipse to a UNC workspace 3) Install an IU containing releng tools Pretty basic stuff, but it is a start.
*** Bug 206950 has been marked as a duplicate of this bug. ***
I've been doing some work around UNC paths and URIs. One thing we need to be careful about is when using URI.resolve with a URI based on a UNC file as the base. What I found is that the end result is not a UNC path URI. e.g. File baseFile = new File("\\\\127.0.0.1\\somefile\\"); File aFile = new File(baseFile, "a"); URI baseURI = new URI("file:////127.0.0.1/somefile/"); URI aBFURI = baseFile.toURI().resolve("a"); URI aBUURI = baseURI.resolve("a"); System.out.println(baseFile); System.out.println(aFile); \\ Note: baseFile.toURI drops the slash if the file does not exist on disk System.out.println(baseFile.toURI()); System.out.println(aFile.toURI()); System.out.println(aBFURI); System.out.println(aBUURI); ------ RESULT: \\127.0.0.1\somefile \\127.0.0.1\somefile\a file:////127.0.0.1/somefile file:////127.0.0.1/somefile/a file:/127.0.0.1/a file:/127.0.0.1/somefile/a
Given the few bugs that have been reported on that, that there is an easy workaround which consists in creating a drive, that URI do not support these things properly (see previous comment), the time it will take to fix this, and the ease with which this kind of support decays, all that weighted with the low benefits I'm tempted to close as wontfix or at least remove from 3.5.
I you decide to "won't fix" this bug, I then suggest to explicitly forbid UNC : it will avoid new p2 users to waste their time.
Before closing on this topic this will be brought up to the PMC and probably on the cross-project ML. Pierre, I suppose that you guys been able to work around this?
We finally went with mounted drives, but it took some time to understand the problem. Moreover, when the same p2 admin tool is launched on different boxes, you have to ensure that your network drives are mounted on the same letter.
It seems that Simon's findings in comment #34 is a known Sun bug http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=5086147 and it is fixed in the JDK 6 (aka Mustang). First, we need to verify that the scenarios of going back and forth between URI and File works properly. If it does then we need to test eclipse 3.5 on JDK 6. If running with JDK 6 appears to make eclipse 3.5 be able to come up, then we may be in a better situation since we are now using URI consistently which are much more anal about how things work. We may also add a check in simpleconfigurator to detect invalid combination of UNC paths on startup and fail gracefully indicating to the user that a JDK 6 is required. John, could you please take a look at the first and second steps and report. Thx. Marking 3.5 M5, but only to make sure we do the analysis.
Running with fixes for bug 260082 and bug 260084, I can start N20090105-2000 from a UNC install location.
At what level are we fixing this? Only for java 6? What about the rest of eclipse?
I have hit another roadblock that URI#resolve also doesn't behave correctly with UNC paths. There is a Sun bug for this too: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4723726 The root of the problem is that Java incorrectly encodes a UNC path like this: \\SERVER\SOME\PATH As the following URI: file:////SERVER/SOME/PATH However the correct encoding is the store the server name as the host component: file://SERVER/SOME/PATH There is an excellent reference from Microsoft on this subject found here: http://blogs.msdn.com/ie/archive/2006/12/06/file-uris-in-windows.aspx I think we are stuck with using Java's faulty encoding of UNC paths since we are using URI, but we can perhaps workaround the bug in URI#resolve.
Given that Sun is addressing some of those URI problems in Java 6, we could point the two bugs at each others to get them to address the problem fully.
With all of the latest fixes from HEAD, I am able to install into a UNC location, and perform further provisioning operations after installation. I suggest we open new bugs for any future issues that come up, since this one is very old and has been used to track several unrelated issues since it was created.
This is great! Thanks!
Re-reading this, I missed comment #41: > At what level are we fixing this? Only for java 6? > What about the rest of eclipse? None of the fixes rely on Java 6. I can't answer for the rest of Eclipse. I do know that there has been very little effort scheduled for ensuring UNC paths work well across the platform. In the absence of a definite plan item with time scheduled for this, we are just making a best effort approach where we investigate and fix bugs as they are reported. I believe with these latest fixes we are in better shape than Eclipse 3.4, but I won't claim that we have no more platform bugs in dealing with UNC paths.
(In reply to comment #42) > I think we are stuck with using Java's faulty encoding of UNC paths since we > are using URI, but we can perhaps workaround the bug in URI#resolve. So do I get this right that in Eclipse, we encode \\SERVER\SOME\PATH As the following URI: file:////SERVER/SOME/PATH because the correct encoding (SERVER as the "authority") leads to exceptions in Sun's broken URI implementation? Was the option proposed in comment 1 of http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=5086147 ever considered, namely encoding the UNC path as file:/%27SERVER/SOME/PATH which should protect from incorrect normalization during resolve? In order to be consistent across more than just Equinox p2, I'd like to know what format we'll want to use and what tools / converters clients are expected to use. How can we best document this? In terms of testing, the following areas come to mind where file <-> URI conversions take place: * Linked resources * EFS IFileStore paths anything else (like classpath entries, external library references, ...)?
Martin, the way I would put it is that the Java class libraries consistently encode UNC paths that way. For example: new java.io.File("//SERVER/PATH").toURI() Produces: file:////SERVER/PATH The class libraries are relatively consistent here and most things work fine with that encoding. For example: File file1 = new File("//SERVER/PATH"); File file2 = new File(file1.toURI()); file2.toString() -> "\\SERVER\PATH" file1.equals(file2) -> true I have used this same encoding in the resources plugin for example and for the most part it works fine. However there are certain class library methods that will change the encoding. In particular, URI#normalize(), and converting from URI->URL->URI. I had to work around these two problems when using the "four slash" encoding that the class libraries produce. I'm not sure, but I think using a completely different encoding of UNC paths than classes like java.io.File#toURI() would produce even more problems to workaround. You're welcome to try it though ;)
I'm fine with standardizing on the "four slash" encoding as it seems to be the standard used by the class libs. I'm just wondering what your workarounds are specifically, and what clients should be aware of. The solutions chosen in the Platform should be documented somewhere such that they can also be applied in the larger context of Eclipse projects.
I have created a wiki page that includes Tom's instructions for setting up a workspace using UNC paths, and some details on specific code issues that can arise when working with UNC paths: http://wiki.eclipse.org/Eclipse/UNC_Paths
*** Bug 275820 has been marked as a duplicate of this bug. ***
