[cp-patches] Re: PR java/14070: gij and -jar argument should set the manifest Class-path recursively

[Andrew Haley]

Tom Tromey writes:
 > >>>>> "Andrew" == Andrew Haley <address@hidden> writes:
 > 
 > Andrew> OK, this is the new, improved, low-fat version of the patch.
 > Andrew> urls was a Vector, and I've changed it to a LinkedHashSet.  When we
 > Andrew> add a URL, we first check that we haven't added it already, and this
 > Andrew> prevents the loader from looping.
 > 
 > I don't think this satisfies all the constraints we need to satisfy.
 > 
 > I'm pretty sure there is code out there that assumes that
 > URLClassLoader.getURLs() will return the exact URLs given to
 > URLClassLoader, in the exact order.  I think this patch changes this.
 > 
 > 
 > The other question I have is about ordering.  Suppose you create a
 > URLClassLoader with a.jar and b.jar.  Suppose a.jar has a
 > Class-Path entry pointing to a2.jar.  And finally suppose you are
 > looking for a resource named R which appears in both a2.jar and b.jar.
 > Which is found?
 > 
 > I'm sure this case sounds dumb, but I'm picturing the hours of
 > debugging that will happen when we run into it in the field... better
 > to be compatible up front.

No argument.

Okay, this implementation maintains a separate list of URLs and
urlinfos.  The URLs are the URLs we were given, but the urlinfos also
includes the URLs that appeared in the "Class-Path" attributes, in the
same place.  urlinfos doesn't include more than one instace of each
URL.

This means that finding a class involves a simple linear search, and
it can be faster if URLs appear more than once.  It might be better to
make urlinfos a LinkedHashSet.

As far as I can tell, this really does behave the same way as the JDK.

Andrew.


2004-11-02  Andrew Haley  <address@hidden>

        * java/net/URLClassLoader.java (urls): Now a LinkedHashSet.
        (JarURLLoader.classPath): New field.
        (JarURLLoader.JarURLLoader): Read mainfest to parse "Class-Path"
        attribute and add URLs for each entry.
        (addURLImpl): Don't add a URL if it's already in the list.
        (definePackage, findURLResource, findResources): Use
        urlinfos.size(), not urls.size().
        
Index: java/net/URLClassLoader.java
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/net/URLClassLoader.java,v
retrieving revision 1.16.18.4
diff -c -2 -p -r1.16.18.4 URLClassLoader.java
*** java/net/URLClassLoader.java        12 Oct 2004 14:53:07 -0000      
1.16.18.4
--- java/net/URLClassLoader.java        3 Nov 2004 15:56:20 -0000
*************** import java.security.cert.Certificate;
*** 54,57 ****
--- 54,59 ----
  import java.util.Enumeration;
  import java.util.HashMap;
+ import java.util.Iterator;
+ import java.util.StringTokenizer;
  import java.util.Vector;
  import java.util.jar.Attributes;
*************** public class URLClassLoader extends Secu
*** 144,150 ****
  
    /**
!    * Store pre-parsed information for each url into this vector
!    * each element is a URL loader, corresponding to the URL of
!    * the same index in "urls"
     */
    private final Vector urlinfos = new Vector();
--- 146,153 ----
  
    /**
!    * Store pre-parsed information for each url into this vector: each
!    * element is a URL loader.  A jar file has its own class-path
!    * attribute which adds to the URLs that will be searched, but this
!    * does not add to the list of urls.
     */
    private final Vector urlinfos = new Vector();
*************** public class URLClassLoader extends Secu
*** 226,229 ****
--- 229,237 ----
        return null;
      }
+ 
+     Vector getClassPath()
+     {
+       return null;
+     }
    }
  
*************** public class URLClassLoader extends Secu
*** 295,298 ****
--- 303,308 ----
      final URL baseJarURL; // Base jar: url for all resources loaded from jar
  
+     Vector classPath; // The "Class-Path" attribute of this Jar's manifest
+ 
      SoURLLoader soURLLoader;
  
*************** public class URLClassLoader extends Secu
*** 310,313 ****
--- 320,324 ----
  
        this.soURLLoader = null;
+       this.classPath = null;
        URL baseJarURL = null;
        JarFile jarfile = null;
