Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'make minitest' non-zero error code ignored #16160

Closed
p5pRT opened this issue Sep 20, 2017 · 10 comments
Closed

'make minitest' non-zero error code ignored #16160

p5pRT opened this issue Sep 20, 2017 · 10 comments
Labels
miniperl miniperl, minitest and similar 'make' targets

Comments

@p5pRT
Copy link

p5pRT commented Sep 20, 2017

Migrated from rt.perl.org#132139 (status was 'open')

Searchable as RT132139$

@p5pRT
Copy link
Author

p5pRT commented Sep 20, 2017

From @jkeenan

As documented in https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132138,
't/run/switches.t' fails when run under 'miniperl', probably because it
'require's module File​::Spec.

I stumbled upon this by accident today. Following up on a discussion on
IRC #p5p and in https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132092, when
testing the perl-5.27.4 tarball provided by John SJ Anderson on
FreeBSD-11.0, instead of saying, as I normally would​:

#####
$ regular_configure && make test_harness
#####

... (where 'regular_configure' is a shell script that, on FreeBSD,
builds a threaded perl very close to the 'system perl'), I said​:

#####
$ regular_configure && make minitest && make test_harness
#####

I guessed that if 'make minitest' were to FAIL, then 'make test_harness'
would never be run and my attention would immediately be drawn to
failing tests.

I sat watching the terminal and thought I saw a test failure during
'make minitest'. But it quickly became apparent that this failure --
subsequently confirmed as being in 't/run/switches.t' did not prevent
'make test_harness' from starting to run.

So I then cleaned the directory, re-configured and then said 'make
minitest'. 'make minitest' did find and report the error in
't/run/switches.t' but its error code is ignored! (See gzipped
attachment for complete output as reproduced on Linux.)

#####
...
t/run/switchd .................. ok
t/run/switches ................. Can't locate File/Spec.pm in @​INC (you
may need to install the File​::Spec module) (@​INC contains​: ../lib) at
run/switches.t line 424.
# Looks like you planned 137 tests but ran 111.
FAILED--expected 137 tests, saw 111
t/run/switchn .................. ok
...
t/perf/taint ................... skipped
Failed 1 test out of 343, 99.71% okay.
  run/switches.t
### Since not all tests were successful, you may want to run some of
### them individually and examine any diagnostic messages they produce.
### See the INSTALL document's section on "make test".
### You have a good chance to get more information by running
### ./perl harness
### in the 't' directory since most (>=80%) of the tests succeeded.
### You may have to set your dynamic library search path,
### LD_LIBRARY_PATH, to point to the build directory​:
### setenv LD_LIBRARY_PATH `pwd`; cd t; ./perl harness
### LD_LIBRARY_PATH=`pwd`; export LD_LIBRARY_PATH; cd t; ./perl harness
### export LD_LIBRARY_PATH=`pwd`; cd t; ./perl harness
### for csh-style shells, like tcsh; or for traditional/modern
### Bourne-style shells, like bash, ksh, and zsh, respectively.
Elapsed​: 124 sec
u=1.69 s=0.24 cu=39.93 cs=1.73 scripts=343 tests=348120
makefile​:824​: recipe for target 'minitest' failed
make​: [minitest] Error 1 (ignored)
#####

Ignoring this error code, IMO, diminishes the value of 'make minitest'
to an author or committer trying to evaluate the correctness of a patch.

Is there a rationale for ignoring this error code?

If not, what is the recommended fix?

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Sep 20, 2017

@p5pRT
Copy link
Author

p5pRT commented Sep 20, 2017

From @jkeenan

