[make] make sure no command fails

Summary: Run with `SHELL = bash -e -u -o pipefail` to catch many kinds of failures. We were silently failing during `make install` because of some missing escaping, and the failure was hidden because it was happening inside a bash `for` loop. This fixes the escaping issue and makes sure such issues will result in an error as of now. Also removes dangerous `find -exec` instances: `find` will `exit 0` event if some commands failed. Fixes #887 Reviewed By: mbouaziz Differential Revision: D7569054 fbshipit-source-id: 542fe50
7 years ago · f1bcb91542
parent 594ddab2a5
commit f1bcb91542
6 changed files with 101 additions and 113 deletions
--- a/176
+++ b/176
@ -285,8 +285,7 @@ ocaml_unit_test: test_build
 	INFER_ARGS=--results-dir^infer-out-unit-tests $(BUILD_DIR)/test/inferunit.bc)

 define silence_make
-  ($(1) 2> >(grep -v "warning: \(ignoring old\|overriding\) \(commands\|recipe\) for target") \
-  ; exit $${PIPESTATUS[0]})
+  $(1) 2> >(grep -v "warning: \(ignoring old\|overriding\) \(commands\|recipe\) for target")
 endef

 .PHONY: $(DIRECT_TESTS:%=direct_%_test)
@ -424,7 +423,7 @@ test-replace: $(BUILD_SYSTEMS_TESTS:%=build_%_replace) $(DIRECT_TESTS:%=direct_%
 .PHONY: uninstall
 uninstall:
 	$(REMOVE_DIR) $(DESTDIR)$(libdir)/infer/
-	$(REMOVE) $(DESTDIR)$(bindir)/infer
+	$(REMOVE) $(DESTDIR)$(bindir)/infer*
 	$(REMOVE) $(INFER_COMMANDS:%=$(DESTDIR)$(bindir)/%)
 	$(REMOVE) $(foreach manual,$(INFER_MANUALS_GZIPPED),\
 	  $(DESTDIR)$(mandir)/man1/$(notdir $(manual)))
@ -435,112 +434,101 @@ test_clean: $(DIRECT_TESTS:%=direct_%_clean) $(BUILD_SYSTEMS_TESTS:%=build_%_cle
 .PHONY: install
 install: infer $(INFER_MANUALS_GZIPPED)
 # create directory structure
-	test -d      $(DESTDIR)$(bindir) || \
-	  $(MKDIR_P) $(DESTDIR)$(bindir)
-	test -d      $(DESTDIR)$(mandir)/man1 || \
-	  $(MKDIR_P) $(DESTDIR)$(mandir)/man1
-	test -d      $(DESTDIR)$(libdir)/infer/ || \
-	  $(MKDIR_P) $(DESTDIR)$(libdir)/infer/
+	test -d      '$(DESTDIR)$(bindir)' || \
+	  $(MKDIR_P) '$(DESTDIR)$(bindir)'
+	test -d      '$(DESTDIR)$(mandir)/man1' || \
+	  $(MKDIR_P) '$(DESTDIR)$(mandir)/man1'
+	test -d      '$(DESTDIR)$(libdir)/infer/' || \
+	  $(MKDIR_P) '$(DESTDIR)$(libdir)/infer/'
 ifeq ($(BUILD_C_ANALYZERS),yes)
-	test -d      $(DESTDIR)$(libdir)/infer/facebook-clang-plugins/libtooling/build/ || \
-	  $(MKDIR_P) $(DESTDIR)$(libdir)/infer/facebook-clang-plugins/libtooling/build/
-	$(QUIET)for i in $$(find facebook-clang-plugins/clang/install -type d); do \
-	  test -d      $(DESTDIR)$(libdir)/infer/$$i || \
-	    $(MKDIR_P) $(DESTDIR)$(libdir)/infer/$$i; \
-	done
-	test -d      $(DESTDIR)$(libdir)/infer/infer/lib/clang_wrappers/ || \
-	  $(MKDIR_P) $(DESTDIR)$(libdir)/infer/infer/lib/clang_wrappers/
-	$(QUIET)for i in $$(find infer/models/cpp/include/ -type d); do \
-	  test -d      $(DESTDIR)$(libdir)/infer/$$i || \
-	    $(MKDIR_P) $(DESTDIR)$(libdir)/infer/$$i; \
-	done
-	test -d      $(DESTDIR)$(libdir)/infer/infer/lib/linter_rules/ || \
-	  $(MKDIR_P) $(DESTDIR)$(libdir)/infer/infer/lib/linter_rules
-	test -d      $(DESTDIR)$(libdir)/infer/infer/etc/ || \
-		$(MKDIR_P) $(DESTDIR)$(libdir)/infer/infer/etc
+	test -d      '$(DESTDIR)$(libdir)/infer/facebook-clang-plugins/libtooling/build/' || \
+	  $(MKDIR_P) '$(DESTDIR)$(libdir)/infer/facebook-clang-plugins/libtooling/build/'
+	find facebook-clang-plugins/clang/install -type d -print0 | xargs -0 -I \{\} \
+	  $(SHELL) -c "test -d '$(DESTDIR)$(libdir)'/infer/{} || \
+	    $(MKDIR_P) '$(DESTDIR)$(libdir)'/infer/{}"
+	test -d      '$(DESTDIR)$(libdir)/infer/infer/lib/clang_wrappers/' || \
+	  $(MKDIR_P) '$(DESTDIR)$(libdir)/infer/infer/lib/clang_wrappers/'
+	find infer/models/cpp/include -type d -print0 | xargs -0 -I \{\} \
+	  $(SHELL) -c "test -d '$(DESTDIR)$(libdir)'/infer/{} || \
+	    $(MKDIR_P) '$(DESTDIR)$(libdir)'/infer/{}"
+	test -d      '$(DESTDIR)$(libdir)/infer/infer/lib/linter_rules/' || \
+	  $(MKDIR_P) '$(DESTDIR)$(libdir)/infer/infer/lib/linter_rules/'
+	test -d      '$(DESTDIR)$(libdir)/infer/infer/etc/' || \
+		$(MKDIR_P) '$(DESTDIR)$(libdir)/infer/infer/etc'
 endif
 ifeq ($(BUILD_JAVA_ANALYZERS),yes)
-	test -d      $(DESTDIR)$(libdir)/infer/infer/lib/java/ || \
-	  $(MKDIR_P) $(DESTDIR)$(libdir)/infer/infer/lib/java/
-endif
-	test -d      $(DESTDIR)$(libdir)/infer/infer/annotations/ || \
-	  $(MKDIR_P) $(DESTDIR)$(libdir)/infer/infer/annotations/
-	test -d      $(DESTDIR)$(libdir)/infer/infer/lib/wrappers/ || \
-	  $(MKDIR_P) $(DESTDIR)$(libdir)/infer/infer/lib/wrappers/
-	test -d      $(DESTDIR)$(libdir)/infer/infer/lib/specs/ || \
-	  $(MKDIR_P) $(DESTDIR)$(libdir)/infer/infer/lib/specs/
-	test -d      $(DESTDIR)$(libdir)/infer/infer/lib/python/ || \
-	  $(MKDIR_P) $(DESTDIR)$(libdir)/infer/infer/lib/python/
-	test -d      $(DESTDIR)$(libdir)/infer/infer/lib/python/inferlib/ || \
-	  $(MKDIR_P) $(DESTDIR)$(libdir)/infer/infer/lib/python/inferlib/
-	test -d      $(DESTDIR)$(libdir)/infer/infer/lib/python/inferlib/capture/ || \
-	  $(MKDIR_P) $(DESTDIR)$(libdir)/infer/infer/lib/python/inferlib/capture/
-	test -d      $(DESTDIR)$(libdir)/infer/infer/bin/ || \
-	  $(MKDIR_P) $(DESTDIR)$(libdir)/infer/infer/bin/
-
+	test -d      '$(DESTDIR)$(libdir)/infer/infer/lib/java/' || \
+	  $(MKDIR_P) '$(DESTDIR)$(libdir)/infer/infer/lib/java/'
+endif
+	test -d      '$(DESTDIR)$(libdir)/infer/infer/annotations/' || \
+	  $(MKDIR_P) '$(DESTDIR)$(libdir)/infer/infer/annotations/'
+	test -d      '$(DESTDIR)$(libdir)/infer/infer/lib/wrappers/' || \
+	  $(MKDIR_P) '$(DESTDIR)$(libdir)/infer/infer/lib/wrappers/'
+	test -d      '$(DESTDIR)$(libdir)/infer/infer/lib/specs/' || \
+	  $(MKDIR_P) '$(DESTDIR)$(libdir)/infer/infer/lib/specs/'
+	test -d      '$(DESTDIR)$(libdir)/infer/infer/lib/python/' || \
+	  $(MKDIR_P) '$(DESTDIR)$(libdir)/infer/infer/lib/python/'
+	test -d      '$(DESTDIR)$(libdir)/infer/infer/lib/python/inferlib/' || \
+	  $(MKDIR_P) '$(DESTDIR)$(libdir)/infer/infer/lib/python/inferlib/'
+	test -d      '$(DESTDIR)$(libdir)/infer/infer/lib/python/inferlib/capture/' || \
+	  $(MKDIR_P) '$(DESTDIR)$(libdir)/infer/infer/lib/python/inferlib/capture/'
+	test -d      '$(DESTDIR)$(libdir)/infer/infer/bin/' || \
+	  $(MKDIR_P) '$(DESTDIR)$(libdir)/infer/infer/bin/'
 # copy files
 ifeq ($(BUILD_C_ANALYZERS),yes)
-	$(INSTALL_DATA) -C          facebook-clang-plugins/libtooling/build/FacebookClangPlugin.dylib \
-	  $(DESTDIR)$(libdir)/infer/facebook-clang-plugins/libtooling/build/FacebookClangPlugin.dylib
-	$(QUIET)for i in $$(find facebook-clang-plugins/clang/install -not -type d); do \
-	  $(INSTALL_PROGRAM) -C $$i $(DESTDIR)$(libdir)/infer/$$i; \
-	done
-	$(QUIET)for i in $$(find infer/lib/clang_wrappers/*); do \
-	  $(INSTALL_PROGRAM) -C $$i $(DESTDIR)$(libdir)/infer/$$i; \
-	done
+	$(INSTALL_DATA) -C          'facebook-clang-plugins/libtooling/build/FacebookClangPlugin.dylib' \
+	  '$(DESTDIR)$(libdir)/infer/facebook-clang-plugins/libtooling/build/FacebookClangPlugin.dylib'
+	find facebook-clang-plugins/clang/install -not -type d -print0 | xargs -0 -I \{\} \
+	  $(INSTALL_PROGRAM) -C \{\} '$(DESTDIR)$(libdir)'/infer/\{\}
+	find infer/lib/clang_wrappers/* -print0 | xargs -0 -I \{\} \
+	  $(INSTALL_PROGRAM) -C \{\} '$(DESTDIR)$(libdir)'/infer/\{\}
 #	  only for files that point to infer
-	(cd $(DESTDIR)$(libdir)/infer/infer/lib/wrappers/ && \
-	 $(foreach cc,$(shell find $(LIB_DIR)/wrappers -type l), \
-	  [ $(cc) -ef $(INFER_BIN) ] && \
-	  $(REMOVE) $(notdir $(cc)) && \
-	  $(LN_S) ../../bin/infer $(notdir $(cc));))
-	$(QUIET)for i in $$(find infer/lib/specs/*); do \
-	  $(INSTALL_DATA) -C $$i $(DESTDIR)$(libdir)/infer/$$i; \
-	done
-	$(QUIET)for i in $$(find infer/models/cpp/include/ -not -type d); do \
-		$(INSTALL_DATA) -C $$i $(DESTDIR)$(libdir)/infer/$$i; \
-	done
-	$(INSTALL_DATA) -C          infer/lib/linter_rules/linters.al \
-	  $(DESTDIR)$(libdir)/infer/infer/lib/linter_rules/linters.al
-	$(INSTALL_DATA) -C          infer/etc/clang_ast.dict \
-	  $(DESTDIR)$(libdir)/infer/infer/etc/clang_ast.dict
+	(cd '$(DESTDIR)$(libdir)/infer/infer/lib/wrappers/' && \
+	 $(foreach cc,$(shell find '$(LIB_DIR)/wrappers' -type l), \
+	  [ $(cc) -ef '$(INFER_BIN)' ] && \
+	  $(REMOVE) '$(notdir $(cc))' && \
+	  $(LN_S) ../../bin/infer '$(notdir $(cc))';))
+	find infer/lib/specs/* -print0 | xargs -0 -I \{\} \
+	  $(INSTALL_DATA) -C \{\} '$(DESTDIR)$(libdir)'/infer/\{\}
+	find infer/models/cpp/include -not -type d -print0 | xargs -0 -I \{\} \
+		$(INSTALL_DATA) -C \{\} '$(DESTDIR)$(libdir)'/infer/\{\}
+	$(INSTALL_DATA) -C          'infer/lib/linter_rules/linters.al' \
+	  '$(DESTDIR)$(libdir)/infer/infer/lib/linter_rules/linters.al'
+	$(INSTALL_DATA) -C          'infer/etc/clang_ast.dict' \
+	  '$(DESTDIR)$(libdir)/infer/infer/etc/clang_ast.dict'
 endif
 ifeq ($(BUILD_JAVA_ANALYZERS),yes)
-	$(INSTALL_DATA) -C          infer/annotations/annotations.jar \
-	  $(DESTDIR)$(libdir)/infer/infer/annotations/annotations.jar
-	$(QUIET)for i in infer/lib/java/*.jar; do \
-	  $(INSTALL_DATA) -C $$i $(DESTDIR)$(libdir)/infer/$$i; \
-	done
-	$(INSTALL_PROGRAM) -C      $(LIB_DIR)/wrappers/javac \
-	  $(DESTDIR)$(libdir)/infer/infer/lib/wrappers/
-endif
-	$(QUIET)for i in $$(find infer/lib/python/inferlib/* -type f); do \
-	  $(INSTALL_DATA) -C $$i $(DESTDIR)$(libdir)/infer/$$i; \
-	done
+	$(INSTALL_DATA) -C          'infer/annotations/annotations.jar' \
+	  '$(DESTDIR)$(libdir)/infer/infer/annotations/annotations.jar'
+	find infer/lib/java/*.jar -print0 | xargs -0 -I \{\} \
+	  $(INSTALL_DATA) -C \{\} '$(DESTDIR)$(libdir)'/infer/\{\}
+	$(INSTALL_PROGRAM) -C      '$(LIB_DIR)'/wrappers/javac \
+	  '$(DESTDIR)$(libdir)'/infer/infer/lib/wrappers/
+endif
+	find infer/lib/python/inferlib/* -type f -print0 | xargs -0 -I \{\} \
+	  $(INSTALL_DATA) -C \{\} '$(DESTDIR)$(libdir)'/infer/\{\}
 	$(INSTALL_PROGRAM) -C       infer/lib/python/infer.py \
-	  $(DESTDIR)$(libdir)/infer/infer/lib/python/infer.py
+	  '$(DESTDIR)$(libdir)'/infer/infer/lib/python/infer.py
 	$(INSTALL_PROGRAM) -C       infer/lib/python/inferTraceBugs \
-	  $(DESTDIR)$(libdir)/infer/infer/lib/python/inferTraceBugs
+	  '$(DESTDIR)$(libdir)'/infer/infer/lib/python/inferTraceBugs
 	$(INSTALL_PROGRAM) -C       infer/lib/python/report.py \
-	  $(DESTDIR)$(libdir)/infer/infer/lib/python/report.py
-	$(INSTALL_PROGRAM) -C $(INFER_BIN) $(DESTDIR)$(libdir)/infer/infer/bin/
-	(cd $(DESTDIR)$(bindir)/ && \
+	  '$(DESTDIR)$(libdir)'/infer/infer/lib/python/report.py
+	$(INSTALL_PROGRAM) -C '$(INFER_BIN)' '$(DESTDIR)$(libdir)'/infer/infer/bin/
+	(cd '$(DESTDIR)$(bindir)/' && \
 	 $(REMOVE) infer && \
-	 $(LN_S) $(libdir_relative_to_bindir)/infer/infer/bin/infer infer)
+	 $(LN_S) '$(libdir_relative_to_bindir)'/infer/infer/bin/infer infer)
 	for alias in $(INFER_COMMANDS); do \
-	  (cd $(DESTDIR)$(bindir)/ && \
-	   $(REMOVE) $$alias && \
-	   $(LN_S) infer $$alias); done
+	  (cd '$(DESTDIR)$(bindir)'/ && \
+	   $(REMOVE) "$$alias" && \
+	   $(LN_S) infer "$$alias"); done
 	for alias in $(INFER_COMMANDS); do \
-	  (cd $(DESTDIR)$(libdir)/infer/infer/bin && \
-	   $(REMOVE) $$alias && \
-	   $(LN_S) infer $$alias); done
-	$(QUIET)for i in $(MAN_DIR)/man1/*; do \
-	  $(INSTALL_DATA) -C $$i $(DESTDIR)$(mandir)/man1/$$(basename $$i); \
-	done
-
+	  (cd '$(DESTDIR)$(libdir)'/infer/infer/bin && \
+	   $(REMOVE) "$$alias" && \
+	   $(LN_S) infer "$$alias"); done
+	find '$(MAN_DIR)'/man1 -print0 | xargs -0 \
+	  $(SHELL) -c '$(INSTALL_DATA) -C $$1 "$(DESTDIR)$(mandir)/man1/$$(basename $$1)"'
 ifeq ($(IS_FACEBOOK_TREE),yes)
-	$(QUIET)$(MAKE) -C facebook install
+	$(MAKE) -C facebook install
 endif

 # Nuke objects built from OCaml. Useful when changing the OCaml compiler, for instance.
--- a/Makefile.config
+++ b/Makefile.config
@ -6,7 +6,7 @@
 # of patent rights can be found in the PATENTS file in the same directory.

 ORIG_SHELL = $(shell echo "$$SHELL")
-SHELL = bash
+SHELL = bash -e -o pipefail -u

 ORIG_SHELL_PATH = $(shell printf "%s" "$$PATH")

@ -153,10 +153,10 @@ else
 # Detect if we are already wrapped inside a silent_on_success call and try not to clutter the output
 # too much in that case.
 define silent_on_success
-  if [ "$$INSIDE_SILENT_ON_SUCCESS" = 1 ]; then \
-    echo "*** inner $(1)"; \
-    echo "*** inner command: $(2)"; \
-    echo "*** inner CWD: $(CURDIR)"; \
+  if [ -n "$${INSIDE_SILENT_ON_SUCCESS-}" ]; then \
+    echo '*** inner $(1)'; \
+    echo '*** inner command: $(2)'; \
+    echo '*** inner CWD: $(CURDIR)'; \
    ($(2)); \
    exit $$?; \
  fi; \
@ -165,7 +165,7 @@ define silent_on_success
  UNIX_START_DATE=$$(date +"%s"); \
  HUMAN_START_DATE=$$(date +"%H:%M:%S"); \
  if [ -z $(SILENT) ]; then \
-    printf "[%s][%$(MAX_PID_SIZE)s] $(TERM_INFO)%s...$(TERM_RESET)\n" \
+    printf '[%s][%$(MAX_PID_SIZE)s] $(TERM_INFO)%s...$(TERM_RESET)\n' \
      "$$HUMAN_START_DATE" "$$HASH" "$(1)"; \
  fi; \
  $(MKDIR_P) $(ABSOLUTE_ROOT_DIR)/_build_logs; \
@ -173,9 +173,9 @@ define silent_on_success
         2>$(ABSOLUTE_ROOT_DIR)/_build_logs/cmd-$$HASH.err; \
  ERRCODE=$$?; \
  if [ $$ERRCODE != 0 ]; then \
-    echo "$(TERM_ERROR)[*ERROR**][$$HASH] *** ERROR $(1)$(TERM_RESET)" >&2; \
-    echo "$(TERM_ERROR)[*ERROR**][$$HASH] *** command: $(2)$(TERM_RESET)" >&2; \
-    echo "$(TERM_ERROR)[*ERROR**][$$HASH] *** CWD: $(CURDIR)$(TERM_RESET)" >&2; \
+    echo "$(TERM_ERROR)[*ERROR**][$$HASH] *** ERROR '$(1)'$(TERM_RESET)" >&2; \
+    echo "$(TERM_ERROR)[*ERROR**][$$HASH] *** command: '$(2)'$(TERM_RESET)" >&2; \
+    echo "$(TERM_ERROR)[*ERROR**][$$HASH] *** CWD: '$(CURDIR)'$(TERM_RESET)" >&2; \
    echo "$(TERM_ERROR)[*ERROR**][$$HASH] *** stdout:$(TERM_RESET)" >&2; \
    sed -e "s/^\(.*\)$$/$(TERM_ERROR)[*ERROR**][$$HASH]$(TERM_RESET) \1/" \
      $(ABSOLUTE_ROOT_DIR)/_build_logs/cmd-$$HASH.out; >&2; \
--- a/configure.ac
+++ b/configure.ac
@ -177,7 +177,6 @@ pre-compiled here:
 fi
 # end if($enable_c_analyzers)

-
 # OCaml dependencies
 AC_PROG_OCAML
 AC_ASSERT_PROG([ocamlc], [$OCAMLC])
--- a/infer/tests/base.make
+++ b/infer/tests/base.make
@ -14,10 +14,11 @@ include $(ROOT_DIR)/Makefile.config
 TEST_REL_DIR = $(patsubst $(abspath $(TESTS_DIR))/%,%,$(abspath $(CURDIR)))

 define check_no_duplicates
-  grep "DUPLICATE_SYMBOLS" $(1); test $$? -ne 0 || \
-  (echo '$(TEST_ERROR)Duplicate symbols found in $(CURDIR).' \
-   'Please make sure all the function names in all the source test files are different.$(TEST_RESET)';\
-   exit 1)
+  if grep -q "DUPLICATE_SYMBOLS" $(1); then \
+    printf '$(TERM_ERROR)Duplicate symbols found in $(CURDIR).$(TERM_RESET)\n' >&2; \
+    printf '$(TERM_ERROR)Please make sure all the function names in all the source test files are different.$(TERM_RESET)' >&2; \
+    exit 1; \
+  fi
 endef

 define check_no_diff
--- a/infer/tests/build_systems/fail_on_issue/Makefile
+++ b/infer/tests/build_systems/fail_on_issue/Makefile
@ -24,8 +24,8 @@ default: compile

 issues.exp.test: $(CLANG_DEPS) $(SOURCES)
 	$(QUIET)$(call silent_on_success,Testing Infer fails on issue,\
-	  ($(INFER_BIN) --fail-on-issue -- clang $(CLANG_OPTIONS) $(SOURCES); \
-	   echo "infer exit code: $$?" > $@))
+	  exit_code=0; $(INFER_BIN) --fail-on-issue -- clang $(CLANG_OPTIONS) $(SOURCES) || exit_code=$$?; \
+	  echo "infer exit code: $$exit_code" > $@)

 .PHONY: compile
 compile: $(OBJECTS)
--- a/infer/tests/clang-frontend.make
+++ b/infer/tests/clang-frontend.make
@ -29,10 +29,10 @@ print: capture

 .PHONY: test
 test: capture
-	$(QUIET)for file in $(SOURCES) ; do \
-	  diff -u $$file.dot $$file.test.dot || error=1 ; \
+	$(QUIET)error=0; for file in $(SOURCES) ; do \
+	  diff -u "$$file.dot" "$$file.test.dot" || error=1 ; \
 	done ; \
-	if [ 0$$error -eq 1 ]; then exit 1; fi
+	if [ $$error = 1 ]; then exit 1; fi

 .PHONY: replace
 replace: capture