*************** public class URLClassLoader extends Secu
*** 330,334 ****
                  if (libDir != null && (libDir.isDirectory()))
                    {
!                     File soFile = new File (libDirName + File.separator + 
f.getName() 
                                              + ".so");
                      if (soFile != null && soFile.isFile())
--- 341,346 ----
                  if (libDir != null && (libDir.isDirectory()))
                    {
!                     File soFile = new File (libDirName 
!                                             + File.separator + f.getName() 
                                              + ".so");
                      if (soFile != null && soFile.isFile())
*************** public class URLClassLoader extends Secu
*** 338,342 ****
                    }
                }
!           }
        }
        catch (IOException ioe)
--- 350,386 ----
                    }
                }
! 
!             Manifest manifest;
!             Attributes attributes;
!             String classPathString;
! 
!             if ((manifest = jarfile.getManifest()) != null
!                 && (attributes = manifest.getMainAttributes()) != null
!                 && ((classPathString 
!                      = attributes.getValue(Attributes.Name.CLASS_PATH)) 
!                     != null))
!               {
!                 this.classPath = new Vector();
! 
!                 StringTokenizer st
!                   = new StringTokenizer 
!                     (classPathString,
!                      System.getProperty ("path.separator", ":"));
!       
!                 while (st.hasMoreElements ()) 
!                   {  
!                     String e = st.nextToken ();
!                     try
!                       {
!                         URL url = new URL(baseURL, e);
!                         this.classPath.add(url);
!                       } 
!                     catch (java.net.MalformedURLException xx)
!                       {
!                         // Give up
!                       }
!                   }
!               }
!           }
        }
        catch (IOException ioe)
*************** public class URLClassLoader extends Secu
*** 388,391 ****
--- 432,440 ----
          }
      }
+ 
+     Vector getClassPath()
+     {
+       return classPath;
+     }
    }
  
*************** public class URLClassLoader extends Secu
*** 779,782 ****
--- 828,832 ----
    protected void addURL(URL newUrl)
    {
+     urls.add(newUrl);
      addURLImpl(newUrl);
    }
*************** public class URLClassLoader extends Secu
*** 806,815 ****
              loader = new RemoteURLLoader(this, newUrl);
  
!           // Cache it.
!           urlloaders.put(newUrl, loader);
          }
  
-       urls.add(newUrl);
        urlinfos.add(loader);
        }
    }
--- 856,878 ----
              loader = new RemoteURLLoader(this, newUrl);
  
!             // Cache it.
!             urlloaders.put(newUrl, loader);
          }
  
        urlinfos.add(loader);
+ 
+       Vector extraUrls = loader.getClassPath();
+       if (extraUrls != null)
+         {
+           Iterator it = extraUrls.iterator();
+           while (it.hasNext())
+             {
+               URL url = (URL)it.next();
+               URLLoader extraLoader = (URLLoader) urlloaders.get(url);
+               if (! urlinfos.contains (extraLoader))
+                 addURLImpl(url);
+             }
+         }
+ 
        }
    }
*************** public class URLClassLoader extends Secu
*** 822,826 ****
    {
      for (int i = 0; i < newUrls.length; i++)
!       addURLImpl(newUrls[i]);
    }
  
--- 885,889 ----
    {
      for (int i = 0; i < newUrls.length; i++)
!       addURL(newUrls[i]);
    }
  
*************** public class URLClassLoader extends Secu
*** 879,883 ****
      // Just try to find the resource by the (almost) same name
      String resourceName = className.replace('.', '/') + ".class";
!     int max = urls.size();
      Resource resource = null;
      for (int i = 0; i < max && resource == null; i++)
--- 942,946 ----
      // Just try to find the resource by the (almost) same name
      String resourceName = className.replace('.', '/') + ".class";
!     int max = urlinfos.size();
      Resource resource = null;
      for (int i = 0; i < max && resource == null; i++)
*************** public class URLClassLoader extends Secu
*** 988,992 ****
    private Resource findURLResource(String resourceName)
    {
!     int max = urls.size();
      for (int i = 0; i < max; i++)
        {
--- 1051,1055 ----
    private Resource findURLResource(String resourceName)
    {
!     int max = urlinfos.size();
      for (int i = 0; i < max; i++)
        {
*************** public class URLClassLoader extends Secu
*** 1059,1063 ****
    {
      Vector resources = new Vector();
!     int max = urls.size();
      for (int i = 0; i < max; i++)
        {
--- 1122,1126 ----
    {
      Vector resources = new Vector();
!     int max = urlinfos.size();
      for (int i = 0; i < max; i++)
        {