Summary of my perl5 (revision 5 version 27 subversion 4) configuration​:
  Commit id​: e271727
  Platform​:
  osname=linux
  osvers=4.4.0-93-generic
  archname=x86_64-linux
  uname='linux zareason 4.4.0-93-generic #116-ubuntu smp fri aug 11 21​:17​:51 utc 2017 x86_64 x86_64 x86_64 gnulinux '
  config_args='-des -Dusedevel'
  hint=recommended
  useposix=true
  d_sigaction=define
  useithreads=undef
  usemultiplicity=undef
  use64bitint=define
  use64bitall=define
  uselongdouble=undef
  usemymalloc=n
  default_inc_excludes_dot=define
  bincompat5005=undef
  Compiler​:
  cc='cc'
  ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
  optimize='-O2'
  cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
  ccversion=''
  gccversion='5.4.0 20160609'
  gccosandvers=''
  intsize=4
  longsize=8
  ptrsize=8
  doublesize=8
  byteorder=12345678
  doublekind=3
  d_longlong=define
  longlongsize=8
  d_longdbl=define
  longdblsize=16
  longdblkind=3
  ivtype='long'
  ivsize=8
  nvtype='double'
  nvsize=8
  Off_t='off_t'
  lseeksize=8
  alignbytes=8
  prototype=define
  Linker and Libraries​:
  ld='cc'
  ldflags =' -fstack-protector-strong -L/usr/local/lib'
  libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/5/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /lib64 /usr/lib64
  libs=-lpthread -lnsl -ldb -ldl -lm -lcrypt -lutil -lc
  perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  libc=libc-2.23.so
  so=so
  useshrplib=false
  libperl=libperl.a
  gnulibc_version='2.23'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs
  dlext=so
  d_dlsymun=undef
  ccdlflags='-Wl,-E'
  cccdlflags='-fPIC'
  lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector-strong'

Characteristics of this binary (from libperl)​:
  Compile-time options​:
  HAS_TIMES
  PERLIO_LAYERS
  PERL_COPY_ON_WRITE
  PERL_DONT_CREATE_GVSV
  PERL_MALLOC_WRAP
  PERL_OP_PARENT
  PERL_PRESERVE_IVUV
  PERL_USE_DEVEL
  USE_64_BIT_ALL
  USE_64_BIT_INT
  USE_LARGE_FILES
  USE_LOCALE
  USE_LOCALE_COLLATE
  USE_LOCALE_CTYPE
  USE_LOCALE_NUMERIC
  USE_LOCALE_TIME
  USE_PERLIO
  USE_PERL_ATOF
  Built under linux
  Compiled at Sep 16 2017 10​:17​:33
  %ENV​:
  PERLBREW_BASHRC_VERSION="0.78"
  PERLBREW_HOME="/home/jkeenan/.perlbrew"
  PERLBREW_MANPATH="/home/jkeenan/perl5/perlbrew/perls/perl-5.26.0/man"
  PERLBREW_PATH="/home/jkeenan/perl5/perlbrew/bin​:/home/jkeenan/perl5/perlbrew/perls/perl-5.26.0/bin"
  PERLBREW_PERL="perl-5.26.0"
  PERLBREW_ROOT="/home/jkeenan/perl5/perlbrew"
  PERLBREW_VERSION="0.78"
  PERL_WORKDIR="/home/jkeenan/gitwork/perl"
  @​INC​:
  lib
  /usr/local/lib/perl5/site_perl/5.27.4/x86_64-linux
  /usr/local/lib/perl5/site_perl/5.27.4
  /usr/local/lib/perl5/5.27.4/x86_64-linux
  /usr/local/lib/perl5/5.27.4

@p5pRT
Copy link
Author

p5pRT commented Sep 21, 2017

From @tonycoz

On Wed, 20 Sep 2017 16​:33​:32 -0700, jkeenan@​pobox.com wrote​:

As documented in https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132138,
't/run/switches.t' fails when run under 'miniperl', probably because it
'require's module File​::Spec.

I stumbled upon this by accident today. Following up on a discussion on
IRC #p5p and in https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132092, when
testing the perl-5.27.4 tarball provided by John SJ Anderson on
FreeBSD-11.0, instead of saying, as I normally would​:

#####
$ regular_configure && make test_harness
#####

... (where 'regular_configure' is a shell script that, on FreeBSD,
builds a threaded perl very close to the 'system perl'), I said​:

#####
$ regular_configure && make minitest && make test_harness
#####

I guessed that if 'make minitest' were to FAIL, then 'make test_harness'
would never be run and my attention would immediately be drawn to
failing tests.

I sat watching the terminal and thought I saw a test failure during
'make minitest'. But it quickly became apparent that this failure --
subsequently confirmed as being in 't/run/switches.t' did not prevent
'make test_harness' from starting to run.

