Patchwork [3,of,3] test-ssh: convert dumpenv to python for Windows

login
register
mail settings
Submitter Matt Harbison
Date Dec. 17, 2017, 12:27 a.m.
Message ID <ca9cde0697c1e923f4c9.1513470444@Envy>
Download mbox | patch
Permalink /patch/26328/
State Accepted
Headers show

Comments

Matt Harbison - Dec. 17, 2017, 12:27 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1513468682 18000
#      Sat Dec 16 18:58:02 2017 -0500
# Node ID ca9cde0697c1e923f4c9a3396343afc238ba92f2
# Parent  d1fbb07b0b7259ae4ca88434e0eda2a21359f7ec
test-ssh: convert dumpenv to python for Windows

Previously, this complained:

    remote: '.' is not recognized as an internal or external command,
    remote: operable program or batch file.

1 out of 4 Linux runs failed to print 17 after converting, and instead printed
an empty remote line.  Not sure why, but maybe the flush will help.
Matt Harbison - Dec. 17, 2017, 6:44 p.m.
> On Dec 16, 2017, at 7:27 PM, Matt Harbison <mharbison72@gmail.com> wrote:
> 
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1513468682 18000
> #      Sat Dec 16 18:58:02 2017 -0500
> # Node ID ca9cde0697c1e923f4c9a3396343afc238ba92f2
> # Parent  d1fbb07b0b7259ae4ca88434e0eda2a21359f7ec
> test-ssh: convert dumpenv to python for Windows
> 
> Previously, this complained:
> 
>    remote: '.' is not recognized as an internal or external command,
>    remote: operable program or batch file.
> 
> 1 out of 4 Linux runs failed to print 17 after converting, and instead printed
> an empty remote line.  Not sure why, but maybe the flush will help.
> 
> diff --git a/tests/test-ssh.t b/tests/test-ssh.t
> --- a/tests/test-ssh.t
> +++ b/tests/test-ssh.t
> @@ -598,17 +598,19 @@
>   [255]
> 
> test that custom environment is passed down to ssh executable
> -  $ cat >>dumpenv <<EOF
> -  > #! /bin/sh
> -  > echo \$VAR >&2
> +  $ cat >>dumpenv.py <<EOF
> +  > from __future__ import print_function
> +  > import sys
> +  > from mercurial import encoding
> +  > print('%s' % encoding.environ.get('VAR', ''), file=sys.stderr)
> +  > sys.stderr.flush()
>> EOF
> -  $ chmod +x dumpenv
> -  $ hg pull ssh://something --config ui.ssh="./dumpenv"
> +  $ hg pull ssh://something --config ui.ssh="$PYTHON dumpenv.py"
>   pulling from ssh://something/
>   remote: 
>   abort: no suitable response from remote hg!
>   [255]
> -  $ hg pull ssh://something --config ui.ssh="./dumpenv" --config sshenv.VAR=17
> +  $ hg pull ssh://something --config ui.ssh="$PYTHON dumpenv.py" --config sshenv.VAR=17
>   pulling from ssh://something/
>   remote: 17

No idea why, but if I run this in my Linux VM with -j9, it wants to append an extra “remote: “ at this point.  If run without workers, it appears stable.

>   abort: no suitable response from remote hg!
Gregory Szorc - Dec. 17, 2017, 7:07 p.m.
On Sun, Dec 17, 2017 at 10:44 AM, Matt Harbison <mharbison72@gmail.com>
wrote:

>
> > On Dec 16, 2017, at 7:27 PM, Matt Harbison <mharbison72@gmail.com>
> wrote:
> >
> > # HG changeset patch
> > # User Matt Harbison <matt_harbison@yahoo.com>
> > # Date 1513468682 18000
> > #      Sat Dec 16 18:58:02 2017 -0500
> > # Node ID ca9cde0697c1e923f4c9a3396343afc238ba92f2
> > # Parent  d1fbb07b0b7259ae4ca88434e0eda2a21359f7ec
> > test-ssh: convert dumpenv to python for Windows
> >
> > Previously, this complained:
> >
> >    remote: '.' is not recognized as an internal or external command,
> >    remote: operable program or batch file.
> >
> > 1 out of 4 Linux runs failed to print 17 after converting, and instead
> printed
> > an empty remote line.  Not sure why, but maybe the flush will help.
> >
> > diff --git a/tests/test-ssh.t b/tests/test-ssh.t
> > --- a/tests/test-ssh.t
> > +++ b/tests/test-ssh.t
> > @@ -598,17 +598,19 @@
> >   [255]
> >
> > test that custom environment is passed down to ssh executable
> > -  $ cat >>dumpenv <<EOF
> > -  > #! /bin/sh
> > -  > echo \$VAR >&2
> > +  $ cat >>dumpenv.py <<EOF
> > +  > from __future__ import print_function
> > +  > import sys
> > +  > from mercurial import encoding
> > +  > print('%s' % encoding.environ.get('VAR', ''), file=sys.stderr)
> > +  > sys.stderr.flush()
> >> EOF
> > -  $ chmod +x dumpenv
> > -  $ hg pull ssh://something --config ui.ssh="./dumpenv"
> > +  $ hg pull ssh://something --config ui.ssh="$PYTHON dumpenv.py"
> >   pulling from ssh://something/
> >   remote:
> >   abort: no suitable response from remote hg!
> >   [255]
> > -  $ hg pull ssh://something --config ui.ssh="./dumpenv" --config
> sshenv.VAR=17
> > +  $ hg pull ssh://something --config ui.ssh="$PYTHON dumpenv.py"
> --config sshenv.VAR=17
> >   pulling from ssh://something/
> >   remote: 17
>
> No idea why, but if I run this in my Linux VM with -j9, it wants to append
> an extra “remote: “ at this point.  If run without workers, it appears
> stable.
>

The flush() shouldn't be necessary because stdio descriptors will get
flushed when process exits. The new Python appears the same as the old
shell script.

I suspect we are encountering a preexisting race condition. I suspect the
Python version is more susceptible to it because of Python process startup
overhead. Before, the shell script likely ran in ~1ms. Now, it takes >10ms.
That may be enough to perturb the race condition.


> >   abort: no suitable response from remote hg!
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Matt Harbison - Dec. 17, 2017, 7:42 p.m.
> On Dec 17, 2017, at 2:07 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> 
>> On Sun, Dec 17, 2017 at 10:44 AM, Matt Harbison <mharbison72@gmail.com> wrote:
>> 
>> > On Dec 16, 2017, at 7:27 PM, Matt Harbison <mharbison72@gmail.com> wrote:
>> >
>> > # HG changeset patch
>> > # User Matt Harbison <matt_harbison@yahoo.com>
>> > # Date 1513468682 18000
>> > #      Sat Dec 16 18:58:02 2017 -0500
>> > # Node ID ca9cde0697c1e923f4c9a3396343afc238ba92f2
>> > # Parent  d1fbb07b0b7259ae4ca88434e0eda2a21359f7ec
>> > test-ssh: convert dumpenv to python for Windows
>> >
>> > Previously, this complained:
>> >
>> >    remote: '.' is not recognized as an internal or external command,
>> >    remote: operable program or batch file.
>> >
>> > 1 out of 4 Linux runs failed to print 17 after converting, and instead printed
>> > an empty remote line.  Not sure why, but maybe the flush will help.
>> >
>> > diff --git a/tests/test-ssh.t b/tests/test-ssh.t
>> > --- a/tests/test-ssh.t
>> > +++ b/tests/test-ssh.t
>> > @@ -598,17 +598,19 @@
>> >   [255]
>> >
>> > test that custom environment is passed down to ssh executable
>> > -  $ cat >>dumpenv <<EOF
>> > -  > #! /bin/sh
>> > -  > echo \$VAR >&2
>> > +  $ cat >>dumpenv.py <<EOF
>> > +  > from __future__ import print_function
>> > +  > import sys
>> > +  > from mercurial import encoding
>> > +  > print('%s' % encoding.environ.get('VAR', ''), file=sys.stderr)
>> > +  > sys.stderr.flush()
>> >> EOF
>> > -  $ chmod +x dumpenv
>> > -  $ hg pull ssh://something --config ui.ssh="./dumpenv"
>> > +  $ hg pull ssh://something --config ui.ssh="$PYTHON dumpenv.py"
>> >   pulling from ssh://something/
>> >   remote:
>> >   abort: no suitable response from remote hg!
>> >   [255]
>> > -  $ hg pull ssh://something --config ui.ssh="./dumpenv" --config sshenv.VAR=17
>> > +  $ hg pull ssh://something --config ui.ssh="$PYTHON dumpenv.py" --config sshenv.VAR=17
>> >   pulling from ssh://something/
>> >   remote: 17
>> 
>> No idea why, but if I run this in my Linux VM with -j9, it wants to append an extra “remote: “ at this point.  If run without workers, it appears stable.
> 
> The flush() shouldn't be necessary because stdio descriptors will get flushed when process exits. The new Python appears the same as the old shell script.

