[Rd] Sys.which() caching path to `which`

[Ivan Krylov]

On Thu, 11 Jan 2024 09:30:55 +1300
Simon Urbanek <simon.urbanek using R-project.org> wrote:

> That said, WHICH is a mess - it may make sense to switch to the
> command -v built-in which is part of POSIX (where available - which
> is almost everywhere today) which would not require an external tool

This is a bit tricky to implement. I've prepared the patch at the end
of this e-mail, tested it on GNU/Linux and tried to test on OpenBSD [*]
(I cannot test on a Mac), but then I realised one crucial detail:
unlike `which`, `command -v` returns names of shell builtins if
something is both an executable and a builtin. So for things like `[`,
Sys.which would behave differently if changed to use command -v:

$ sh -c 'which ['
/usr/bin/[
$ sh -c 'command -v ['
[

R checks the returned string with file.exists(), so the new
Sys.which('[') returns an empty string instead of /usr/bin/[. That's
probably undesirable, isn't it?

Index: configure
===================================================================
--- configure	(revision 85802)
+++ configure	(working copy)
@@ -949,7 +949,6 @@
 PDFTEX
 TEX
 PAGER
-WHICH
 SED
 INSTALL_DATA
 INSTALL_SCRIPT
@@ -5390,66 +5389,6 @@
 done
 test -n "$SED" || SED="/bin/sed"
 
-
-## 'which' is not POSIX, and might be a shell builtin or alias
-##  (but should not be in 'sh')
-for ac_prog in which
-do
-  # Extract the first word of "$ac_prog", so it can be a program name with args.
-set dummy $ac_prog; ac_word=$2
-{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5
-printf %s "checking for $ac_word... " >&6; }
-if test ${ac_cv_path_WHICH+y}
-then :
-  printf %s "(cached) " >&6
-else $as_nop
-  case $WHICH in
-  [\\/]* | ?:[\\/]*)
-  ac_cv_path_WHICH="$WHICH" # Let the user override the test with a path.
-  ;;
-  *)
-  as_save_IFS=$IFS; IFS=$PATH_SEPARATOR
-for as_dir in $PATH
-do
-  IFS=$as_save_IFS
-  case $as_dir in #(((
-    '') as_dir=./ ;;
-    */) ;;
-    *) as_dir=$as_dir/ ;;
-  esac
-    for ac_exec_ext in '' $ac_executable_extensions; do
-  if as_fn_executable_p "$as_dir$ac_word$ac_exec_ext"; then
-    ac_cv_path_WHICH="$as_dir$ac_word$ac_exec_ext"
-    printf "%s\n" "$as_me:${as_lineno-$LINENO}: found $as_dir$ac_word$ac_exec_ext" >&5
-    break 2
-  fi
-done
-  done
-IFS=$as_save_IFS
-
-  ;;
-esac
-fi
-WHICH=$ac_cv_path_WHICH
-if test -n "$WHICH"; then
-  { printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: $WHICH" >&5
-printf "%s\n" "$WHICH" >&6; }
-else
-  { printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: no" >&5
-printf "%s\n" "no" >&6; }
-fi
-
-
-  test -n "$WHICH" && break
-done
-test -n "$WHICH" || WHICH="which"
-
-if test "${WHICH}" = which ; then
-  ## needed to build and run R
-  ## ends up hard-coded in the utils package
-  as_fn_error $? "which is required but missing" "$LINENO" 5
-fi
-
 ## Make
 : ${MAKE=make}
 
Index: configure.ac
===================================================================
--- configure.ac	(revision 85802)
+++ configure.ac	(working copy)
@@ -680,15 +680,6 @@
 ## we would like a POSIX sed, and need one on Solaris
 AC_PATH_PROGS(SED, sed, /bin/sed, [/usr/xpg4/bin:$PATH])
 
