c-api.test fails on MS-Windows due to non-portable quoting

c-api.test fails on MS-Windows due to non-portable quoting

[Eli Zaretskii]

It fails like this:

     Running c-api.test
     'CUR' is not recognized as an internal or external command,
     operable program or batch file.
     egrep: Unmatched ( or \('CUR' is not recognized as an internal or external command, operable program or batch file.

This is because it quotes shell commands /bin/sh '..' style:

     (define (egrep string filename)
       (zero? (system (string-append "egrep '" string "' " filename
				     " >" %null-device))))

The solution is to use the ".." style of quoting:

--- test-suite/tests/c-api.test~0	2016-01-02 13:32:40.000000000 +0200
+++ test-suite/tests/c-api.test	2016-07-23 14:12:57.257375000 +0300
@@ -22,7 +22,7 @@
 (define srcdir (cdr (assq 'srcdir %guile-build-info)))
 
 (define (egrep string filename)
-  (zero? (system (string-append "egrep '" string "' " filename
+  (zero? (system (string-append "egrep \"" string "\" " filename
                                 " >" %null-device))))
 
 (define (seek-offset-test dirname)

OK to push such a change?

Re: c-api.test fails on MS-Windows due to non-portable quoting

[Andy Wingo]

On Sat 23 Jul 2016 13:18, Eli Zaretskii <eliz@gnu.org> writes:

> It fails like this:
>
>      Running c-api.test
>      'CUR' is not recognized as an internal or external command,
>      operable program or batch file.
>      egrep: Unmatched ( or \('CUR' is not recognized as an internal or external command, operable program or batch file.
>
> This is because it quotes shell commands /bin/sh '..' style:

Of course, because that's how `system' is specified.

> --- test-suite/tests/c-api.test~0	2016-01-02 13:32:40.000000000 +0200
> +++ test-suite/tests/c-api.test	2016-07-23 14:12:57.257375000 +0300
> @@ -22,7 +22,7 @@
>  (define srcdir (cdr (assq 'srcdir %guile-build-info)))
>  
>  (define (egrep string filename)
> -  (zero? (system (string-append "egrep '" string "' " filename
> +  (zero? (system (string-append "egrep \"" string "\" " filename
>                                  " >" %null-device))))
>  
>  (define (seek-offset-test dirname)
>
> OK to push such a change?

I think instead to get this to work on MinGW we should switch to use
system* instead of praying that we get quoting right ;) Something like:

  (zero? (system* "egrep" "-q" string filename))

Andy

Re: c-api.test fails on MS-Windows due to non-portable quoting

[Eli Zaretskii]

> From: Andy Wingo <wingo@pobox.com>
> Cc: guile-devel@gnu.org
> Date: Sat, 23 Jul 2016 23:11:08 +0200
> 
> > It fails like this:
> >
> >      Running c-api.test
> >      'CUR' is not recognized as an internal or external command,
> >      operable program or batch file.
> >      egrep: Unmatched ( or \('CUR' is not recognized as an internal or 
> > external command, operable program or batch file.
> >
> > This is because it quotes shell commands /bin/sh '..' style:
> 
> Of course, because that's how `system' is specified.

On Posix hosts, yes.  But the ANSI C standard only says that the
argument will be passed to the host environment's command processor.

> > --- test-suite/tests/c-api.test~0     2016-01-02 13:32:40.000000000 +0200
> > +++ test-suite/tests/c-api.test       2016-07-23 14:12:57.257375000 +0300
> > @@ -22,7 +22,7 @@
> >  (define srcdir (cdr (assq 'srcdir %guile-build-info)))
> >  
> >  (define (egrep string filename)
> > -  (zero? (system (string-append "egrep '" string "' " filename
> > +  (zero? (system (string-append "egrep \"" string "\" " filename
> >                                  " >" %null-device))))
> >  
> >  (define (seek-offset-test dirname)
> >
> > OK to push such a change?
> 
> I think instead to get this to work on MinGW we should switch to use
> system* instead of praying that we get quoting right ;) Something like:
> 
>   (zero? (system* "egrep" "-q" string filename))

For this to work, the Windows implementation of system* will need to
be augmented to quote characters special for the shell, because
(unlike execvp on Posix hosts) the arguments of spawnvp are eventually
concatenated into a single string that gets passed to the system API
which invokes programs.  If this is the way you are willing to solve
this, I will submit a patch to that effect.

Re: c-api.test fails on MS-Windows due to non-portable quoting

[Mark H Weaver]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andy Wingo <wingo@pobox.com>
>> Cc: guile-devel@gnu.org
>> Date: Sat, 23 Jul 2016 23:11:08 +0200
>> 
>> > It fails like this:
>> >
>> >      Running c-api.test
>> >      'CUR' is not recognized as an internal or external command,
>> >      operable program or batch file.
>> >      egrep: Unmatched ( or \('CUR' is not recognized as an internal or 
>> > external command, operable program or batch file.
>> >
>> > This is because it quotes shell commands /bin/sh '..' style:
>> 
>> Of course, because that's how `system' is specified.
>
> On Posix hosts, yes.  But the ANSI C standard only says that the
> argument will be passed to the host environment's command processor.
>
>> > --- test-suite/tests/c-api.test~0     2016-01-02 13:32:40.000000000 +0200
>> > +++ test-suite/tests/c-api.test       2016-07-23 14:12:57.257375000 +0300
>> > @@ -22,7 +22,7 @@
>> >  (define srcdir (cdr (assq 'srcdir %guile-build-info)))
>> >  
>> >  (define (egrep string filename)
>> > -  (zero? (system (string-append "egrep '" string "' " filename
>> > +  (zero? (system (string-append "egrep \"" string "\" " filename
>> >                                  " >" %null-device))))
>> >  
>> >  (define (seek-offset-test dirname)
>> >
>> > OK to push such a change?
>> 
>> I think instead to get this to work on MinGW we should switch to use
>> system* instead of praying that we get quoting right ;) Something like:
>> 
>>   (zero? (system* "egrep" "-q" string filename))
>
> For this to work, the Windows implementation of system* will need to
> be augmented to quote characters special for the shell, because
> (unlike execvp on Posix hosts) the arguments of spawnvp are eventually
> concatenated into a single string that gets passed to the system API
> which invokes programs.  If this is the way you are willing to solve
> this, I will submit a patch to that effect.

I think this is the approach we should take here.  In general, we prefer
to use 'system*' over 'system' because it avoids having to worry about
quoting issues on POSIX systems.  We should enable this to work properly
on MinGW, and avoid a situation where users might be discouraged from
using 'system*' for the sake of MinGW compatibility.

What do you think?

      Mark

Re: c-api.test fails on MS-Windows due to non-portable quoting

[Eli Zaretskii]

> From: Mark H Weaver <mhw@netris.org>
> Cc: Andy Wingo <wingo@pobox.com>,  guile-devel@gnu.org
> Date: Wed, 10 Aug 2016 02:24:56 -0400
> 
> >>   (zero? (system* "egrep" "-q" string filename))
> >
> > For this to work, the Windows implementation of system* will need to
> > be augmented to quote characters special for the shell, because
> > (unlike execvp on Posix hosts) the arguments of spawnvp are eventually
> > concatenated into a single string that gets passed to the system API
> > which invokes programs.  If this is the way you are willing to solve
> > this, I will submit a patch to that effect.
> 
> I think this is the approach we should take here.  In general, we prefer
> to use 'system*' over 'system' because it avoids having to worry about
> quoting issues on POSIX systems.  We should enable this to work properly
> on MinGW, and avoid a situation where users might be discouraged from
> using 'system*' for the sake of MinGW compatibility.
> 
> What do you think?

If you suggest to do what I described above, then I obviously agree.

Thanks.

Re: c-api.test fails on MS-Windows due to non-portable quoting

[Eli Zaretskii]

> Date: Wed, 10 Aug 2016 17:26:15 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: wingo@pobox.com, guile-devel@gnu.org
> 
> If you suggest to do what I described above, then I obviously agree.

IOW, do you want me to send a patch along the lines I suggested?

Re: c-api.test fails on MS-Windows due to non-portable quoting

[Mark H Weaver]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Wed, 10 Aug 2016 17:26:15 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: wingo@pobox.com, guile-devel@gnu.org
>> 
>> If you suggest to do what I described above, then I obviously agree.
>
> IOW, do you want me to send a patch along the lines I suggested?

Yes, please!

    Thanks,
      Mark

Re: c-api.test fails on MS-Windows due to non-portable quoting

[Eli Zaretskii]

> From: Mark H Weaver <mhw@netris.org>
> Cc: wingo@pobox.com,  guile-devel@gnu.org
> Date: Wed, 10 Aug 2016 13:03:09 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Date: Wed, 10 Aug 2016 17:26:15 +0300
> >> From: Eli Zaretskii <eliz@gnu.org>
> >> Cc: wingo@pobox.com, guile-devel@gnu.org
> >> 
> >> If you suggest to do what I described above, then I obviously agree.
> >
> > IOW, do you want me to send a patch along the lines I suggested?
> 
> Yes, please!

Will do when I have enough time (correctly quoting command-line
arguments on Windows is a tricky business).

Re: c-api.test fails on MS-Windows due to non-portable quoting

[Eli Zaretskii]

> Date: Sat, 13 Aug 2016 12:11:33 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: wingo@pobox.com, guile-devel@gnu.org
> 
> > From: Mark H Weaver <mhw@netris.org>
> > Cc: wingo@pobox.com,  guile-devel@gnu.org
> > Date: Wed, 10 Aug 2016 13:03:09 -0400
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > >> Date: Wed, 10 Aug 2016 17:26:15 +0300
> > >> From: Eli Zaretskii <eliz@gnu.org>
> > >> Cc: wingo@pobox.com, guile-devel@gnu.org
> > >> 
> > >> If you suggest to do what I described above, then I obviously agree.
> > >
> > > IOW, do you want me to send a patch along the lines I suggested?
> > 
> > Yes, please!
> 
> Will do when I have enough time (correctly quoting command-line
> arguments on Windows is a tricky business).

On further thought, I decided to reuse code we already have, rather
than write something new.  Is the approach below acceptable?  If it
is, I will post it in Git format wrt the current repo.

--- libguile/simpos.c~0	2016-01-02 16:24:55.000000000 +0200
+++ libguile/simpos.c	2016-08-13 13:56:43.014875000 +0300
@@ -45,12 +45,12 @@
 # include <sys/wait.h>
 #endif
 
+#include "posix.h"
+
 #ifdef __MINGW32__
-# include <process.h>	/* for spawnvp and friends */
+#include "posix-w32.h"
 #endif
 
-#include "posix.h"
-
 \f
 extern int system();
 \f
@@ -124,9 +124,9 @@ SCM_DEFINE (scm_system_star, "system*", 
       SCM oldquit;
       SCM sigquit;
 #endif
-#ifdef HAVE_FORK
       int pid;
-#else
+#ifndef HAVE_FORK
+      int p1[2], p2[2];
       int status;
 #endif
       char **execargv;
@@ -177,7 +177,21 @@ SCM_DEFINE (scm_system_star, "system*", 
           return scm_from_int (status);
         }
 #else  /* !HAVE_FORK */
+#ifdef __MINGW32__
+      /* MS-Windows spawnvp needs execargv[] strings quoted if they
+	 include special characters, like whitespace.  The required
+	 quoting is non-trivial, and also depends on whether
+	 execargv[0] is cmd.exe.  So we invoke start_child instead,
+	 which already has all that figured out.  */
+      pid = start_child (execargv[0], execargv, 0, p1, 0, p2,
+			 fileno (stdin), fileno (stdout), fileno (stderr));
+      if (pid == -1)
+        SCM_SYSERROR;
+
+      waitpid (pid, &status, 0);
+#else
       status = spawnvp (P_WAIT, execargv[0], (const char * const *)execargv);
+#endif
       scm_sigaction (sigint, SCM_CAR (oldint), SCM_CDR (oldint));
 #ifdef SIGQUIT
       scm_sigaction (sigquit, SCM_CAR (oldquit), SCM_CDR (oldquit));

Re: c-api.test fails on MS-Windows due to non-portable quoting

[Eli Zaretskii]

Ping!  (2 weeks)

> Date: Sat, 13 Aug 2016 14:55:27 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: wingo@pobox.com, guile-devel@gnu.org
> 
> > Date: Sat, 13 Aug 2016 12:11:33 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: wingo@pobox.com, guile-devel@gnu.org
> > 
> > > From: Mark H Weaver <mhw@netris.org>
> > > Cc: wingo@pobox.com,  guile-devel@gnu.org
> > > Date: Wed, 10 Aug 2016 13:03:09 -0400
> > > 
> > > Eli Zaretskii <eliz@gnu.org> writes:
> > > 
> > > >> Date: Wed, 10 Aug 2016 17:26:15 +0300
> > > >> From: Eli Zaretskii <eliz@gnu.org>
> > > >> Cc: wingo@pobox.com, guile-devel@gnu.org
> > > >> 
> > > >> If you suggest to do what I described above, then I obviously agree.
> > > >
> > > > IOW, do you want me to send a patch along the lines I suggested?
> > > 
> > > Yes, please!
> > 
> > Will do when I have enough time (correctly quoting command-line
> > arguments on Windows is a tricky business).
> 
> On further thought, I decided to reuse code we already have, rather
> than write something new.  Is the approach below acceptable?  If it
> is, I will post it in Git format wrt the current repo.
> 
> --- libguile/simpos.c~0	2016-01-02 16:24:55.000000000 +0200
> +++ libguile/simpos.c	2016-08-13 13:56:43.014875000 +0300
> @@ -45,12 +45,12 @@
>  # include <sys/wait.h>
>  #endif
>  
> +#include "posix.h"
> +
>  #ifdef __MINGW32__
> -# include <process.h>	/* for spawnvp and friends */
> +#include "posix-w32.h"
>  #endif
>  
> -#include "posix.h"
> -
>  \f
>  extern int system();
>  \f
> @@ -124,9 +124,9 @@ SCM_DEFINE (scm_system_star, "system*", 
>        SCM oldquit;
>        SCM sigquit;
>  #endif
> -#ifdef HAVE_FORK
>        int pid;
> -#else
> +#ifndef HAVE_FORK
> +      int p1[2], p2[2];
>        int status;
>  #endif
>        char **execargv;
> @@ -177,7 +177,21 @@ SCM_DEFINE (scm_system_star, "system*", 
>            return scm_from_int (status);
>          }
>  #else  /* !HAVE_FORK */
> +#ifdef __MINGW32__
> +      /* MS-Windows spawnvp needs execargv[] strings quoted if they
> +	 include special characters, like whitespace.  The required
> +	 quoting is non-trivial, and also depends on whether
> +	 execargv[0] is cmd.exe.  So we invoke start_child instead,
> +	 which already has all that figured out.  */
> +      pid = start_child (execargv[0], execargv, 0, p1, 0, p2,
> +			 fileno (stdin), fileno (stdout), fileno (stderr));
> +      if (pid == -1)
> +        SCM_SYSERROR;
> +
> +      waitpid (pid, &status, 0);
> +#else
>        status = spawnvp (P_WAIT, execargv[0], (const char * const *)execargv);
> +#endif
>        scm_sigaction (sigint, SCM_CAR (oldint), SCM_CDR (oldint));
>  #ifdef SIGQUIT
>        scm_sigaction (sigquit, SCM_CAR (oldquit), SCM_CDR (oldquit));
> 
> 

Re: c-api.test fails on MS-Windows due to non-portable quoting

[Andy Wingo]

On Sat 27 Aug 2016 10:23, Eli Zaretskii <eliz@gnu.org> writes:

> Ping!  (2 weeks)

Tx for the ping, and good idea!  I ended up applying something
similar -- I moved system* to posix.c, enabled it iff open-process was
available, and implemented in terms of open-process in a portable way.
Hopefully that should fix the problem.  Thanks again for the patch, it
was a great idea!

Cheers,

Andy

Re: c-api.test fails on MS-Windows due to non-portable quoting

[Eli Zaretskii]

> From: Andy Wingo <wingo@pobox.com>
> Cc: mhw@netris.org,  guile-devel@gnu.org
> Date: Wed, 31 Aug 2016 10:52:10 +0200
> 
> On Sat 27 Aug 2016 10:23, Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Ping!  (2 weeks)
> 
> Tx for the ping, and good idea!  I ended up applying something
> similar -- I moved system* to posix.c, enabled it iff open-process was
> available, and implemented in terms of open-process in a portable way.
> Hopefully that should fix the problem.  Thanks again for the patch, it
> was a great idea!

Thanks, the commit looks good to me.