That was my understanding of things too, but before I added the flush, the instability seemed to be to omit the “remote: 17” line (but add the empty remote line).

> I suspect we are encountering a preexisting race condition. I suspect the Python version is more susceptible to it because of Python process startup overhead. Before, the shell script likely ran in ~1ms. Now, it takes >10ms. That may be enough to perturb the race condition.
>  
>> >   abort: no suitable response from remote hg!
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Matt Harbison - Dec. 21, 2017, 9:49 p.m.
> On Dec 17, 2017, at 2:42 PM, Matt Harbison <mharbison72@gmail.com> wrote:
> 
> 
>> On Dec 17, 2017, at 2:07 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
>> 
>>> On Sun, Dec 17, 2017 at 10:44 AM, Matt Harbison <mharbison72@gmail.com> wrote:
>>> 
>>> > On Dec 16, 2017, at 7:27 PM, Matt Harbison <mharbison72@gmail.com> wrote:
>>> >
>>> > # HG changeset patch
>>> > # User Matt Harbison <matt_harbison@yahoo.com>
>>> > # Date 1513468682 18000
>>> > #      Sat Dec 16 18:58:02 2017 -0500
>>> > # Node ID ca9cde0697c1e923f4c9a3396343afc238ba92f2
>>> > # Parent  d1fbb07b0b7259ae4ca88434e0eda2a21359f7ec
>>> > test-ssh: convert dumpenv to python for Windows
>>> >
>>> > Previously, this complained:
>>> >
>>> >    remote: '.' is not recognized as an internal or external command,
>>> >    remote: operable program or batch file.
>>> >
>>> > 1 out of 4 Linux runs failed to print 17 after converting, and instead printed
>>> > an empty remote line.  Not sure why, but maybe the flush will help.
>>> >
>>> > diff --git a/tests/test-ssh.t b/tests/test-ssh.t
>>> > --- a/tests/test-ssh.t
>>> > +++ b/tests/test-ssh.t
>>> > @@ -598,17 +598,19 @@
>>> >   [255]
>>> >
>>> > test that custom environment is passed down to ssh executable
>>> > -  $ cat >>dumpenv <<EOF
>>> > -  > #! /bin/sh
>>> > -  > echo \$VAR >&2
>>> > +  $ cat >>dumpenv.py <<EOF
>>> > +  > from __future__ import print_function
>>> > +  > import sys
>>> > +  > from mercurial import encoding
>>> > +  > print('%s' % encoding.environ.get('VAR', ''), file=sys.stderr)
>>> > +  > sys.stderr.flush()
>>> >> EOF
>>> > -  $ chmod +x dumpenv
>>> > -  $ hg pull ssh://something --config ui.ssh="./dumpenv"
>>> > +  $ hg pull ssh://something --config ui.ssh="$PYTHON dumpenv.py"
>>> >   pulling from ssh://something/
>>> >   remote:
>>> >   abort: no suitable response from remote hg!
>>> >   [255]
>>> > -  $ hg pull ssh://something --config ui.ssh="./dumpenv" --config sshenv.VAR=17
>>> > +  $ hg pull ssh://something --config ui.ssh="$PYTHON dumpenv.py" --config sshenv.VAR=17
>>> >   pulling from ssh://something/
>>> >   remote: 17
>>> 
>>> No idea why, but if I run this in my Linux VM with -j9, it wants to append an extra “remote: “ at this point.  If run without workers, it appears stable.
>> 
>> The flush() shouldn't be necessary because stdio descriptors will get flushed when process exits. The new Python appears the same as the old shell script.
> 
> That was my understanding of things too, but before I added the flush, the instability seemed to be to omit the “remote: 17” line (but add the empty remote line).
> 
>> I suspect we are encountering a preexisting race condition. I suspect the Python version is more susceptible to it because of Python process startup overhead. Before, the shell script likely ran in ~1ms. Now, it takes >10ms. That may be enough to perturb the race condition.

