[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: .ONESEHLL not working as expected in 3.82

From: Paul Smith
Subject: Re: .ONESEHLL not working as expected in 3.82
Date: Sun, 28 Apr 2013 15:15:09 -0400

On Sun, 2013-04-28 at 21:14 +0300, Eli Zaretskii wrote:
> > From: Paul Smith <address@hidden>
> > Cc: address@hidden, address@hidden
> > Date: Sat, 27 Apr 2013 12:54:10 -0400
> > 
> > On Sat, 2013-04-27 at 19:17 +0300, Eli Zaretskii wrote:
> > > The .ONESHELL feature is now supported on MS-Windows, for the default
> > > Windows shell (cmd.exe) or compatible replacements, in the development
> > > version (commit e56aad4).
> > 
> > Nice!
> I see you followed this up with a commit (30843de) which moved code
> around that deals with the one_shell use case.  What was the reason
> for that change?  I think it breaks the use case where a Unixy shell
> is used on MS-Windows under .ONESHELL; this used to work before.

I moved it because the way you had it caused one of the test cases to
fail: you had changed that block of code to be inside the if-statement,
when before it was outside/after the if-statement.

> The code block that you moved is needed on MS-Windows as well, when
> the shell passes the is_bourne_compatible_shell test.

I think you're right, sorry.  I thought that code was inside a unix-only
section but I see now it wasn't.

The code used to look like this:

  if (is_bourne_compatible_shell())
       ... modify the script to remove @-+ ...
       *t = '\0';

  /* create an argv list ... */
    new_argv[n++] = NULL;
  return new_argv;

So, the setup of the new_argv[] was AFTER and outside the if-statement.
In the change you made, it was moved to be INSIDE the if-statement.
That caused a test case to fail because in the situation where we do
have one-shell and we do not have a POSIX shell, new_argv[] was empty
and no command was invoked.

I moved it back, but accidentally made it not work for Windows.

The goal of this code in the if-statement is to implement a special case
allowing ONESHELL to be easier to add in the case where you DO have a
standard shell.  In that case, and ONLY in that case, we remove the
internal @-+ characters.  This allows you to have something like:

          @echo hi
          @echo there
          @echo how are you

And have it continue to work if you add ONESHELL (for performance
reasons) without rewriting all the recipes.

However, if you do NOT have a POSIX shell, then we do NOT remove these
internal characters: we simply provide the script as-is and only the
first line is checked for special characters.  This lets you use
something like Perl, where @ is a special character, for example:

  SHELL = /usr/bin/perl
         @print "hi";
         @array = qw(there how are you);
         print "@array\n";

I think the implementation you have is not quite right.  I think the
parsing of the @-+ stuff is common across all platforms if we have a
shell, so you don't need the "else /* non-posix shell */".

I think it pseudo-code it would look something like this:

  if (posix-shell)
      ...strip out @-+ from LINE...
#ifdef WINDOWS32
  if (need a batch file)
      ...write LINE to the batch file & setup new_argv for batch...
      ...chop LINE up into new_argv...
  return new_argv;

Or something.  Also, I'm not sure about adding things like @echo off to
the batch file.  That assumes that we'll always be using command.com to
run the batch file, but what if the user specified C:/perl/bin/perl.exe
or something as their SHELL?

I'm probably missing something about the implementation though.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]