From 6c05684f323cc01fccaaa62f1d492f60ae2df8e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20G=C3=B3rny?= Date: Sun, 18 Dec 2022 07:27:37 +0100 Subject: [PATCH 1/3] pythongh-100220: Fix error handling in make rules Use `set -e` before compound shell commands in order to ensure that make targets fail correctly when at least one of the subcommands fail. This is necessary since make considers a target failed only if one of the shell invocations returns with unsuccessful exit status. If a shell script does not exit explicitly, the shell uses the exit status of the *last* executed command. This means that when multiple commands are executed (e.g. through a `for` loop), the exit statuses of prior command invocations are ignored. This can be either resolved by adding an explicit `|| exit 1` to every command that is expected to succeed, or by running the whole script with `set -e`. The latter was used here as it the rules seem to be written with the assumption that individual commands were supposed to cause the make rules to fail. --- Makefile.pre.in | 58 +++++++++++++------ ...-12-18-07-24-44.gh-issue-100220.BgSV7C.rst | 2 + 2 files changed, 41 insertions(+), 19 deletions(-) create mode 100644 Misc/NEWS.d/next/Build/2022-12-18-07-24-44.gh-issue-100220.BgSV7C.rst diff --git a/Makefile.pre.in b/Makefile.pre.in index 7a84b953d97962..84c90b281e44bd 100644 --- a/Makefile.pre.in +++ b/Makefile.pre.in @@ -738,6 +738,7 @@ $(LIBRARY): $(LIBRARY_OBJS) $(AR) $(ARFLAGS) $@ $(LIBRARY_OBJS) libpython$(LDVERSION).so: $(LIBRARY_OBJS) $(DTRACE_OBJS) + set -e; \ if test $(INSTSONAME) != $(LDLIBRARY); then \ $(BLDSHARED) -Wl,-h$(INSTSONAME) -o $(INSTSONAME) $(LIBRARY_OBJS) $(MODLIBS) $(SHLIBS) $(LIBC) $(LIBM); \ $(LN) -f $(INSTSONAME) $@; \ @@ -894,7 +895,8 @@ $(LIBEXPAT_A): $(LIBEXPAT_OBJS) # pybuilddir.txt is created too late. We cannot use it in Makefile # targets. ln --relative is not portable. sharedmods: $(SHAREDMODS) pybuilddir.txt - @target=`cat pybuilddir.txt`; \ + @set -e; \ + target=`cat pybuilddir.txt`; \ $(MKDIR_P) $$target; \ for mod in X $(SHAREDMODS); do \ if test $$mod != X; then \ @@ -907,7 +909,8 @@ checksharedmods: sharedmods $(PYTHON_FOR_BUILD_DEPS) $(BUILDPYTHON) @$(RUNSHARED) $(PYTHON_FOR_BUILD) $(srcdir)/Tools/build/check_extension_modules.py rundsymutil: sharedmods $(PYTHON_FOR_BUILD_DEPS) $(BUILDPYTHON) - @if [ ! -z $(DSYMUTIL) ] ; then \ + @set -e; \ + if [ ! -z $(DSYMUTIL) ] ; then \ echo $(DSYMUTIL_PATH) $(BUILDPYTHON); \ $(DSYMUTIL_PATH) $(BUILDPYTHON); \ if test -f $(LDLIBRARY); then \ @@ -1814,7 +1817,8 @@ commoninstall: check-clean-src @FRAMEWORKALTINSTALLFIRST@ \ DESTDIRS= $(exec_prefix) $(LIBDIR) $(BINLIBDEST) $(DESTSHARED) sharedinstall: all - @for i in $(DESTDIRS); \ + @set -e; \ + for i in $(DESTDIRS); \ do \ if test ! -d $(DESTDIR)$$i; then \ echo "Creating directory $$i"; \ @@ -1822,7 +1826,8 @@ sharedinstall: all else true; \ fi; \ done - @for i in X $(SHAREDMODS); do \ + @set -e; \ + for i in X $(SHAREDMODS); do \ if test $$i != X; then \ echo $(INSTALL_SHARED) $$i $(DESTSHARED)/`basename $$i`; \ $(INSTALL_SHARED) $$i $(DESTDIR)$(DESTSHARED)/`basename $$i`; \ @@ -1836,7 +1841,8 @@ sharedinstall: all # Install the interpreter with $(VERSION) affixed # This goes into $(exec_prefix) altbininstall: $(BUILDPYTHON) @FRAMEWORKPYTHONW@ - @for i in $(BINDIR) $(LIBDIR); \ + @set -e; \ + for i in $(BINDIR) $(LIBDIR); \ do \ if test ! -d $(DESTDIR)$$i; then \ echo "Creating directory $$i"; \ @@ -1855,7 +1861,8 @@ altbininstall: $(BUILDPYTHON) @FRAMEWORKPYTHONW@ fi; \ (cd $(DESTDIR)$(BINDIR); $(LN) python$(LDVERSION)$(EXE) python$(VERSION)$(EXE)); \ fi - @if test "$(PY_ENABLE_SHARED)" = 1 -o "$(STATIC_LIBPYTHON)" = 1; then \ + @set -e; \ + if test "$(PY_ENABLE_SHARED)" = 1 -o "$(STATIC_LIBPYTHON)" = 1; then \ if test -f $(LDLIBRARY) && test "$(PYTHONFRAMEWORKDIR)" = "no-framework" ; then \ if test -n "$(DLLLIBRARY)" ; then \ $(INSTALL_SHARED) $(DLLLIBRARY) $(DESTDIR)$(BINDIR); \ @@ -1942,7 +1949,8 @@ bininstall: altbininstall # Install the versioned manual page altmaninstall: - @for i in $(MANDIR) $(MANDIR)/man1; \ + @set -e; \ + for i in $(MANDIR) $(MANDIR)/man1; \ do \ if test ! -d $(DESTDIR)$$i; then \ echo "Creating directory $$i"; \ @@ -2077,7 +2085,8 @@ COMPILEALL_OPTS=-j0 TEST_MODULES=@TEST_MODULES@ libinstall: all $(srcdir)/Modules/xxmodule.c - @for i in $(SCRIPTDIR) $(LIBDEST); \ + @set -e; \ + for i in $(SCRIPTDIR) $(LIBDEST); \ do \ if test ! -d $(DESTDIR)$$i; then \ echo "Creating directory $$i"; \ @@ -2085,7 +2094,8 @@ libinstall: all $(srcdir)/Modules/xxmodule.c else true; \ fi; \ done - @if test "$(TEST_MODULES)" = yes; then \ + @set -e; \ + if test "$(TEST_MODULES)" = yes; then \ subdirs="$(LIBSUBDIRS) $(TESTSUBDIRS)"; \ else \ subdirs="$(LIBSUBDIRS)"; \ @@ -2101,7 +2111,8 @@ libinstall: all $(srcdir)/Modules/xxmodule.c else true; \ fi; \ done - @for i in $(srcdir)/Lib/*.py; \ + @set -e; \ + for i in $(srcdir)/Lib/*.py; \ do \ if test -x $$i; then \ $(INSTALL_SCRIPT) $$i $(DESTDIR)$(LIBDEST); \ @@ -2111,7 +2122,8 @@ libinstall: all $(srcdir)/Modules/xxmodule.c echo $(INSTALL_DATA) $$i $(LIBDEST); \ fi; \ done - @if test "$(TEST_MODULES)" = yes; then \ + @set -e; \ + if test "$(TEST_MODULES)" = yes; then \ subdirs="$(LIBSUBDIRS) $(TESTSUBDIRS)"; \ else \ subdirs="$(LIBSUBDIRS)"; \ @@ -2201,7 +2213,8 @@ scripts: $(SCRIPT_2TO3) $(SCRIPT_IDLE) $(SCRIPT_PYDOC) python-config # Install the include files INCLDIRSTOMAKE=$(INCLUDEDIR) $(CONFINCLUDEDIR) $(INCLUDEPY) $(CONFINCLUDEPY) inclinstall: - @for i in $(INCLDIRSTOMAKE); \ + @set -e; \ + for i in $(INCLDIRSTOMAKE); \ do \ if test ! -d $(DESTDIR)$$i; then \ echo "Creating directory $$i"; \ @@ -2219,17 +2232,20 @@ inclinstall: $(INSTALL) -d -m $(DIRMODE) $(DESTDIR)$(INCLUDEPY)/internal; \ else true; \ fi - @for i in $(srcdir)/Include/*.h; \ + @set -e; \ + for i in $(srcdir)/Include/*.h; \ do \ echo $(INSTALL_DATA) $$i $(INCLUDEPY); \ $(INSTALL_DATA) $$i $(DESTDIR)$(INCLUDEPY); \ done - @for i in $(srcdir)/Include/cpython/*.h; \ + @set -e; \ + for i in $(srcdir)/Include/cpython/*.h; \ do \ echo $(INSTALL_DATA) $$i $(INCLUDEPY)/cpython; \ $(INSTALL_DATA) $$i $(DESTDIR)$(INCLUDEPY)/cpython; \ done - @for i in $(srcdir)/Include/internal/*.h; \ + @set -e; \ + for i in $(srcdir)/Include/internal/*.h; \ do \ echo $(INSTALL_DATA) $$i $(INCLUDEPY)/internal; \ $(INSTALL_DATA) $$i $(DESTDIR)$(INCLUDEPY)/internal; \ @@ -2244,7 +2260,8 @@ LIBPL= @LIBPL@ LIBPC= $(LIBDIR)/pkgconfig libainstall: all scripts - @for i in $(LIBDIR) $(LIBPL) $(LIBPC) $(BINDIR); \ + @set -e; \ + for i in $(LIBDIR) $(LIBPL) $(LIBPC) $(BINDIR); \ do \ if test ! -d $(DESTDIR)$$i; then \ echo "Creating directory $$i"; \ @@ -2252,7 +2269,8 @@ libainstall: all scripts else true; \ fi; \ done - @if test "$(STATIC_LIBPYTHON)" = 1; then \ + @set -e; \ + if test "$(STATIC_LIBPYTHON)" = 1; then \ if test -d $(LIBRARY); then :; else \ if test "$(PYTHONFRAMEWORKDIR)" = no-framework; then \ if test "$(SHLIB_SUFFIX)" = .dll; then \ @@ -2282,7 +2300,8 @@ libainstall: all scripts $(INSTALL_SCRIPT) $(SCRIPT_2TO3) $(DESTDIR)$(BINDIR)/2to3-$(VERSION) $(INSTALL_SCRIPT) $(SCRIPT_IDLE) $(DESTDIR)$(BINDIR)/idle$(VERSION) $(INSTALL_SCRIPT) $(SCRIPT_PYDOC) $(DESTDIR)$(BINDIR)/pydoc$(VERSION) - @if [ -s Modules/python.exp -a \ + @set -e; \ + if [ -s Modules/python.exp -a \ "`echo $(MACHDEP) | sed 's/^\(...\).*/\1/'`" = "aix" ]; then \ echo; echo "Installing support files for building shared extension modules on AIX:"; \ $(INSTALL_DATA) Modules/python.exp \ @@ -2322,7 +2341,8 @@ frameworkinstallstructure: $(LDLIBRARY) exit 1; \ else true; \ fi - @for i in $(prefix)/Resources/English.lproj $(prefix)/lib; do\ + @set -e; \ + for i in $(prefix)/Resources/English.lproj $(prefix)/lib; do\ if test ! -d $(DESTDIR)$$i; then \ echo "Creating directory $(DESTDIR)$$i"; \ $(INSTALL) -d -m $(DIRMODE) $(DESTDIR)$$i; \ diff --git a/Misc/NEWS.d/next/Build/2022-12-18-07-24-44.gh-issue-100220.BgSV7C.rst b/Misc/NEWS.d/next/Build/2022-12-18-07-24-44.gh-issue-100220.BgSV7C.rst new file mode 100644 index 00000000000000..3de0527c7c692e --- /dev/null +++ b/Misc/NEWS.d/next/Build/2022-12-18-07-24-44.gh-issue-100220.BgSV7C.rst @@ -0,0 +1,2 @@ +Fix error handling in compound Make rules that lead to the install targets +succeeding when the respective files were not installed. From 86c946f5f9dbaace6b34279218470a72c73317fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20G=C3=B3rny?= Date: Fri, 3 Mar 2023 14:53:20 +0100 Subject: [PATCH 2/3] Set shell flag `-e` globally Instead of setting `set -e` per target, add it to the definition of `SHELL` to force it globally. This was suggested by Zachary Ware. --- Makefile.pre.in | 60 +++++++++++++++++-------------------------------- 1 file changed, 20 insertions(+), 40 deletions(-) diff --git a/Makefile.pre.in b/Makefile.pre.in index 84c90b281e44bd..b3bf4e28dd3d68 100644 --- a/Makefile.pre.in +++ b/Makefile.pre.in @@ -59,7 +59,7 @@ DSYMUTIL_PATH= @DSYMUTIL_PATH@ GNULD= @GNULD@ # Shell used by make (some versions default to the login shell, which is bad) -SHELL= /bin/sh +SHELL= /bin/sh -e # Use this to make a link between python$(VERSION) and python in $(BINDIR) LN= @LN@ @@ -738,7 +738,6 @@ $(LIBRARY): $(LIBRARY_OBJS) $(AR) $(ARFLAGS) $@ $(LIBRARY_OBJS) libpython$(LDVERSION).so: $(LIBRARY_OBJS) $(DTRACE_OBJS) - set -e; \ if test $(INSTSONAME) != $(LDLIBRARY); then \ $(BLDSHARED) -Wl,-h$(INSTSONAME) -o $(INSTSONAME) $(LIBRARY_OBJS) $(MODLIBS) $(SHLIBS) $(LIBC) $(LIBM); \ $(LN) -f $(INSTSONAME) $@; \ @@ -895,8 +894,7 @@ $(LIBEXPAT_A): $(LIBEXPAT_OBJS) # pybuilddir.txt is created too late. We cannot use it in Makefile # targets. ln --relative is not portable. sharedmods: $(SHAREDMODS) pybuilddir.txt - @set -e; \ - target=`cat pybuilddir.txt`; \ + @target=`cat pybuilddir.txt`; \ $(MKDIR_P) $$target; \ for mod in X $(SHAREDMODS); do \ if test $$mod != X; then \ @@ -909,8 +907,7 @@ checksharedmods: sharedmods $(PYTHON_FOR_BUILD_DEPS) $(BUILDPYTHON) @$(RUNSHARED) $(PYTHON_FOR_BUILD) $(srcdir)/Tools/build/check_extension_modules.py rundsymutil: sharedmods $(PYTHON_FOR_BUILD_DEPS) $(BUILDPYTHON) - @set -e; \ - if [ ! -z $(DSYMUTIL) ] ; then \ + @if [ ! -z $(DSYMUTIL) ] ; then \ echo $(DSYMUTIL_PATH) $(BUILDPYTHON); \ $(DSYMUTIL_PATH) $(BUILDPYTHON); \ if test -f $(LDLIBRARY); then \ @@ -1817,8 +1814,7 @@ commoninstall: check-clean-src @FRAMEWORKALTINSTALLFIRST@ \ DESTDIRS= $(exec_prefix) $(LIBDIR) $(BINLIBDEST) $(DESTSHARED) sharedinstall: all - @set -e; \ - for i in $(DESTDIRS); \ + @for i in $(DESTDIRS); \ do \ if test ! -d $(DESTDIR)$$i; then \ echo "Creating directory $$i"; \ @@ -1826,8 +1822,7 @@ sharedinstall: all else true; \ fi; \ done - @set -e; \ - for i in X $(SHAREDMODS); do \ + @for i in X $(SHAREDMODS); do \ if test $$i != X; then \ echo $(INSTALL_SHARED) $$i $(DESTSHARED)/`basename $$i`; \ $(INSTALL_SHARED) $$i $(DESTDIR)$(DESTSHARED)/`basename $$i`; \ @@ -1841,8 +1836,7 @@ sharedinstall: all # Install the interpreter with $(VERSION) affixed # This goes into $(exec_prefix) altbininstall: $(BUILDPYTHON) @FRAMEWORKPYTHONW@ - @set -e; \ - for i in $(BINDIR) $(LIBDIR); \ + @for i in $(BINDIR) $(LIBDIR); \ do \ if test ! -d $(DESTDIR)$$i; then \ echo "Creating directory $$i"; \ @@ -1861,8 +1855,7 @@ altbininstall: $(BUILDPYTHON) @FRAMEWORKPYTHONW@ fi; \ (cd $(DESTDIR)$(BINDIR); $(LN) python$(LDVERSION)$(EXE) python$(VERSION)$(EXE)); \ fi - @set -e; \ - if test "$(PY_ENABLE_SHARED)" = 1 -o "$(STATIC_LIBPYTHON)" = 1; then \ + @if test "$(PY_ENABLE_SHARED)" = 1 -o "$(STATIC_LIBPYTHON)" = 1; then \ if test -f $(LDLIBRARY) && test "$(PYTHONFRAMEWORKDIR)" = "no-framework" ; then \ if test -n "$(DLLLIBRARY)" ; then \ $(INSTALL_SHARED) $(DLLLIBRARY) $(DESTDIR)$(BINDIR); \ @@ -1949,8 +1942,7 @@ bininstall: altbininstall # Install the versioned manual page altmaninstall: - @set -e; \ - for i in $(MANDIR) $(MANDIR)/man1; \ + @for i in $(MANDIR) $(MANDIR)/man1; \ do \ if test ! -d $(DESTDIR)$$i; then \ echo "Creating directory $$i"; \ @@ -2085,8 +2077,7 @@ COMPILEALL_OPTS=-j0 TEST_MODULES=@TEST_MODULES@ libinstall: all $(srcdir)/Modules/xxmodule.c - @set -e; \ - for i in $(SCRIPTDIR) $(LIBDEST); \ + @for i in $(SCRIPTDIR) $(LIBDEST); \ do \ if test ! -d $(DESTDIR)$$i; then \ echo "Creating directory $$i"; \ @@ -2094,8 +2085,7 @@ libinstall: all $(srcdir)/Modules/xxmodule.c else true; \ fi; \ done - @set -e; \ - if test "$(TEST_MODULES)" = yes; then \ + @if test "$(TEST_MODULES)" = yes; then \ subdirs="$(LIBSUBDIRS) $(TESTSUBDIRS)"; \ else \ subdirs="$(LIBSUBDIRS)"; \ @@ -2111,8 +2101,7 @@ libinstall: all $(srcdir)/Modules/xxmodule.c else true; \ fi; \ done - @set -e; \ - for i in $(srcdir)/Lib/*.py; \ + @for i in $(srcdir)/Lib/*.py; \ do \ if test -x $$i; then \ $(INSTALL_SCRIPT) $$i $(DESTDIR)$(LIBDEST); \ @@ -2122,8 +2111,7 @@ libinstall: all $(srcdir)/Modules/xxmodule.c echo $(INSTALL_DATA) $$i $(LIBDEST); \ fi; \ done - @set -e; \ - if test "$(TEST_MODULES)" = yes; then \ + @if test "$(TEST_MODULES)" = yes; then \ subdirs="$(LIBSUBDIRS) $(TESTSUBDIRS)"; \ else \ subdirs="$(LIBSUBDIRS)"; \ @@ -2213,8 +2201,7 @@ scripts: $(SCRIPT_2TO3) $(SCRIPT_IDLE) $(SCRIPT_PYDOC) python-config # Install the include files INCLDIRSTOMAKE=$(INCLUDEDIR) $(CONFINCLUDEDIR) $(INCLUDEPY) $(CONFINCLUDEPY) inclinstall: - @set -e; \ - for i in $(INCLDIRSTOMAKE); \ + @for i in $(INCLDIRSTOMAKE); \ do \ if test ! -d $(DESTDIR)$$i; then \ echo "Creating directory $$i"; \ @@ -2232,20 +2219,17 @@ inclinstall: $(INSTALL) -d -m $(DIRMODE) $(DESTDIR)$(INCLUDEPY)/internal; \ else true; \ fi - @set -e; \ - for i in $(srcdir)/Include/*.h; \ + @for i in $(srcdir)/Include/*.h; \ do \ echo $(INSTALL_DATA) $$i $(INCLUDEPY); \ $(INSTALL_DATA) $$i $(DESTDIR)$(INCLUDEPY); \ done - @set -e; \ - for i in $(srcdir)/Include/cpython/*.h; \ + @for i in $(srcdir)/Include/cpython/*.h; \ do \ echo $(INSTALL_DATA) $$i $(INCLUDEPY)/cpython; \ $(INSTALL_DATA) $$i $(DESTDIR)$(INCLUDEPY)/cpython; \ done - @set -e; \ - for i in $(srcdir)/Include/internal/*.h; \ + @for i in $(srcdir)/Include/internal/*.h; \ do \ echo $(INSTALL_DATA) $$i $(INCLUDEPY)/internal; \ $(INSTALL_DATA) $$i $(DESTDIR)$(INCLUDEPY)/internal; \ @@ -2260,8 +2244,7 @@ LIBPL= @LIBPL@ LIBPC= $(LIBDIR)/pkgconfig libainstall: all scripts - @set -e; \ - for i in $(LIBDIR) $(LIBPL) $(LIBPC) $(BINDIR); \ + @for i in $(LIBDIR) $(LIBPL) $(LIBPC) $(BINDIR); \ do \ if test ! -d $(DESTDIR)$$i; then \ echo "Creating directory $$i"; \ @@ -2269,8 +2252,7 @@ libainstall: all scripts else true; \ fi; \ done - @set -e; \ - if test "$(STATIC_LIBPYTHON)" = 1; then \ + @if test "$(STATIC_LIBPYTHON)" = 1; then \ if test -d $(LIBRARY); then :; else \ if test "$(PYTHONFRAMEWORKDIR)" = no-framework; then \ if test "$(SHLIB_SUFFIX)" = .dll; then \ @@ -2300,8 +2282,7 @@ libainstall: all scripts $(INSTALL_SCRIPT) $(SCRIPT_2TO3) $(DESTDIR)$(BINDIR)/2to3-$(VERSION) $(INSTALL_SCRIPT) $(SCRIPT_IDLE) $(DESTDIR)$(BINDIR)/idle$(VERSION) $(INSTALL_SCRIPT) $(SCRIPT_PYDOC) $(DESTDIR)$(BINDIR)/pydoc$(VERSION) - @set -e; \ - if [ -s Modules/python.exp -a \ + @if [ -s Modules/python.exp -a \ "`echo $(MACHDEP) | sed 's/^\(...\).*/\1/'`" = "aix" ]; then \ echo; echo "Installing support files for building shared extension modules on AIX:"; \ $(INSTALL_DATA) Modules/python.exp \ @@ -2341,8 +2322,7 @@ frameworkinstallstructure: $(LDLIBRARY) exit 1; \ else true; \ fi - @set -e; \ - for i in $(prefix)/Resources/English.lproj $(prefix)/lib; do\ + @for i in $(prefix)/Resources/English.lproj $(prefix)/lib; do\ if test ! -d $(DESTDIR)$$i; then \ echo "Creating directory $(DESTDIR)$$i"; \ $(INSTALL) -d -m $(DIRMODE) $(DESTDIR)$$i; \ From 4006cd4e211c0a53c218b8cf3494d93803fa6d36 Mon Sep 17 00:00:00 2001 From: Zachary Ware Date: Fri, 7 Apr 2023 11:16:46 -0500 Subject: [PATCH 3/3] Update Misc/NEWS.d/next/Build/2022-12-18-07-24-44.gh-issue-100220.BgSV7C.rst --- .../Build/2022-12-18-07-24-44.gh-issue-100220.BgSV7C.rst | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Build/2022-12-18-07-24-44.gh-issue-100220.BgSV7C.rst b/Misc/NEWS.d/next/Build/2022-12-18-07-24-44.gh-issue-100220.BgSV7C.rst index 3de0527c7c692e..7135317cd06fa2 100644 --- a/Misc/NEWS.d/next/Build/2022-12-18-07-24-44.gh-issue-100220.BgSV7C.rst +++ b/Misc/NEWS.d/next/Build/2022-12-18-07-24-44.gh-issue-100220.BgSV7C.rst @@ -1,2 +1,4 @@ -Fix error handling in compound Make rules that lead to the install targets -succeeding when the respective files were not installed. +Changed the default value of the ``SHELL`` Makefile variable from ``/bin/sh`` +to ``/bin/sh -e`` to ensure that complex recipes correctly fail after an error. +Previously, ``make install`` could fail to install some files and yet return +a successful result.