An additional data point: I ran without workers, with —loop, and it failed the same way in 3 out of 444 runs on the same Linux VM.

>>> >   abort: no suitable response from remote hg!
>>> _______________________________________________
>>> Mercurial-devel mailing list
>>> Mercurial-devel@mercurial-scm.org
>>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
Yuya Nishihara - Dec. 22, 2017, 2:04 p.m.
On Thu, 21 Dec 2017 16:49:07 -0500, Matt Harbison wrote:
> > On Dec 17, 2017, at 2:42 PM, Matt Harbison <mharbison72@gmail.com> wrote:
> >> On Dec 17, 2017, at 2:07 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> >>> On Sun, Dec 17, 2017 at 10:44 AM, Matt Harbison <mharbison72@gmail.com> wrote:
> >>> > test that custom environment is passed down to ssh executable
> >>> > -  $ cat >>dumpenv <<EOF
> >>> > -  > #! /bin/sh
> >>> > -  > echo \$VAR >&2
> >>> > +  $ cat >>dumpenv.py <<EOF
> >>> > +  > from __future__ import print_function
> >>> > +  > import sys
> >>> > +  > from mercurial import encoding
> >>> > +  > print('%s' % encoding.environ.get('VAR', ''), file=sys.stderr)
> >>> > +  > sys.stderr.flush()
> >>> >> EOF
> >>> > -  $ chmod +x dumpenv
> >>> > -  $ hg pull ssh://something --config ui.ssh="./dumpenv"
> >>> > +  $ hg pull ssh://something --config ui.ssh="$PYTHON dumpenv.py"

Perhaps the simplest workaround is setting ui.ssh="sh dumpenv"?

> >> I suspect we are encountering a preexisting race condition. I suspect the Python version is more susceptible to it because of Python process startup overhead. Before, the shell script likely ran in ~1ms. Now, it takes >10ms. That may be enough to perturb the race condition.
> 
> An additional data point: I ran without workers, with —loop, and it failed the same way in 3 out of 444 runs on the same Linux VM.

IIRC, the order of "remote:" messages was historically unstable as we do
select() channels asynchronously. I won't be surprised if there's another
issue.

Patch

diff --git a/tests/test-ssh.t b/tests/test-ssh.t
--- a/tests/test-ssh.t
+++ b/tests/test-ssh.t
@@ -598,17 +598,19 @@ 
   [255]
 
 test that custom environment is passed down to ssh executable
-  $ cat >>dumpenv <<EOF
-  > #! /bin/sh
-  > echo \$VAR >&2
+  $ cat >>dumpenv.py <<EOF
+  > from __future__ import print_function
+  > import sys
+  > from mercurial import encoding
+  > print('%s' % encoding.environ.get('VAR', ''), file=sys.stderr)
+  > sys.stderr.flush()
   > EOF
-  $ chmod +x dumpenv
-  $ hg pull ssh://something --config ui.ssh="./dumpenv"
+  $ hg pull ssh://something --config ui.ssh="$PYTHON dumpenv.py"
   pulling from ssh://something/
   remote: 
   abort: no suitable response from remote hg!
   [255]
-  $ hg pull ssh://something --config ui.ssh="./dumpenv" --config sshenv.VAR=17
+  $ hg pull ssh://something --config ui.ssh="$PYTHON dumpenv.py" --config sshenv.VAR=17
   pulling from ssh://something/
   remote: 17
   abort: no suitable response from remote hg!