public inbox for java-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* Patch: RFC: fix ProcessBuilder; windows help needed
@ 2007-02-03  0:24 Tom Tromey
  2007-02-03 18:11 ` Mohan Embar
  2007-02-11 19:08 ` Mohan Embar
  0 siblings, 2 replies; 29+ messages in thread
From: Tom Tromey @ 2007-02-03  0:24 UTC (permalink / raw)
  To: GCJ-patches

I'm still testing this patch.  I'll check it in once I'm done.

During the last Classpath merge I must have stubbed out
ProcessBuilder.  This patch fixes that and removes a divergence of
ours in the process.

One problem with this patch is that I did not update the Windows
port.  I added the needed method argument, but it does not do
anything.  I would appreciate it if someone could fix this.

Tom

Index: ChangeLog
from  Tom Tromey  <tromey@redhat.com>

	* sources.am, Makefile.in: Rebuilt.
	* scripts/makemake.tcl (emit_package_rule): Don't omit
	VMProcess.java.
	* Makefile.am (nat_source_files): Added natVMProcess.cc.
	(inner_nat_headers): Added ImmediateEOFInputStream.h.
	* gcj/javaprims.h: Regenerated.
	* java/lang/System.java (EnvironmentMap): Now package-private.
	(EnvironmentMap(Map)): New constructor.
	(EnvironmentMap.put): New method.
	* java/lang/natWin32Process.cc (startProcess): Update.
	* java/lang/Win32Process.java (Win32Process): Added 'redirect'
	argument.
	(startProcess): Likewise.
	* java/lang/EcosProcess.java (EcosProcess): Added 'redirect'
	argument.
	* java/lang/natPosixProcess.cc (nativeSpawn): Handle redirection.
	* java/lang/PosixProcess.java (redirect): New field.
	(PosixProcess): Added 'redirect' argument.
	* java/lang/natRuntime.cc (execInternal): Added 'redirect'
	argument to Process creation.
	* java/lang/natVMProcess.cc: New file.
	* java/lang/ProcessBuilder.java: Removed.
	* java/lang/VMProcess.java: New file.

Index: scripts/makemake.tcl
===================================================================
--- scripts/makemake.tcl	(revision 121470)
+++ scripts/makemake.tcl	(working copy)
@@ -285,7 +285,7 @@
     # Object and Class are special cases due to an apparent compiler
     # bug.  Process is a special case because we don't build all
     # concrete implementations of Process on all platforms.
-    set omit "| tr ' ' '\\n' | fgrep -v Object.class | fgrep -v Class.class | grep -v '\[^/\]Process' "
+    set omit "| tr ' ' '\\n' | fgrep -v Object.class | fgrep -v Class.class | egrep -v '\(Ecos\|Posix\|Win32\)Process' "
   } else {
     set omit ""
   }
Index: gcj/javaprims.h
===================================================================
--- gcj/javaprims.h	(revision 121470)
+++ gcj/javaprims.h	(working copy)
@@ -1,7 +1,7 @@
 // javaprims.h - Main external header file for libgcj.  -*- c++ -*-
 
 
-/* Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006
+/* Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007
    Free Software Foundation
 
    This file is part of libgcj.
@@ -248,6 +248,7 @@
       class VMCompiler;
       class VMDouble;
       class VMFloat;
+      class VMProcess;
       class VMThrowable;
       class VerifyError;
       class VirtualMachineError;
Index: java/lang/Win32Process.java
===================================================================
--- java/lang/Win32Process.java	(revision 121470)
+++ java/lang/Win32Process.java	(working copy)
@@ -1,6 +1,6 @@
 // Win32Process.java - Subclass of Process for Win32 systems.
 
-/* Copyright (C) 2002, 2003, 2006  Free Software Foundation
+/* Copyright (C) 2002, 2003, 2006, 2007  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -51,9 +51,8 @@
 
   public native int waitFor () throws InterruptedException;
 
-  public Win32Process (String[] progarray,
-                          String[] envp,
-                          File dir)
+  public Win32Process (String[] progarray, String[] envp, File dir,
+		       boolean redirect)
     throws IOException
   {
     for (int i = 0; i < progarray.length; i++)
@@ -64,7 +63,7 @@
           progarray[i] = "\"" + s + "\"";
       }
 
-    startProcess (progarray, envp, dir);
+    startProcess (progarray, envp, dir, redirect);
   }
 
   // The standard streams (stdin, stdout and stderr, respectively)
@@ -81,8 +80,9 @@
 
   private native boolean hasExited ();
   private native void startProcess (String[] progarray,
-           String[] envp,
-           File dir)
+				    String[] envp,
+				    File dir,
+				    boolean redirect)
     throws IOException;
   private native void cleanup ();
 }
Index: java/lang/VMProcess.java
===================================================================
--- java/lang/VMProcess.java	(revision 0)
+++ java/lang/VMProcess.java	(revision 0)
@@ -0,0 +1,68 @@
+/* java.lang.VMProcess -- VM implementation of java.lang.ProcessBuilder
+   Copyright (C) 2007 Free Software Foundation, Inc.
+
+This file is part of GNU Classpath.
+
+GNU Classpath is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2, or (at your option)
+any later version.
+
+GNU Classpath is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GNU Classpath; see the file COPYING.  If not, write to the
+Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+02110-1301 USA.
+
+Linking this library statically or dynamically with other modules is
+making a combined work based on this library.  Thus, the terms and
+conditions of the GNU General Public License cover the whole
+combination.
+
+As a special exception, the copyright holders of this library give you
+permission to link this library with independent modules to produce an
+executable, regardless of the license terms of these independent
+modules, and to copy and distribute the resulting executable under
+terms of your choice, provided that you also meet, for each linked
+independent module, the terms and conditions of the license of that
+module.  An independent module is a module which is not derived from
+or based on this library.  If you modify this library, you may extend
+this exception to your version of the library, but you are not
+obligated to do so.  If you do not wish to do so, delete this
+exception statement from your version. */
+
+package java.lang;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+
+final class VMProcess
+{
+  static native Process nativeExec(String[] cmd, String[] env,
+				   File dir, boolean redirect)
+    throws IOException;
+
+  static Process exec(List<String> cmd, Map<String, String> env,
+		      File dir, boolean redirect) throws IOException
+  {
+    String[] acmd = (String[]) cmd.toArray(new String[cmd.size()]);
+    String[] aenv = new String[env.size()];
+
+    int i = 0;
+    Iterator iter = env.entrySet().iterator();
+    while (iter.hasNext())
+      {
+	Map.Entry entry = (Map.Entry) iter.next();
+	aenv[i++] = entry.getKey() + "=" + entry.getValue();
+      }
+
+    return nativeExec(acmd, aenv, dir, redirect);
+  }
+}
Index: java/lang/System.java
===================================================================
--- java/lang/System.java	(revision 121470)
+++ java/lang/System.java	(working copy)
@@ -1,5 +1,5 @@
 /* System.java -- useful methods to interface with the system
-   Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006
+   Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007
    Free Software Foundation, Inc.
 
 This file is part of GNU Classpath.
@@ -827,7 +827,7 @@
    *
    * @author Andrew John Hughes (gnu_andrew@member.fsf.org)
    */
