Hello! On Fri, Sep 16, 2022 at 09:03:38PM +0400, Sergey Kandaurov wrote:
> > On 7 Sep 2022, at 05:46, Maxim Dounin <mdou...@mdounin.ru> wrote: > > > > Hello! > > > > On Tue, Sep 06, 2022 at 05:52:51PM +0400, Sergey Kandaurov wrote: > > > >> # HG changeset patch > >> # User Sergey Kandaurov <pluk...@nginx.com> > >> # Date 1662472340 -14400 > >> # Tue Sep 06 17:52:20 2022 +0400 > >> # Node ID 7eb87eaf06f4b9161cfe298f195947dbddedd01d > >> # Parent 78fe648d54a79822a72f3a5f8cc79651b3b1f5a7 > >> Tests: fixed try_run() to remove temporary directory on failure. > >> > >> Keeping the file handle pointing to the "stderr" file prevented removal > >> of the temporary directory on win32 if execution aborted in run(). > >> The fix is to move safe file operations surrounding run() out of the > >> eval block such that the state of file handles is consistent. > >> Fixes a1874249496d. > > > > I believe it might need some additional emphasis that this is > > about Windows only. For example: > > > > Tests: fixed try_run() to remove temporary directory on win32. > > > > Agree, thx. > > >> > >> diff -r 78fe648d54a7 -r 7eb87eaf06f4 lib/Test/Nginx.pm > >> --- a/lib/Test/Nginx.pm Tue Aug 30 17:24:16 2022 +0400 > >> +++ b/lib/Test/Nginx.pm Tue Sep 06 17:52:20 2022 +0400 > >> @@ -290,8 +290,9 @@ sub has_daemon($) { > >> sub try_run($$) { > >> my ($self, $message) = @_; > >> > >> + open OLDERR, ">&", \*STDERR; > >> + > >> eval { > >> - open OLDERR, ">&", \*STDERR; > >> open NEWERR, ">", $self->{_testdir} . '/stderr' > >> or die "Can't open stderr: $!"; > >> close STDERR; > >> @@ -299,10 +300,10 @@ sub try_run($$) { > >> close NEWERR; > >> > >> $self->run(); > >> + }; > >> > >> - close STDERR; > >> - open STDERR, ">&", \*OLDERR; > >> - }; > >> + close STDERR; > >> + open STDERR, ">&", \*OLDERR; > >> > >> return $self unless $@; > > > > Wouldn't it be better to move all stderr handling out of the eval? > > > > diff --git a/lib/Test/Nginx.pm b/lib/Test/Nginx.pm > > --- a/lib/Test/Nginx.pm > > +++ b/lib/Test/Nginx.pm > > @@ -291,14 +291,13 @@ sub try_run($$) { > > my ($self, $message) = @_; > > > > open OLDERR, ">&", \*STDERR; > > + open NEWERR, ">", $self->{_testdir} . '/stderr' > > + or die "Can't open stderr: $!"; > > + close STDERR; > > + open STDERR, ">&", \*NEWERR; > > + close NEWERR; > > > > eval { > > - open NEWERR, ">", $self->{_testdir} . '/stderr' > > - or die "Can't open stderr: $!"; > > - close STDERR; > > - open STDERR, ">&", \*NEWERR; > > - close NEWERR; > > - > > $self->run(); > > }; > > > > The reason was to continue hide '/stderr' open errors in eval, > but I tend to think that it's rather better to die explicitly, > as that's not something to happen. > > > > > Alternatively, it might be the right time to backout a1874249496d > > completely and rely on "-e ..." instead, given that it was > > introduced in 1.19.5 and already present in all supported > > versions. > > I thought about that, looks like the time has come to cleanup things. > It doesn't break out nicely into two pieces, so comes in one change. > > # HG changeset patch > # User Sergey Kandaurov <pluk...@nginx.com> > # Date 1663347154 -14400 > # Fri Sep 16 20:52:34 2022 +0400 > # Node ID 16c3e0b1c8878bb127425b4da873f8eba604c879 > # Parent 78fe648d54a79822a72f3a5f8cc79651b3b1f5a7 > Tests: updated try_run() to rely on nginx "-e". > > The "-e" command line option introduced in nginx 1.19.5 is now used > to print error line on startup failures with TEST_NGINX_VERBOSE set. > This change replaces a previous approach (a1874249496d) compatible > with pre-1.19.5 nginx versions that used to redirect stderr to file. > Hence, "-e" compatibility is removed. > > As a side effect, this fixes temporary directory removal on win32 > left on startup failures because the "stderr" file was kept open. > > diff --git a/lib/Test/Nginx.pm b/lib/Test/Nginx.pm > --- a/lib/Test/Nginx.pm > +++ b/lib/Test/Nginx.pm > @@ -48,8 +48,6 @@ sub new { > ) > or die "Can't create temp directory: $!\n"; > $self->{_testdir} =~ s!\\!/!g if $^O eq 'MSWin32'; > - mkdir "$self->{_testdir}/logs" > - or die "Can't create logs directory: $!\n"; > > Test::More::BAIL_OUT("no $NGINX binary found") > unless -x $NGINX; > @@ -291,24 +289,16 @@ sub try_run($$) { > my ($self, $message) = @_; > > eval { > - open OLDERR, ">&", \*STDERR; > - open NEWERR, ">", $self->{_testdir} . '/stderr' > - or die "Can't open stderr: $!"; > - close STDERR; > - open STDERR, ">&", \*NEWERR; > - close NEWERR; > - > + open OLDERR, ">&", \*STDERR; close STDERR; > $self->run(); > - > - close STDERR; > open STDERR, ">&", \*OLDERR; > }; > > return $self unless $@; > > if ($ENV{TEST_NGINX_VERBOSE}) { > - open F, '<', $self->{_testdir} . '/stderr' > - or die "Can't open stderr: $!"; > + open F, '<', $self->{_testdir} . '/error.log' > + or die "Can't open error.log: $!"; > log_core($_) while (<F>); > close F; > } > @@ -350,10 +340,8 @@ sub run(;$) { > my @globals = $self->{_test_globals} ? > () : ('-g', "pid $testdir/nginx.pid; " > . "error_log $testdir/error.log debug;"); > - my @error = $self->has_version('1.19.5') ? > - ('-e', 'error.log') : (); > exec($NGINX, '-p', "$testdir/", '-c', 'nginx.conf', > - @error, @globals) > + '-e', 'error.log', @globals) > or die "Unable to exec(): $!\n"; > } > > @@ -425,10 +413,8 @@ sub dump_config() { > my @globals = $self->{_test_globals} ? > () : ('-g', "pid $testdir/nginx.pid; " > . "error_log $testdir/error.log debug;"); > - my @error = $self->has_version('1.19.5') ? > - ('-e', 'error.log') : (); > my $command = "$NGINX -T -p $testdir/ -c nginx.conf " > - . join(' ', @error, @globals); > + . join(' ', '-e', 'error.log', @globals); This should be changed to explicitly use "-e error.log", much like "-c nginx.conf" above. > > return qx/$command 2>&1/; > } > @@ -481,10 +467,8 @@ sub reload() { > my @globals = $self->{_test_globals} ? > () : ('-g', "pid $testdir/nginx.pid; " > . "error_log $testdir/error.log debug;"); > - my @error = $self->has_version('1.19.5') ? > - ('-e', 'error.log') : (); > system($NGINX, '-p', $testdir, '-c', "nginx.conf", > - '-s', 'reload', @error, @globals) == 0 > + '-s', 'reload', '-e', 'error.log', @globals) == 0 > or die "system() failed: $?\n"; > > } else { > @@ -506,10 +490,8 @@ sub stop() { > my @globals = $self->{_test_globals} ? > () : ('-g', "pid $testdir/nginx.pid; " > . "error_log $testdir/error.log debug;"); > - my @error = $self->has_version('1.19.5') ? > - ('-e', 'error.log') : (); > system($NGINX, '-p', $testdir, '-c', "nginx.conf", > - '-s', 'quit', @error, @globals) == 0 > + '-s', 'quit', '-e', 'error.log', @globals) == 0 > or die "system() failed: $?\n"; > > } else { > @@ -530,10 +512,8 @@ sub stop() { > my @globals = $self->{_test_globals} ? > () : ('-g', "pid $testdir/nginx.pid; " > . "error_log $testdir/error.log debug;"); > - my @error = $self->has_version('1.19.5') ? > - ('-e', 'error.log') : (); > system($NGINX, '-p', $testdir, '-c', "nginx.conf", > - '-s', 'stop', @error, @globals) == 0 > + '-s', 'stop', '-e', 'error.log', @globals) == 0 > or die "system() failed: $?\n"; > > } else { Otherwise looks good. -- Maxim Dounin http://mdounin.ru/ _______________________________________________ nginx-devel mailing list -- nginx-devel@nginx.org To unsubscribe send an email to nginx-devel-le...@nginx.org