So I then cleaned the directory, re-configured and then said 'make
minitest'. 'make minitest' did find and report the error in
't/run/switches.t' but its error code is ignored! (See gzipped
attachment for complete output as reproduced on Linux.)

minitest has ignored errors since it was introduced in 16d20bd​:

commit 16d20bd
Author​: Andy Dougherty <doughera@​lafcol.lafayette.edu>
Date​: Tue May 30 22​:59​:41 1995 +0000

  This is my patch patch.1i for perl5.001.
 
...
  Makefile.SH
...
  Add 'minitest' target.
 
...

Inline Patch
diff --git a/Makefile.SH b/Makefile.SH
index f5dff60..cdd6333 100644
--- a/Makefile.SH
+++ b/Makefile.SH
... @​@​ \-374\,6 \+395\,11 @​@​ test​: miniperl perl preplibrary $\(dynamic\_ext\)   \- cd t && chmod \+x TEST \*/\*\.t   \- cd t && \(rm \-f perl; $\(LNS\) \.\./perl perl\) && \./perl TEST \+minitest​: miniperl
+ - cd t && chmod +x TEST */*.t
+ - cd t && (rm -f perl; $(LNS) ../miniperl perl) \
+ && ./perl TEST base/*.t comp/*.t cmd/*.t io/*.t op/*.t </dev/tty
+
clist​: $(c)
  echo $(c) | tr ' ' '\012' >.clist

Tony

@p5pRT
Copy link
Author

p5pRT commented Sep 21, 2017

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Sep 21, 2017

From @jkeenan

On Thu, 21 Sep 2017 00​:47​:45 GMT, tonyc wrote​:

On Wed, 20 Sep 2017 16​:33​:32 -0700, jkeenan@​pobox.com wrote​:

As documented in https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132138,
't/run/switches.t' fails when run under 'miniperl', probably because
it
'require's module File​::Spec.

I stumbled upon this by accident today. Following up on a discussion
on
IRC #p5p and in https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132092,
when
testing the perl-5.27.4 tarball provided by John SJ Anderson on
FreeBSD-11.0, instead of saying, as I normally would​:

#####
$ regular_configure && make test_harness
#####

... (where 'regular_configure' is a shell script that, on FreeBSD,
builds a threaded perl very close to the 'system perl'), I said​:

#####
$ regular_configure && make minitest && make test_harness
#####

I guessed that if 'make minitest' were to FAIL, then 'make
test_harness'
would never be run and my attention would immediately be drawn to
failing tests.

I sat watching the terminal and thought I saw a test failure during
'make minitest'. But it quickly became apparent that this failure --
subsequently confirmed as being in 't/run/switches.t' did not prevent
'make test_harness' from starting to run.

So I then cleaned the directory, re-configured and then said 'make
minitest'. 'make minitest' did find and report the error in
't/run/switches.t' but its error code is ignored! (See gzipped
attachment for complete output as reproduced on Linux.)

minitest has ignored errors since it was introduced in
16d20bd​:

commit 16d20bd
Author​: Andy Dougherty <doughera@​lafcol.lafayette.edu>
Date​: Tue May 30 22​:59​:41 1995 +0000

This is my patch patch.1i for perl5.001.

...
Makefile.SH
...
Add 'minitest' target.

...
diff --git a/Makefile.SH b/Makefile.SH
index f5dff60..cdd6333 100644
--- a/Makefile.SH
+++ b/Makefile.SH
...
@​@​ -374,6 +395,11 @​@​ test​: miniperl perl preplibrary $(dynamic_ext)
- cd t && chmod +x TEST */*.t
- cd t && (rm -f perl; $(LNS) ../perl perl) && ./perl TEST
</dev/tty