-## 'which' is not POSIX, and might be a shell builtin or alias
-##  (but should not be in 'sh')
-AC_PATH_PROGS(WHICH, which, which)
-if test "${WHICH}" = which ; then
-  ## needed to build and run R
-  ## ends up hard-coded in the utils package
-  AC_MSG_ERROR([[which is required but missing]])
-fi
-
 ## Make
 : ${MAKE=make}
 AC_SUBST(MAKE)
Index: src/library/base/Makefile.in
===================================================================
--- src/library/base/Makefile.in	(revision 85802)
+++ src/library/base/Makefile.in	(working copy)
@@ -28,7 +28,7 @@
 all: Makefile DESCRIPTION
 	@$(ECHO) "building package '$(pkg)'"
 	@$(MKINSTALLDIRS) $(top_builddir)/library/$(pkg)
-	@WHICH="@WHICH@" $(MAKE) mkRbase mkdesc2 mkdemos2
+	@$(MAKE) mkRbase mkdesc2 mkdemos2
 	@$(INSTALL_DATA) $(srcdir)/inst/CITATION $(top_builddir)/library/$(pkg)
 
 include $(top_srcdir)/share/make/basepkg.mk
@@ -45,12 +45,12 @@
 mkR: mkRbase
 
 Rsimple:
-	@WHICH="@WHICH@" $(MAKE) mkRbase mkRsimple
+	@$(MAKE) mkRbase mkRsimple
 
 ## Remove files to allow this to be done repeatedly
 Rlazy:
 	- using rm -f  $(top_builddir)/library/$(pkg)/R/$(pkg)*
-	@WHICH="@WHICH@" $(MAKE) mkRbase
+	@$(MAKE) mkRbase
 	@cat $(srcdir)/makebasedb.R | \
 	  R_DEFAULT_PACKAGES=NULL LC_ALL=C $(R_EXE) > /dev/null
 	@$(INSTALL_DATA) $(srcdir)/baseloader.R \
@@ -57,4 +57,4 @@
 	  $(top_builddir)/library/$(pkg)/R/$(pkg)
 
 Rlazycomp:
-	@WHICH="@WHICH@" $(MAKE) mkRbase mklazycomp
+	@$(MAKE) mkRbase mklazycomp
Index: src/library/base/R/unix/system.unix.R
===================================================================
--- src/library/base/R/unix/system.unix.R	(revision 85802)
+++ src/library/base/R/unix/system.unix.R	(working copy)
@@ -114,23 +114,14 @@
 Sys.which <- function(names)
 {
     res <- character(length(names)); names(res) <- names
-    ## hopefully configure found [/usr]/bin/which
-    which <- "@WHICH@"
-    if (!nzchar(which)) {
-        warning("'which' was not found on this platform")
-        return(res)
-    }
     for(i in seq_along(names)) {
         if(is.na(names[i])) {res[i] <- NA; next}
         ## Quoting was added in 3.0.0
-        ans <- suppressWarnings(system(paste(which, shQuote(names[i])),
-                                       intern = TRUE, ignore.stderr = TRUE))
-        ## Solaris' which gives 'no foo in ...' message on stdout,
-        ## GNU which does it on stderr
-        if(grepl("solaris", R.version$os)) {
-            tmp <- strsplit(ans[1], " ", fixed = TRUE)[[1]]
-            if(identical(tmp[1:3], c("no", i, "in"))) ans <- ""
-        }
+        ans <- tryCatch(
+            suppressWarnings(system(paste("command -v", shQuote(names[i])),
+                                          intern = TRUE, ignore.stderr = TRUE)),
+            error = \(e) ""
+        )
         res[i] <- if(length(ans)) ans[1] else ""
         ## final check that this is a real path and not an error message
         if(!file.exists(res[i])) res[i] <- ""


-- 
Best regards,
Ivan

[*] example(Sys.which()) works, but there are multiple problems
elsewhere, e.g. tan(1+1000i) returns NaN+1i and Matrix doesn't install
without gmake