-  private static class EnvironmentMap
+  static class EnvironmentMap
     extends HashMap<String,String>
   {
     
@@ -853,8 +853,21 @@
     {
       super();
     }
-    
+
     /**
+     * Constructs a new <code>EnvironmentMap</code> containing
+     * the contents of the specified map.
+     *
+     * @param m the map to be added to this.
+     * @throws NullPointerException if a key or value is null.
+     * @throws ClassCastException if a key or value is not a String.
+     */    
+    EnvironmentMap(Map<String,String> m)
+    {
+      super(m);
+    }
+
+    /**
      * Blocks queries containing a null key or one which is not
      * of type <code>String</code>.  All other queries
      * are forwarded to the superclass.
@@ -938,8 +951,33 @@
         keys = new EnvironmentSet(super.keySet());
       return keys;
     }
-    
+
     /**
+     * Associates the given key to the given value. If the
+     * map already contains the key, its value is replaced.
+     * The map does not accept null keys or values, or keys
+     * and values not of type {@link String}.
+     *
+     * @param key the key to map.
+     * @param value the value to be mapped.
+     * @return the previous value of the key, or null if there was no mapping
+     * @throws NullPointerException if a key or value is null.
+     * @throws ClassCastException if a key or value is not a String.
+     */
+    public String put(String key, String value)
+    {
+      if (key == null)
+	throw new NullPointerException("A new key is null.");
+      if (value == null)
+	throw new NullPointerException("A new value is null.");
+      if (!(key instanceof String))
+	throw new ClassCastException("A new key is not a String.");
+      if (!(value instanceof String))
+	throw new ClassCastException("A new value is not a String.");
+      return super.put(key, value);
+    }
+
+    /**
      * Removes a key-value pair from the map.  The queried key may not
      * be null or of a type other than a <code>String</code>.
      *
Index: java/lang/natVMProcess.cc
===================================================================
--- java/lang/natVMProcess.cc	(revision 0)
+++ java/lang/natVMProcess.cc	(revision 0)
@@ -0,0 +1,34 @@
+// natVMProcess.cc - native code for ProcessBuilder
+
+/* Copyright (C) 2007 Free Software Foundation
+
+   This file is part of libgcj.
+
+This software is copyrighted work licensed under the terms of the
+Libgcj License.  Please consult the file "LIBGCJ_LICENSE" for
+details.  */
+
+#include <config.h>
+
+#include <gcj/cni.h>
+#include <jvm.h>
+
+#include <platform.h>
+
+#include <java/lang/VMProcess.h>
+#include <java/lang/Process.h>
+#include <java/io/File.h>
+
+// It is convenient and safe to simply include all of these.
+#include <java/lang/Win32Process.h>
+#include <java/lang/EcosProcess.h>
+#include <java/lang/PosixProcess.h>
+
+::java::lang::Process *
+java::lang::VMProcess::nativeExec (jstringArray cmd,
+				   jstringArray env,
+				   ::java::io::File *dir,
+				   jboolean redirect)
+{
+  return new _Jv_platform_process (cmd, env, dir, redirect);
+}
Index: java/lang/EcosProcess.java
===================================================================
--- java/lang/EcosProcess.java	(revision 121470)
+++ java/lang/EcosProcess.java	(working copy)
@@ -1,6 +1,6 @@
 // EcosProcess.java - Subclass of Process for eCos systems.
 
-/* Copyright (C) 1998, 1999, 2006  Free Software Foundation
+/* Copyright (C) 1998, 1999, 2006, 2007  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -51,9 +51,8 @@
     return 0;
   }
 
-  public EcosProcess (String[] progarray,
-                          String[] envp,
-                          File dir)
+  public EcosProcess (String[] progarray, String[] envp, File dir,
+		      boolean redirect)
     throws IOException
   {
     throw new IOException ("eCos processes unimplemented");
Index: java/lang/PosixProcess.java
===================================================================
--- java/lang/PosixProcess.java	(revision 121470)
+++ java/lang/PosixProcess.java	(working copy)
@@ -1,5 +1,5 @@
 // PosixProcess.java - Subclass of Process for POSIX systems.
-/* Copyright (C) 1998, 1999, 2004, 2006  Free Software Foundation
+/* Copyright (C) 1998, 1999, 2004, 2006, 2007  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -349,8 +349,8 @@
    */
   private native void nativeSpawn();
 
-  PosixProcess(String[] progarray, String[] envp, File dir)
-           throws IOException
+  PosixProcess(String[] progarray, String[] envp, File dir, boolean redirect)
+    throws IOException
   {
     // Check to ensure there is something to run, and avoid
     // dereferencing null pointers in native code.
@@ -360,6 +360,7 @@
     this.progarray = progarray;
     this.envp = envp;
     this.dir = dir;
+    this.redirect = redirect;
 
     // Start a ProcessManager if there is not one already running.
     synchronized (queueLock)
@@ -414,6 +415,7 @@
   private String[] progarray;
   private String[] envp;
   private File dir;
+  private boolean redirect;
 
   /** Set by the ProcessManager on problems starting. */
   private Throwable exception;
Index: java/lang/natWin32Process.cc
===================================================================
--- java/lang/natWin32Process.cc	(revision 121470)
+++ java/lang/natWin32Process.cc	(working copy)
@@ -1,6 +1,6 @@
 // natWin32Process.cc - Native side of Win32 process code.
 
-/* Copyright (C) 2003, 2006  Free Software Foundation
+/* Copyright (C) 2003, 2006, 2007  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -210,8 +210,9 @@
 
 void
 java::lang::Win32Process::startProcess (jstringArray progarray,
-                                           jstringArray envp,
-                                           java::io::File *dir)
+					jstringArray envp,
+					java::io::File *dir,
+					jboolean redirect)
 {
   using namespace java::io;
 
Index: java/lang/natPosixProcess.cc
===================================================================
--- java/lang/natPosixProcess.cc	(revision 121470)
+++ java/lang/natPosixProcess.cc	(working copy)
@@ -1,6 +1,6 @@
 // natPosixProcess.cc - Native side of POSIX process code.
 
-/* Copyright (C) 1998, 1999, 2000, 2002, 2003, 2004, 2005, 2006  Free Software Foundation
+/* Copyright (C) 1998, 1999, 2000, 2002, 2003, 2004, 2005, 2006, 2007  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -41,6 +41,7 @@
 #include <java/io/FileOutputStream.h>
 #include <java/io/IOException.h>
 #include <java/lang/OutOfMemoryError.h>
+#include <java/lang/PosixProcess$EOFInputStream.h>
 
 using gnu::java::nio::channels::FileChannelImpl;
 
@@ -231,7 +232,7 @@
   try
     {
       // Transform arrays to native form.
-    args = (char **) _Jv_Malloc ((progarray->length + 1) * sizeof (char *));
+      args = (char **) _Jv_Malloc ((progarray->length + 1) * sizeof (char *));
 
       // Initialize so we can gracefully recover.
       jstring *elts = elements (progarray);
@@ -262,23 +263,30 @@
 	path = new_string (dir->getPath ());
 
       // Create pipes for I/O.  MSGP is for communicating exec()
-      // status.
-      if (pipe (inp) || pipe (outp) || pipe (errp) || pipe (msgp)
+      // status.  If redirecting stderr to stdout, we don't need to
+      // create the ERRP pipe.
+      if (pipe (inp) || pipe (outp) || pipe (msgp)
 	  || fcntl (msgp[1], F_SETFD, FD_CLOEXEC))
-      throw new IOException (JvNewStringUTF (strerror (errno)));
+	throw new IOException (JvNewStringUTF (strerror (errno)));
+      if (! redirect && pipe (errp))
+	throw new IOException (JvNewStringUTF (strerror (errno)));
 
       // We create the streams before forking.  Otherwise if we had an
       // error while creating the streams we would have run the child
       // with no way to communicate with it.
-    errorStream =
-      new FileInputStream (new
-                           FileChannelImpl (errp[0], FileChannelImpl::READ));
-    inputStream =
-      new FileInputStream (new
-                           FileChannelImpl (inp[0], FileChannelImpl::READ));
-    outputStream =
-      new FileOutputStream (new FileChannelImpl (outp[1],
-                                             FileChannelImpl::WRITE));
+      if (redirect)
+	errorStream = PosixProcess$EOFInputStream::instance;
+      else
+	errorStream =
+	  new FileInputStream (new
+			       FileChannelImpl (errp[0],
+						FileChannelImpl::READ));
+      inputStream =
+	new FileInputStream (new
+			     FileChannelImpl (inp[0], FileChannelImpl::READ));
+      outputStream =
+	new FileOutputStream (new FileChannelImpl (outp[1],
+						   FileChannelImpl::WRITE));
 
       // We don't use vfork() because that would cause the local
       // environment to be set by the child.
@@ -319,14 +327,17 @@
 	  // We ignore errors from dup2 because they should never occur.
 	  dup2 (outp[0], 0);
 	  dup2 (inp[1], 1);
-	  dup2 (errp[1], 2);
+	  dup2 (redirect ? inp[1] : errp[1], 2);
 
 	  // Use close and not myclose -- we're in the child, and we
 	  // aren't worried about the possible race condition.
 	  close (inp[0]);
 	  close (inp[1]);
-	  close (errp[0]);
-	  close (errp[1]);
+	  if (! redirect)
+	    {
+	      close (errp[0]);
+	      close (errp[1]);
+	    }
 	  close (outp[0]);
 	  close (outp[1]);
 	  close (msgp[0]);
@@ -362,8 +373,11 @@
 
       myclose (outp[0]);
       myclose (inp[1]);
-      myclose (errp[1]);
-      myclose (msgp[1]);
+      if (! redirect)
+	{
+	  myclose (errp[1]);
+	  myclose (msgp[1]);
+	}
 
       char c;
       int r = read (msgp[0], &c, 1);
@@ -406,7 +420,7 @@
 	{
 	  if (errorStream != NULL)
 	    errorStream->close ();
-	  else
+	  else if (! redirect)
 	    myclose (errp[0]);
 	}
       catch (java::lang::Throwable *ignore)
@@ -417,10 +431,11 @@
       // the use of myclose.
       myclose (outp[0]);
       myclose (inp[1]);
-      myclose (errp[1]);
+      if (! redirect)
+	myclose (errp[1]);
       myclose (msgp[1]);
 
-    exception = thrown;
+      exception = thrown;
     }
 
   myclose (msgp[0]);
@@ -430,6 +445,7 @@
     {
       fcntl (outp[1], F_SETFD, FD_CLOEXEC);
       fcntl (inp[0], F_SETFD, FD_CLOEXEC);
-      fcntl (errp[0], F_SETFD, FD_CLOEXEC);
+      if (! redirect)
+	fcntl (errp[0], F_SETFD, FD_CLOEXEC);
     }
 }
Index: java/lang/ProcessBuilder.java
===================================================================
--- java/lang/ProcessBuilder.java	(revision 121470)
+++ java/lang/ProcessBuilder.java	(working copy)
@@ -1,118 +0,0 @@
-/* ProcessBuilder.java - Represent spawned system process
-   Copyright (C) 2005, 2006  Free Software Foundation, Inc.
-
-This file is part of GNU Classpath.
-
-GNU Classpath is free software; you can redistribute it and/or modify
-it under the terms of the GNU General Public License as published by
-the Free Software Foundation; either version 2, or (at your option)
-any later version.
-
-GNU Classpath is distributed in the hope that it will be useful, but
-WITHOUT ANY WARRANTY; without even the implied warranty of
-MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-General Public License for more details.
-
-You should have received a copy of the GNU General Public License
-along with GNU Classpath; see the file COPYING.  If not, write to the
-Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
-02110-1301 USA.
-
-Linking this library statically or dynamically with other modules is
-making a combined work based on this library.  Thus, the terms and
-conditions of the GNU General Public License cover the whole
-combination.
-
-As a special exception, the copyright holders of this library give you
-permission to link this library with independent modules to produce an
-executable, regardless of the license terms of these independent
-modules, and to copy and distribute the resulting executable under
-terms of your choice, provided that you also meet, for each linked
-independent module, the terms and conditions of the license of that
-module.  An independent module is a module which is not derived from
-or based on this library.  If you modify this library, you may extend
-this exception to your version of the library, but you are not
-obligated to do so.  If you do not wish to do so, delete this
-exception statement from your version. */
-
-
-package java.lang;
-
-import java.io.File;
-import java.io.IOException;
-import java.util.Arrays;
-import java.util.List;
-import java.util.Map;
-
-public final class ProcessBuilder
-{
-  private File directory = new File(System.getProperty("user.dir"));
-  private List<String> command;
-  // FIXME: make a copy.
-  private Map<String, String> environment = System.getenv();
-  private boolean redirect = false;
-
-  public ProcessBuilder(List<String> command)
-  {
-    this.command = command;
-  }
-
-  public ProcessBuilder(String... command)
-  {
-    this.command = Arrays.asList(command);
-  }
-
-  public List<String> command()
-  {
-    return command;
-  }
-
-  public ProcessBuilder command(List<String> command)
-  {
-    this.command = command;
-    return this;
-  }
-
-  public ProcessBuilder command(String... command)
-  {
-    this.command = Arrays.asList(command);
-    return this;
-  }
-
-  public File directory()
-  {
-    return directory;
-  }
-
-  public ProcessBuilder directory(File directory)
-  {
-    this.directory = directory;
-    return this;
-  }
-
-  public Map<String, String> environment()
-  {
-    return environment;
-  }
-
-  public boolean redirectErrorStream()
-  {
-    return redirect;
-  }
-
-  public ProcessBuilder redirectErrorStream(boolean redirect)
-  {
-    this.redirect = redirect;
-    return this;
-  }
-
-  public Process start() throws IOException
-  {
-    SecurityManager sm = SecurityManager.current; // Be thread-safe!
-    if (sm != null)
-      sm.checkExec(command.get(0));
-    //    return VMProcess.exec(command, environment, directory, redirect);
-    // FIXME
-    return null;
-  }
-}
Index: java/lang/natRuntime.cc
===================================================================
--- java/lang/natRuntime.cc	(revision 121470)
+++ java/lang/natRuntime.cc	(working copy)
@@ -1,6 +1,6 @@
 // natRuntime.cc - Implementation of native side of Runtime class.
 
-/* Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006  Free Software Foundation
+/* Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -297,7 +297,7 @@
 				   jstringArray env,
 				   java::io::File *dir)
 {
-  return new _Jv_platform_process (cmd, env, dir);
+  return new _Jv_platform_process (cmd, env, dir, false);
 }
 
 jint
Index: Makefile.am
===================================================================
--- Makefile.am	(revision 121470)
+++ Makefile.am	(working copy)
@@ -399,6 +399,7 @@
 	java/nio/DirectByteBufferImpl$$ReadWrite.h \
 	java/nio/channels/Pipe$$SinkChannel.h \
 	java/nio/channels/Pipe$$SourceChannel.h \
+	java/lang/VMProcess$ImmediateEOFInputStream.h \
 	java/lang/reflect/Proxy$$ProxyData.h \
 	java/lang/reflect/Proxy$$ProxyType.h \
 	gnu/java/net/PlainSocketImpl$$SocketInputStream.h \
@@ -864,6 +865,7 @@
 java/lang/natThread.cc \
 java/lang/natThreadLocal.cc \
 java/lang/natVMClassLoader.cc \
+java/lang/natVMProcess.cc \
 java/lang/natVMThrowable.cc \
 java/lang/ref/natReference.cc \
 java/lang/reflect/natArray.cc \

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-03  0:24 Patch: RFC: fix ProcessBuilder; windows help needed Tom Tromey
@ 2007-02-03 18:11 ` Mohan Embar
  2007-02-04  9:47   ` Marco Trudel
  2007-02-04 10:09   ` Mohan Embar
  2007-02-11 19:08 ` Mohan Embar
  1 sibling, 2 replies; 29+ messages in thread
From: Mohan Embar @ 2007-02-03 18:11 UTC (permalink / raw)
  To: tromey; +Cc: GCJ-patches

Hi Tom,

>One problem with this patch is that I did not update the Windows
>port.  I added the needed method argument, but it does not do
>anything.  I would appreciate it if someone could fix this.

My build box just bit the dust so it'll probably be at least a
few days before I can get to this one. I'll look at it as soon
as I can if no one else does.

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-03 18:11 ` Mohan Embar
@ 2007-02-04  9:47   ` Marco Trudel
  2007-02-04 10:09   ` Mohan Embar
  1 sibling, 0 replies; 29+ messages in thread
From: Marco Trudel @ 2007-02-04  9:47 UTC (permalink / raw)
  To: gnustuff; +Cc: tromey, GCJ-patches

Mohan Embar wrote:
> Hi Tom,
> 
>> One problem with this patch is that I did not update the Windows
>> port.  I added the needed method argument, but it does not do
>> anything.  I would appreciate it if someone could fix this.
> 
> My build box just bit the dust so it'll probably be at least a
> few days before I can get to this one. I'll look at it as soon
> as I can if no one else does.

I'd be glad if you could take a look at it because I'll be busy the next 
two weeks. Otherwise I can do it after that time...


Marco

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-03 18:11 ` Mohan Embar
  2007-02-04  9:47   ` Marco Trudel
@ 2007-02-04 10:09   ` Mohan Embar
  2007-02-05 21:05     ` Tom Tromey
  1 sibling, 1 reply; 29+ messages in thread
From: Mohan Embar @ 2007-02-04 10:09 UTC (permalink / raw)
  To: tromey; +Cc: GCJ-patches

Hi Tom,

>>One problem with this patch is that I did not update the Windows
>>port.  I added the needed method argument, but it does not do
>>anything.  I would appreciate it if someone could fix this.
>
>My build box just bit the dust so it'll probably be at least a
>few days before I can get to this one. I'll look at it as soon
>as I can if no one else does.

My build box problems are fixed (cannibalized the power supply
of another broken machine).

I'm assuming that if you're testing this on your end, that you have
one or more testcases, and I'm feeling kind of lazy. Would you care
to send these over?

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-04 10:09   ` Mohan Embar
@ 2007-02-05 21:05     ` Tom Tromey
  2007-02-06 17:58       ` Mohan Embar
  0 siblings, 1 reply; 29+ messages in thread
From: Tom Tromey @ 2007-02-05 21:05 UTC (permalink / raw)
  To: gnustuff; +Cc: GCJ-patches

>>>>> "Mohan" == Mohan Embar <gnustuff@thisiscool.com> writes:

Mohan> I'm assuming that if you're testing this on your end, that you have
Mohan> one or more testcases, and I'm feeling kind of lazy. Would you care
Mohan> to send these over?

Actually I sent out the patch before doing any testing at all, hoping
we could have all the patches aligned before it was ready :)

I wrote a trivial ProcessBuilder test case and put it in Mauve.  It
really only tests the new code path -- the one where we redirect
stderr to stdout.  It relies on having 'ls', but it should be simple
to make work on your system.

Tom

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-05 21:05     ` Tom Tromey
@ 2007-02-06 17:58       ` Mohan Embar
  2007-02-07 16:13         ` Tom Tromey
  0 siblings, 1 reply; 29+ messages in thread
From: Mohan Embar @ 2007-02-06 17:58 UTC (permalink / raw)
  To: tromey; +Cc: GCJ-patches

Hi Tom,

>Mohan> I'm assuming that if you're testing this on your end, that you have
>Mohan> one or more testcases, and I'm feeling kind of lazy. Would you care
>Mohan> to send these over?
>
>Actually I sent out the patch before doing any testing at all, hoping
>we could have all the patches aligned before it was ready :)
>
>I wrote a trivial ProcessBuilder test case and put it in Mauve.  It
>really only tests the new code path -- the one where we redirect
>stderr to stdout.  It relies on having 'ls', but it should be simple
>to make work on your system.

Give me a few days. I need to commit the small patches you and
Andrew just approved, then I'll dig into this.

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-06 17:58       ` Mohan Embar
@ 2007-02-07 16:13         ` Tom Tromey
  0 siblings, 0 replies; 29+ messages in thread
From: Tom Tromey @ 2007-02-07 16:13 UTC (permalink / raw)
  To: gnustuff; +Cc: GCJ-patches

>>>>> "Mohan" == Mohan Embar <gnustuff@thisiscool.com> writes:

Mohan> Give me a few days. I need to commit the small patches you and
Mohan> Andrew just approved, then I'll dig into this.

No problem.  My test case did find a buglet in my patch (it close()d
one too many fds in the redirect case).  I can send you an update if
you need it; it only affects the posix native code.

Tom

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-03  0:24 Patch: RFC: fix ProcessBuilder; windows help needed Tom Tromey
  2007-02-03 18:11 ` Mohan Embar
@ 2007-02-11 19:08 ` Mohan Embar
  2007-02-12  0:12   ` Tom Tromey
  1 sibling, 1 reply; 29+ messages in thread
From: Mohan Embar @ 2007-02-11 19:08 UTC (permalink / raw)
  To: tromey; +Cc: GCJ-patches

Hi Tom,

>I'm still testing this patch.  I'll check it in once I'm done.
>
>During the last Classpath merge I must have stubbed out
>ProcessBuilder.  This patch fixes that and removes a divergence of
>ours in the process.
>
>One problem with this patch is that I did not update the Windows
>port.  I added the needed method argument, but it does not do
>anything.  I would appreciate it if someone could fix this.

I've started to roll up my sleeves and look at this. The patch is
a tiny bit stale (my fault because I waited so long) because of
System.java, but I think I hand-applied the rejected hunks okay.
I'm new to the whole --enable-maintainer-mode thing. Reading
HACKING, it seems that after I've applied the patch, with automake
1.96 and autoconf 2.59 in my path, I need to do this:

./scripts/makemake.tcl > sources.am
automake

There are other steps, but I'm not sure if I have to follow them.
Two items from the document confused me, so I'm going to wait for further
advice before I proceed:

  "Possibly update the gcj/javaprims.h file with scripts/classes.pl
  (See below, it can only be done after the first source->bytecode
   pass has finished.)"

Does this need to be done? You've added a java.lang.VMProcess.

  "You will need to configure with --enable-maintainer-mode and you
  will need to update the .class files and generated CNI header files in
  your working tree"

I think what the above means is passing --enable-maintainer-mode as
a top-level configure flag, but I am stumped as to what "and you
will need to update the .class files and generated CNI header files in
your working tree" means.

To summarize, I'd like to know exactly what scripts to run after I apply
your patch (and ideally, why I'm running them).

I'd like to get up to speed on all of this, but if you're concerned about
time, a faster path might be committing your changes and letting me
pick of the pieces on the Win32 side. Your call.

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-11 19:08 ` Mohan Embar
@ 2007-02-12  0:12   ` Tom Tromey
  2007-02-13  2:08     ` Mohan Embar
  0 siblings, 1 reply; 29+ messages in thread
From: Tom Tromey @ 2007-02-12  0:12 UTC (permalink / raw)
  To: gnustuff; +Cc: GCJ-patches

>>>>> "Mohan" == Mohan Embar <gnustuff@thisiscool.com> writes:

Mohan> Reading
Mohan> HACKING, it seems that after I've applied the patch, with automake
Mohan> 1.96 and autoconf 2.59 in my path, I need to do this:
Mohan> ./scripts/makemake.tcl > sources.am
Mohan> automake

Yes.

Mohan>   "Possibly update the gcj/javaprims.h file with scripts/classes.pl
Mohan>   (See below, it can only be done after the first source->bytecode
Mohan>    pass has finished.)"
Mohan> Does this need to be done? You've added a java.lang.VMProcess.

Yeah, I did that.  But I think I may have omitted it from my patch, as
that section is generated.

Mohan> I think what the above means is passing --enable-maintainer-mode as
Mohan> a top-level configure flag, but I am stumped as to what "and you
Mohan> will need to update the .class files and generated CNI header files in
Mohan> your working tree" means.

If you --enable-java-maintainer-mode, then this ought to happen
automatically for you.

Mohan> To summarize, I'd like to know exactly what scripts to run
Mohan> after I apply your patch (and ideally, why I'm running them).

Download ecj.jar using the contrib/download_ecj method
Configure with --enable-java-maintainer-mode.
Apply patch.
./scripts/makemake.tcl > sources.am
automake
Build.
When the build fails, run classes.pl and insert output into javaprims.h
Rebuild

Mohan> I'd like to get up to speed on all of this, but if you're
Mohan> concerned about time, a faster path might be committing your
Mohan> changes and letting me pick of the pieces on the Win32
Mohan> side. Your call.

Better to iron this stuff out a little and see if any documentation
changes drop out :)

Tom

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-12  0:12   ` Tom Tromey
@ 2007-02-13  2:08     ` Mohan Embar
  2007-02-13 18:04       ` Tom Tromey
  0 siblings, 1 reply; 29+ messages in thread
From: Mohan Embar @ 2007-02-13  2:08 UTC (permalink / raw)
  To: tromey; +Cc: GCJ-patches

Hi Tom,

>Mohan> To summarize, I'd like to know exactly what scripts to run
>Mohan> after I apply your patch (and ideally, why I'm running them).
>
>Download ecj.jar using the contrib/download_ecj method
>Configure with --enable-java-maintainer-mode.
>Apply patch.
>./scripts/makemake.tcl > sources.am
>automake
>Build.
>When the build fails, run classes.pl and insert output into javaprims.h
>Rebuild

I did the following (trying to do a native i686-pc-linux-gnu build):

- Checked out svn trunk to /datal/gcc
- Went to /datal/gcc/gcc and ran contrib/download_ecj
- Configured and built a Linux native gcc without
  --enable-java-maintainer-mode
- Moved the resultant compiler to /datal/gcc/nativegcc
- Put /datal/gcc/nativegcc/bin in my path and 
  /datal/gcc/nativegcc/lib in LD_LIBRARY_PATH
- Created the following script called ecj1 in /datal/gcc/nativegcc/bin:

======================
#! /bin/sh
gij -cp /datal/gcc/gcc/ecj.jar org.eclipse.jdt.internal.compiler.batch.GCCMain ${1+"$@"}
======================

- Blew away my build directory
- Went to /datal/gcc/gcc/libjava
- With automake 1.96 and autoconf 2.59 in my path, did:

======================
<Apply patch.>
./scripts/makemake.tcl > sources.am
automake
======================

- Configured gcc like this:

======================
$GCC_SRC_DIR/configure --prefix=$PREFIX \
    --enable-languages=c,c++,java \
    --with-mpfr=$GMP_MPFR_DIR \
    --with-gmp=$GMP_MPFR_DIR \
    --with-gcc --with-gnu-as --with-gnu-ld \
    --enable-threads=posix --disable-nls \
    --enable-shared --disable-debug \
    --enable-libgcj  --disable-java-awt --without-x \
    --disable-libgcj-debug --enable-interpreter --enable-hash-synchronization \
    --enable-java-maintainer-mode
======================

- Built.

The build fails, but no VMProcess.class (new class in patch) is built.

What's more, the --enable-java-maintainer-mode switch isn't present
in $builddir/i686-pc-linux-gnu/libjava/config.log. I also get these in config.log:

JAVA_MAINTAINER_MODE_FALSE=''
JAVA_MAINTAINER_MODE_TRUE='#'

Am I missing something? Do I need to do --enable-maintainer-mode on top of
--enable-java-maintainer-mode? Why isn't this flag being passed down?

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-13  2:08     ` Mohan Embar
@ 2007-02-13 18:04       ` Tom Tromey
  2007-02-13 20:25         ` Tom Tromey
  0 siblings, 1 reply; 29+ messages in thread
From: Tom Tromey @ 2007-02-13 18:04 UTC (permalink / raw)
  To: gnustuff; +Cc: GCJ-patches

>>>>> "Mohan" == Mohan Embar <gnustuff@thisiscool.com> writes:

Mohan> What's more, the --enable-java-maintainer-mode switch isn't
Mohan> present in $builddir/i686-pc-linux-gnu/libjava/config.log. I
Mohan> also get these in config.log:

Hmm.  Well, I don't know what is happening here, but this is
definitely the problem.

Mohan> Am I missing something? Do I need to do
Mohan> --enable-maintainer-mode on top of
Mohan> --enable-java-maintainer-mode? Why isn't this flag being passed
Mohan> down?

No clue :(.

Let me do a full update & build here, to see if I can reproduce.

Tom

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-13 18:04       ` Tom Tromey
@ 2007-02-13 20:25         ` Tom Tromey
  2007-02-13 22:01           ` Tom Tromey
  0 siblings, 1 reply; 29+ messages in thread
From: Tom Tromey @ 2007-02-13 20:25 UTC (permalink / raw)
  To: gnustuff; +Cc: GCJ-patches

Mohan> Am I missing something? Do I need to do
Mohan> --enable-maintainer-mode on top of
Mohan> --enable-java-maintainer-mode? Why isn't this flag being passed
Mohan> down?

Tom> Let me do a full update & build here, to see if I can reproduce.

Hmm.  It worked fine for me.
I don't get it :(

Tom

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-13 20:25         ` Tom Tromey
@ 2007-02-13 22:01           ` Tom Tromey
  2007-02-14  1:55             ` Mohan Embar
  0 siblings, 1 reply; 29+ messages in thread
From: Tom Tromey @ 2007-02-13 22:01 UTC (permalink / raw)
  To: gnustuff; +Cc: GCJ-patches

BTW, Mohan -- if it would be simpler for you if I check in the patch,
I am happy to do this.

Tom

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-13 22:01           ` Tom Tromey
@ 2007-02-14  1:55             ` Mohan Embar
  2007-02-14  2:01               ` Tom Tromey
  2007-02-15 17:33               ` Mohan Embar
  0 siblings, 2 replies; 29+ messages in thread
From: Mohan Embar @ 2007-02-14  1:55 UTC (permalink / raw)
  To: tromey; +Cc: GCJ-patches

Hi Tom,

>BTW, Mohan -- if it would be simpler for you if I check in the patch,
>I am happy to do this.

I think I found the problem. I was configuring with an old script without
--enable-java-maintainer-mode because my paths were messed up. Sorry for
the confusion and that I made you do a whole new build. Serves me right
for doing this in a distracted manner....

I want to take another stab at figuring this out on my end. I'm redoing
the build. Stay tuned.

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-14  1:55             ` Mohan Embar
@ 2007-02-14  2:01               ` Tom Tromey
  2007-02-15 17:33               ` Mohan Embar
  1 sibling, 0 replies; 29+ messages in thread
From: Tom Tromey @ 2007-02-14  2:01 UTC (permalink / raw)
  To: gnustuff; +Cc: GCJ-patches

Mohan> I think I found the problem. I was configuring with an old
Mohan> script without --enable-java-maintainer-mode because my paths
Mohan> were messed up. Sorry for the confusion and that I made you do
Mohan> a whole new build. Serves me right for doing this in a
Mohan> distracted manner....

No problem.  I'm relieved to hear it isn't a bug in our build.

Mohan> I want to take another stab at figuring this out on my end. I'm
Mohan> redoing the build. Stay tuned.

I also have another (simpler) patch, in a different area, that I would
like you to try when you have a moment.  In this case I made the
windows changes as well (the change is trivial), but I wanted to make
sure it built before going ahead.

Tom

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-14  1:55             ` Mohan Embar
  2007-02-14  2:01               ` Tom Tromey
@ 2007-02-15 17:33               ` Mohan Embar
  2007-02-15 17:35                 ` David Daney
  2007-02-21 15:43                 ` Mohan Embar
  1 sibling, 2 replies; 29+ messages in thread
From: Mohan Embar @ 2007-02-15 17:33 UTC (permalink / raw)
  To: tromey; +Cc: GCJ-patches

Hi Tom,

>I want to take another stab at figuring this out on my end. I'm redoing
>the build. Stay tuned.

Quick status: I successfully built the Linux native compiler with
--enable-java-maintainer-mode. When I tried building the Windows cross
compiler, the build choked because the cross assembler didn't recognize
flag -Qy. I'm crossing my fingers and hoping this was a temporary glitch
in the source tree; I'm regetting pristine sources today so I can start over
from scratch.

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-15 17:33               ` Mohan Embar
@ 2007-02-15 17:35                 ` David Daney
  2007-02-17  1:55                   ` Mohan Embar
  2007-02-21 15:43                 ` Mohan Embar
  1 sibling, 1 reply; 29+ messages in thread
From: David Daney @ 2007-02-15 17:35 UTC (permalink / raw)
  To: gnustuff; +Cc: tromey, GCJ-patches

Mohan Embar wrote:
> Hi Tom,
> 
>> I want to take another stab at figuring this out on my end. I'm redoing
>> the build. Stay tuned.
> 
> Quick status: I successfully built the Linux native compiler with
> --enable-java-maintainer-mode. When I tried building the Windows cross
> compiler, the build choked because the cross assembler didn't recognize
> flag -Qy. I'm crossing my fingers and hoping this was a temporary glitch
> in the source tree; I'm regetting pristine sources today so I can start over
> from scratch.
> 

For cross builds, the GCC build can use the wrong assembler at times if 
the target assembler is not in the PATH both at configure and make time.

David Daney

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-15 17:35                 ` David Daney
@ 2007-02-17  1:55                   ` Mohan Embar
  0 siblings, 0 replies; 29+ messages in thread
From: Mohan Embar @ 2007-02-17  1:55 UTC (permalink / raw)
  To: David Daney; +Cc: tromey, GCJ-patches

Hi David,

>For cross builds, the GCC build can use the wrong assembler at times if 
>the target assembler is not in the PATH both at configure and make time.

From the post I just sent, it seems to be the other way around. Something
that should be using the build assembler is mistakenly invoking the cross
assembler.

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-15 17:33               ` Mohan Embar
  2007-02-15 17:35                 ` David Daney
@ 2007-02-21 15:43                 ` Mohan Embar
  2007-02-22  1:16                   ` Tom Tromey
  1 sibling, 1 reply; 29+ messages in thread
From: Mohan Embar @ 2007-02-21 15:43 UTC (permalink / raw)
  To: tromey; +Cc: GCJ-patches

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

Hi Tom,

I finally was able to build this after messing around with my paths
even more.

The attached testcase hangs on my machine on Linux with gcj, but
works with Sun Java. Would you be able to try this out on your end?
Once this works on Linux, I can move on to the Windows piece.
I would offer to troubleshoot this on Linux, but I'm really short on
time lately :(

1. Compile Test.java to Test.exe
2. Add the directory with Test.exe to your path.
3. Altenatively compile ProcessBuilderTest.java to a classfile
  and run with Sun Java (it works), or else compile ProcessBuilderTest.java
  to an executable with gcj, then try running the executable (it hangs).

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/


[-- Attachment #2: Test.java --]
[-- Type: application/octet-stream, Size: 207 bytes --]

import java.io.InputStream;
import java.io.IOException;

public class Test
{
  public static void main(String[] args)
  {
    System.out.println("Hello");
    System.err.println("World!*");
  }
}

[-- Attachment #3: ProcessBuilderTest.java --]
[-- Type: application/octet-stream, Size: 572 bytes --]

import java.io.InputStream;
import java.io.IOException;

public class ProcessBuilderTest
{
  public static void main(String[] args)
  {
    try
    {
      ProcessBuilder p1 = new ProcessBuilder("Test.exe");
      p1.redirectErrorStream(true);
      Process p = p1.start();
      InputStream istr = p.getInputStream();
      int nch;
      while ((nch = istr.read()) != -1)
      {
        System.out.print((char) nch);
        if (nch == 42)
          break;
      }
    }
    catch (IOException e)
    {
      e.printStackTrace();
    }
  }
}

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-21 15:43                 ` Mohan Embar
@ 2007-02-22  1:16                   ` Tom Tromey
  2007-02-26 23:12                     ` Mohan Embar
  0 siblings, 1 reply; 29+ messages in thread
From: Tom Tromey @ 2007-02-22  1:16 UTC (permalink / raw)
  To: gnustuff; +Cc: tromey, GCJ-patches

>>>>> "Mohan" == Mohan Embar <gnustuff@thisiscool.com> writes:

Mohan> The attached testcase hangs on my machine on Linux with gcj, but
Mohan> works with Sun Java. Would you be able to try this out on your end?
Mohan> Once this works on Linux, I can move on to the Windows piece.
Mohan> I would offer to troubleshoot this on Linux, but I'm really short on
Mohan> time lately :(

I'm away right now & don't have this readily available.  The patch I
sent to the list had a little bug.  There is one conditional close()
(in the patch the condition was added) that closes 2 fds but should
only close one.

Tom

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-22  1:16                   ` Tom Tromey
@ 2007-02-26 23:12                     ` Mohan Embar
  2007-02-28 15:53                       ` Tom Tromey
  0 siblings, 1 reply; 29+ messages in thread
From: Mohan Embar @ 2007-02-26 23:12 UTC (permalink / raw)
  To: tromey; +Cc: GCJ-patches

Hi Tom,

>Mohan> The attached testcase hangs on my machine on Linux with gcj, but
>Mohan> works with Sun Java. Would you be able to try this out on your end?
>Mohan> Once this works on Linux, I can move on to the Windows piece.
>Mohan> I would offer to troubleshoot this on Linux, but I'm really short on
>Mohan> time lately :(

>I'm away right now & don't have this readily available.  The patch I
>sent to the list had a little bug.  There is one conditional close()
>(in the patch the condition was added) that closes 2 fds but should
>only close one.

I tried doing what I think you said and it still hangs. In any case, I can confirm
that this doesn't break the MinGW build and since I've been dawdling with this,
my vote would be that you just check this in and I'll sort things out on my end
once you've done so.

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-26 23:12                     ` Mohan Embar
@ 2007-02-28 15:53                       ` Tom Tromey
  2007-03-05 13:47                         ` Tom Tromey
  0 siblings, 1 reply; 29+ messages in thread
From: Tom Tromey @ 2007-02-28 15:53 UTC (permalink / raw)
  To: gnustuff; +Cc: tromey, GCJ-patches

>>>>> "Mohan" == Mohan Embar <gnustuff@thisiscool.com> writes:

Mohan> I tried doing what I think you said and it still hangs. In any
Mohan> case, I can confirm that this doesn't break the MinGW build and
Mohan> since I've been dawdling with this, my vote would be that you
Mohan> just check this in and I'll sort things out on my end once
Mohan> you've done so.

Ok.  I'll try your test case against my patch before doing anything on
trunk.

Tom

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
  2007-02-28 15:53                       ` Tom Tromey
@ 2007-03-05 13:47                         ` Tom Tromey
  2007-03-07  4:11                           ` class Character$UnicodeBlock$NameType; (was Re: Patch: RFC: fix ProcessBuilder; windows help needed) Mohan Embar
  2007-03-07 15:39                           ` Patch [MinGW]: ProcessBuilder and redirect Mohan Embar
  0 siblings, 2 replies; 29+ messages in thread
From: Tom Tromey @ 2007-03-05 13:47 UTC (permalink / raw)
  To: gnustuff; +Cc: GCJ-patches

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

>>>>> "Mohan" == Mohan Embar <gnustuff@thisiscool.com> writes:
Mohan> I tried doing what I think you said and it still hangs. In any
Mohan> case, I can confirm that this doesn't break the MinGW build and
Mohan> since I've been dawdling with this, my vote would be that you
Mohan> just check this in and I'll sort things out on my end once
Mohan> you've done so.

Tom> Ok.  I'll try your test case against my patch before doing anything on
Tom> trunk.

It worked for me.  I'm going to commit this patch today.

Tom

^ permalink raw reply	[flat|nested] 29+ messages in thread

* class Character$UnicodeBlock$NameType; (was Re: Patch: RFC: fix ProcessBuilder; windows help needed)
  2007-03-05 13:47                         ` Tom Tromey
@ 2007-03-07  4:11                           ` Mohan Embar
  2007-03-07 14:37                             ` Tom Tromey
  2007-03-07 15:39                           ` Patch [MinGW]: ProcessBuilder and redirect Mohan Embar
  1 sibling, 1 reply; 29+ messages in thread
From: Mohan Embar @ 2007-03-07  4:11 UTC (permalink / raw)
  To: tromey; +Cc: GCJ-patches

Hi Tom,

I'm gearing up to work on this. I added an EOFInputStream to
Win32Process.java and then regenerated gcj/javaprims.h. In
doing so, it picked up an extra class. Not quite sure what you
want to do about this, so I thought I'd mention it:

Index: gcj/javaprims.h
===================================================================
--- gcj/javaprims.h     (revision 122579)
+++ gcj/javaprims.h     (working copy)
@@ -153,6 +153,7 @@
       class Character;
       class Character$Subset;
       class Character$UnicodeBlock;
+      class Character$UnicodeBlock$NameType;
       class Class;
       class ClassCastException;
       class ClassCircularityError;
@@ -254,6 +255,7 @@
       class VirtualMachineError;
       class Void;
       class Win32Process;
+      class Win32Process$EOFInputStream;
       namespace annotation
       {
         class Annotation;

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: class Character$UnicodeBlock$NameType; (was Re: Patch: RFC: fix ProcessBuilder; windows help needed)
  2007-03-07  4:11                           ` class Character$UnicodeBlock$NameType; (was Re: Patch: RFC: fix ProcessBuilder; windows help needed) Mohan Embar
@ 2007-03-07 14:37                             ` Tom Tromey
  2007-03-07 14:55                               ` class Character$UnicodeBlock$NameType; Mohan Embar
  0 siblings, 1 reply; 29+ messages in thread
From: Tom Tromey @ 2007-03-07 14:37 UTC (permalink / raw)
  To: gnustuff; +Cc: GCJ-patches

>>>>> "Mohan" == Mohan Embar <gnustuff@thisiscool.com> writes:

Mohan> I'm gearing up to work on this. I added an EOFInputStream to
Mohan> Win32Process.java and then regenerated gcj/javaprims.h. In
Mohan> doing so, it picked up an extra class. Not quite sure what you
Mohan> want to do about this, so I thought I'd mention it:

Thanks, I forgot about this.
Please check it in.

Tom

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: class Character$UnicodeBlock$NameType;
  2007-03-07 14:37                             ` Tom Tromey
@ 2007-03-07 14:55                               ` Mohan Embar
  0 siblings, 0 replies; 29+ messages in thread
From: Mohan Embar @ 2007-03-07 14:55 UTC (permalink / raw)
  To: tromey; +Cc: GCJ-patches

>Mohan> I'm gearing up to work on this. I added an EOFInputStream to
>Mohan> Win32Process.java and then regenerated gcj/javaprims.h. In
>Mohan> doing so, it picked up an extra class. Not quite sure what you
>Mohan> want to do about this, so I thought I'd mention it:
>
>Thanks, I forgot about this.
>Please check it in.

Done.

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Patch [MinGW]: ProcessBuilder and redirect
  2007-03-05 13:47                         ` Tom Tromey
  2007-03-07  4:11                           ` class Character$UnicodeBlock$NameType; (was Re: Patch: RFC: fix ProcessBuilder; windows help needed) Mohan Embar
@ 2007-03-07 15:39                           ` Mohan Embar
  2007-03-07 16:58                             ` Tom Tromey
  1 sibling, 1 reply; 29+ messages in thread
From: Mohan Embar @ 2007-03-07 15:39 UTC (permalink / raw)
  To: tromey; +Cc: GCJ-patches

Hi Tom,

This makes the redirect flag work on Win32. Could you
glance at it to see if I've got all of the new / regenerated
files covered and that I haven't made any flagrant formatting
mistakes?

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/

2007-03-07  Mohan Embar  <gnustuff@thisiscool.com>

	* java/lang/Win32Process.java: Added nested class EOFInputStream.
	* java/lang/natWin32Process.cc (ChildProcessPipe): Added DUMMY
	enum and implementation.
	(startProcess): Use redirect flag.
	* classpath/lib/java/lang/Win32Process.class: Regenerated.
	* classpath/lib/java/lang/Win32Process$EOFInputStream.class: New.
	* gcj/javaprims.h: Regenerated.
	* java/lang/Win32Process$EOFInputStream.h: New.

Index: java/lang/Win32Process.java
===================================================================
--- java/lang/Win32Process.java	(revision 122579)
+++ java/lang/Win32Process.java	(working copy)
@@ -85,4 +85,13 @@
 				    boolean redirect)
     throws IOException;
   private native void cleanup ();
+
+  private static class EOFInputStream extends InputStream
+  {
+    static EOFInputStream instance = new EOFInputStream();
+    public int read()
+    {
+      return -1;
+    }
+  }
 }
Index: java/lang/natWin32Process.cc
===================================================================
--- java/lang/natWin32Process.cc	(revision 122579)
+++ java/lang/natWin32Process.cc	(working copy)
@@ -25,6 +25,7 @@
 #include <java/io/FileOutputStream.h>
 #include <java/io/IOException.h>
 #include <java/lang/OutOfMemoryError.h>
+#include <java/lang/Win32Process$EOFInputStream.h>
 #include <gnu/java/nio/channels/FileChannelImpl.h>
 
 using gnu::java::nio::channels::FileChannelImpl;
@@ -146,7 +147,7 @@
 public:
   // Indicates from the child process' point of view
   // whether the pipe is for reading or writing.
-  enum EType {INPUT, OUTPUT};
+  enum EType {INPUT, OUTPUT, DUMMY};
 
   ChildProcessPipe(EType eType);
   ~ChildProcessPipe();
@@ -163,8 +164,11 @@
 };
 
 ChildProcessPipe::ChildProcessPipe(EType eType):
-  m_eType(eType)
+  m_eType(eType), m_hRead(0), m_hWrite(0)
 {
+  if (eType == DUMMY)
+    return;
+  
   SECURITY_ATTRIBUTES sAttrs;
 
   // Explicitly allow the handles to the pipes to be inherited.
@@ -195,7 +199,8 @@
   // Close the parent end of the pipe. This
   // destructor is called after the child process
   // has been spawned.
-  CloseHandle(getChildHandle());
+  if (m_eType != DUMMY)
+    CloseHandle(getChildHandle());
 }
 
 HANDLE ChildProcessPipe::getParentHandle()
@@ -284,7 +289,8 @@
       // on each of standard streams.
       ChildProcessPipe aChildStdIn(ChildProcessPipe::INPUT);
       ChildProcessPipe aChildStdOut(ChildProcessPipe::OUTPUT);
-      ChildProcessPipe aChildStdErr(ChildProcessPipe::OUTPUT);
+      ChildProcessPipe aChildStdErr(redirect ? ChildProcessPipe::DUMMY
+				    : ChildProcessPipe::OUTPUT);
 
       outputStream = new FileOutputStream (new FileChannelImpl (
                            (jint) aChildStdIn.getParentHandle (),
@@ -292,7 +298,10 @@
       inputStream = new FileInputStream (new FileChannelImpl (
                            (jint) aChildStdOut.getParentHandle (),
 			   FileChannelImpl::READ));
-      errorStream = new FileInputStream (new FileChannelImpl (
+      if (redirect)
+        errorStream = Win32Process$EOFInputStream::instance;
+      else
+        errorStream = new FileInputStream (new FileChannelImpl (
                            (jint) aChildStdErr.getParentHandle (),
 			   FileChannelImpl::READ));
 
@@ -310,7 +319,8 @@
 
       si.hStdInput = aChildStdIn.getChildHandle();
       si.hStdOutput = aChildStdOut.getChildHandle();
-      si.hStdError = aChildStdErr.getChildHandle();
+      si.hStdError = redirect ? aChildStdOut.getChildHandle()
+                              : aChildStdErr.getChildHandle();
 
       // Spawn the process. CREATE_NO_WINDOW only applies when
       // starting a console application; it suppresses the



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch [MinGW]: ProcessBuilder and redirect
  2007-03-07 15:39                           ` Patch [MinGW]: ProcessBuilder and redirect Mohan Embar
@ 2007-03-07 16:58                             ` Tom Tromey
  0 siblings, 0 replies; 29+ messages in thread
From: Tom Tromey @ 2007-03-07 16:58 UTC (permalink / raw)
  To: gnustuff; +Cc: GCJ-patches

>>>>> "Mohan" == Mohan Embar <gnustuff@thisiscool.com> writes:

Mohan> This makes the redirect flag work on Win32. Could you
Mohan> glance at it to see if I've got all of the new / regenerated
Mohan> files covered and that I haven't made any flagrant formatting
Mohan> mistakes?

Looks reasonable to me.  Thanks.

Tom

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Patch: RFC: fix ProcessBuilder; windows help needed
       [not found] <KGA5WRQOB6SM2ZC8PLE4WR06DA72Q.45d226cf@parallels>
@ 2007-02-13 22:25 ` Tom Tromey
  0 siblings, 0 replies; 29+ messages in thread
From: Tom Tromey @ 2007-02-13 22:25 UTC (permalink / raw)
  To: gnustuff; +Cc: GCJ-patches

>>>>> "Mohan" == Mohan Embar <gnustuff@thisiscool.com> writes:

Mohan> Do I need --enable-maintainer-mode too?

Nope.  I don't use it, typically.

Mohan> Did you check the configure flags I sent you in my previous
Mohan> post? Anything suspect?

Looked reasonable enough.

Mohan> Also, the
Mohan> shell script you said that needed to be made is called "ecj1", right?
Mohan> Not "ecjx" or something different?

Right.  ecj1 and on your path.  'ecjx' is the temporary name we use
before installing it.  This is important so that the gcj we're running
in the libjava build dir doesn't accidentally pick up an ecj1 in that
directory.

Tom

^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2007-03-07 16:58 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-02-03  0:24 Patch: RFC: fix ProcessBuilder; windows help needed Tom Tromey
2007-02-03 18:11 ` Mohan Embar
2007-02-04  9:47   ` Marco Trudel
2007-02-04 10:09   ` Mohan Embar
2007-02-05 21:05     ` Tom Tromey
2007-02-06 17:58       ` Mohan Embar
2007-02-07 16:13         ` Tom Tromey
2007-02-11 19:08 ` Mohan Embar
2007-02-12  0:12   ` Tom Tromey
2007-02-13  2:08     ` Mohan Embar
2007-02-13 18:04       ` Tom Tromey
2007-02-13 20:25         ` Tom Tromey
2007-02-13 22:01           ` Tom Tromey
2007-02-14  1:55             ` Mohan Embar
2007-02-14  2:01               ` Tom Tromey
2007-02-15 17:33               ` Mohan Embar
2007-02-15 17:35                 ` David Daney
2007-02-17  1:55                   ` Mohan Embar
2007-02-21 15:43                 ` Mohan Embar
2007-02-22  1:16                   ` Tom Tromey
2007-02-26 23:12                     ` Mohan Embar
2007-02-28 15:53                       ` Tom Tromey
2007-03-05 13:47                         ` Tom Tromey
2007-03-07  4:11                           ` class Character$UnicodeBlock$NameType; (was Re: Patch: RFC: fix ProcessBuilder; windows help needed) Mohan Embar
2007-03-07 14:37                             ` Tom Tromey
2007-03-07 14:55                               ` class Character$UnicodeBlock$NameType; Mohan Embar
2007-03-07 15:39                           ` Patch [MinGW]: ProcessBuilder and redirect Mohan Embar
2007-03-07 16:58                             ` Tom Tromey
     [not found] <KGA5WRQOB6SM2ZC8PLE4WR06DA72Q.45d226cf@parallels>
2007-02-13 22:25 ` Patch: RFC: fix ProcessBuilder; windows help needed Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).