+minitest​: miniperl
+ - cd t && chmod +x TEST */*.t
+ - cd t && (rm -f perl; $(LNS) ../miniperl perl) \
+ && ./perl TEST base/*.t comp/*.t cmd/*.t io/*.t op/*.t
</dev/tty
+
clist​: $(c)
echo $(c) | tr ' ' '\012' >.clist

Tony

So, on the one hand, I would argue, it does not DWIM for the purpose for which it is recommended in pod/perlgit.pod and by commenters on list today.

On the other hand, to change that situation would require changing the way it has worked for 22 years (and I'm always inclined to trust Andy D's judgment).

I'm inclined to the first hand. I want to use it in a string of commands and I want that string to fail if and when 'make minitest' fails. Otherwise I'm inclined to never use it.

Thank you very much.
--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Sep 21, 2017

From @doughera88

On Wed, Sep 20, 2017 at 06​:54​:20PM -0700, James E Keenan via RT wrote​:

On Thu, 21 Sep 2017 00​:47​:45 GMT, tonyc wrote​:

minitest has ignored errors since it was introduced in
16d20bd​:

commit 16d20bd
Author​: Andy Dougherty <doughera@​lafcol.lafayette.edu>
Date​: Tue May 30 22​:59​:41 1995 +0000

This is my patch patch.1i for perl5.001.

...
Makefile.SH
...
Add 'minitest' target.

...
diff --git a/Makefile.SH b/Makefile.SH
index f5dff60..cdd6333 100644
--- a/Makefile.SH
+++ b/Makefile.SH
...
@​@​ -374,6 +395,11 @​@​ test​: miniperl perl preplibrary $(dynamic_ext)
- cd t && chmod +x TEST */*.t
- cd t && (rm -f perl; $(LNS) ../perl perl) && ./perl TEST
</dev/tty

+minitest​: miniperl
+ - cd t && chmod +x TEST */*.t
+ - cd t && (rm -f perl; $(LNS) ../miniperl perl) \
+ && ./perl TEST base/*.t comp/*.t cmd/*.t io/*.t op/*.t
</dev/tty
+
clist​: $(c)
echo $(c) | tr ' ' '\012' >.clist

Tony

So, on the one hand, I would argue, it does not DWIM for the purpose for which it is recommended in pod/perlgit.pod and by commenters on list today.

On the other hand, to change that situation would require changing the way it has worked for 22 years (and I'm always inclined to trust Andy D's judgment).

I'm inclined to the first hand. I want to use it in a string of commands and I want that string to fail if and when 'make minitest' fails. Otherwise I'm inclined to never use it.

While I appreciate the thought, I don't recall exercising any judgment
there. The 'minitest' target was pretty much a cut & paste of the 'test'
target, which ignored the exit status. I don't recall why. Your inclination
sounds sensible to me.

--
  Andy Dougherty doughera@​lafayette.edu

@p5pRT
Copy link
Author

p5pRT commented Sep 22, 2017

From @jkeenan

On Thu, 21 Sep 2017 02​:25​:47 GMT, doughera wrote​:

On Wed, Sep 20, 2017 at 06​:54​:20PM -0700, James E Keenan via RT wrote​:

On Thu, 21 Sep 2017 00​:47​:45 GMT, tonyc wrote​:

minitest has ignored errors since it was introduced in
16d20bd​:

commit 16d20bd
Author​: Andy Dougherty <doughera@​lafcol.lafayette.edu>
Date​: Tue May 30 22​:59​:41 1995 +0000

This is my patch patch.1i for perl5.001.

...
Makefile.SH
...
Add 'minitest' target.

...
diff --git a/Makefile.SH b/Makefile.SH
index f5dff60..cdd6333 100644
--- a/Makefile.SH
+++ b/Makefile.SH
...
@​@​ -374,6 +395,11 @​@​ test​: miniperl perl preplibrary $(dynamic_ext)
- cd t && chmod +x TEST */*.t
- cd t && (rm -f perl; $(LNS) ../perl perl) && ./perl TEST
</dev/tty

+minitest​: miniperl
+ - cd t && chmod +x TEST */*.t
+ - cd t && (rm -f perl; $(LNS) ../miniperl perl) \
+ && ./perl TEST base/*.t comp/*.t cmd/*.t io/*.t
op/*.t
</dev/tty
+
clist​: $(c)
echo $(c) | tr ' ' '\012' >.clist

Tony

So, on the one hand, I would argue, it does not DWIM for the purpose
for which it is recommended in pod/perlgit.pod and by commenters on
list today.

On the other hand, to change that situation would require changing
the way it has worked for 22 years (and I'm always inclined to trust
Andy D's judgment).

I'm inclined to the first hand. I want to use it in a string of
commands and I want that string to fail if and when 'make minitest'
fails. Otherwise I'm inclined to never use it.

While I appreciate the thought, I don't recall exercising any judgment
there. The 'minitest' target was pretty much a cut & paste of the
'test'
target, which ignored the exit status. I don't recall why. Your
inclination
sounds sensible to me.

It turns out that having 'make' take the exit status of 'make minitest' into consideration is very simple; see patch attached.

However, I want to re-read all references to 'minitest' in the core distro and to allow time for other people to comment.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Sep 22, 2017

From @jkeenan

132139-minitest.diff
diff --git a/Makefile.SH b/Makefile.SH
index 730dcca..b0f0d9d 100755
--- a/Makefile.SH
+++ b/Makefile.SH
@@ -1631,15 +1631,15 @@ minitest_prep:
 	@echo "You may see some irrelevant test failures if you have been unable"
 	@echo "to build lib/Config.pm, or the Unicode data files."
 	@echo " "
-	- cd t && (rm -f $(PERL_EXE); $(LNS) ../$(MINIPERL_EXE) $(PERL_EXE))
+	cd t && (rm -f $(PERL_EXE); $(LNS) ../$(MINIPERL_EXE) $(PERL_EXE))
 
 MINITEST_TESTS = base/*.t comp/*.t cmd/*.t run/*.t io/*.t re/*.t opbasic/*.t op/*.t uni/*.t perf/*.t
 
 minitest: $(MINIPERL_EXE) minitest_prep
-	- cd t && $(RUN_PERL) TEST $(MINITEST_TESTS) <$(devtty)
+	cd t && $(RUN_PERL) TEST $(MINITEST_TESTS) <$(devtty)
 
 minitest-notty minitest_notty: $(MINIPERL_EXE) minitest_prep
-	- cd t && PERL_SKIP_TTY_TEST=1 $(RUN_PERL) TEST $(MINITEST_TESTS)
+	cd t && PERL_SKIP_TTY_TEST=1 $(RUN_PERL) TEST $(MINITEST_TESTS)
 
 # Test via harness
 

jkeenan added a commit that referenced this issue Jan 31, 2020
In pipelines like:

    $ sh ./Configure -des -Dusedevel && \
        make minitest && make test_harness

... we don't want to run 'make test_harness' unless 'make minitest' has
succeeded.

To test:

    $ echo "exit(1);" >> t/base/cond.t
    $ make minitest && echo "hello world"
    # [note that minitest fails quickly and 'echo' is not reached]

    $ git checkout -- t/base/cond.t
    $ make minitest && echo "hello world"
    # [note that minitest runs and 'echo' is reached]

For: #16160
Originally: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132139
jkeenan added a commit that referenced this issue Feb 4, 2020
In pipelines like:

    $ sh ./Configure -des -Dusedevel && \
        make minitest && make test_harness

... we don't want to run 'make test_harness' unless 'make minitest' has
succeeded.

To test:

    $ echo "exit(1);" >> t/base/cond.t
    $ make minitest && echo "hello world"
    # [note that minitest fails quickly and 'echo' is not reached]

    $ git checkout -- t/base/cond.t
    $ make minitest && echo "hello world"
    # [note that minitest runs and 'echo' is reached]

For: #16160
Originally: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132139
@xenu xenu removed the Severity Low label Dec 29, 2021
@jkeenan jkeenan added the miniperl miniperl, minitest and similar 'make' targets label Jun 9, 2022
@jkeenan
Copy link
Contributor

jkeenan commented Jun 9, 2022

This ticket should have been closed in February 2020 when the following commit was applied:

commit 2b689a479128e6b86e3d180f70dcf1b447caf0e6
Author:     James E Keenan <[email protected]>
AuthorDate: Fri Jan 31 03:23:18 2020 +0000
Commit:     James E Keenan <[email protected]>
CommitDate: Tue Feb 4 09:27:50 2020 -0500

    Do not ignore non-zero error code from 'make minitest'

Closing now.

@jkeenan jkeenan closed this as completed Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
miniperl miniperl, minitest and similar 'make' targets
Projects
None yet
Development

No branches or pull requests

3 participants