Merge pull request #6597 from grondo/issue#6594

do not export `PYTHONPATH` by default in Flux commands

Author: mergify[bot]

doc/man1/flux.rst

@@ -80,9 +80,6 @@ is provided below:
    * - :envvar:`LUA_CPATH`
      - Lua binary module search path
 
-   * - :envvar:`PYTHONPATH`
-     - Python module search path:
-
 
 RESOURCES
 =========

doc/man7/flux-environment.rst

@@ -552,11 +552,16 @@ of compiled-in install paths and the environment.
 .. envvar:: PYTHONPATH
             FLUX_PYTHONPATH_PREPEND
 
-   :envvar:`PYTHONPATH` is set so that sub-commands can find required Python
-   libraries:
+   With the :command:`flux python` and :man1:`flux-env` subcommands,
+   :envvar:`PYTHONPATH` is set such that the correct version of
+   Python modules can be found. In these cases the path is set
+   to the following:
 
       $FLUX_PYTHONPATH_PREPEND : install-path : $PYTHONPATH
 
+   The :man1:`flux` command does not otherwise modify :envvar:`PYTHONPATH`
+   unless :envvar:`FLUX_PYTHONPATH_PREPEND` is set.
+
    Values may include multiple directories separated by colons.
 
 .. note::

doc/python/basics.rst

@@ -5,14 +5,10 @@ Flux Python Basics
 Importing the ``flux`` Python package
 -------------------------------------
 
-.. note:: The ``flux`` package which is used to interact with Flux *cannot* be
-   installed into a virtual environment with pip or conda.
+.. note:: The ``flux`` package may now be installed via pip using the
+   ``flux-python`` package.
 
 Flux's Python bindings are available with any installation of Flux.
-When running in a Flux instance, Flux will export the
-`PYTHONPATH <https://docs.python.org/3/using/cmdline.html#envvar-PYTHONPATH>`_
-environment variable so that Python processes can import the flux package
-by the usual import mechanism (``import flux``).
 
 If you want to import the package from outside of a Flux instance,
 running ``/path/to/flux env | grep PYTHONPATH`` in your shell will show you

src/cmd/builtin.h

@@ -12,6 +12,7 @@
 #include <flux/optparse.h>
 #include "src/common/libutil/xzmalloc.h"
 #include "src/common/libutil/log.h"
+#include "src/common/libutil/environment.h"
 
 #ifndef HAVE_CMD_BUILTIN_H
 #define HAVE_CMD_BUILTIN_H 1
@@ -24,6 +25,7 @@ struct builtin_cmd {
 };
 
 void usage (optparse_t *p);
+void builtin_env_add_pythonpath (struct environment *env);
 flux_t *builtin_get_flux_handle (optparse_t *p);
 
 #endif /* !HAVE_CMD_BUILTIN_H */

src/cmd/builtin/env.c

@@ -16,9 +16,20 @@
 #include "builtin.h"
 #include "src/common/libutil/environment.h"
 
+static void set_pythonpath (void)
+{
+    struct environment *env;
+    if (!(env = environment_create ()))
+        log_err_exit ("error creating environment");
+    builtin_env_add_pythonpath (env);
+    environment_apply (env);
+    environment_destroy (env);
+}
+
 static void print_environment (struct environment *env)
 {
     const char *val;
+
     for (val = environment_first (env); val; val = environment_next (env))
         printf("export %s=\"%s\"\n", environment_cursor (env), val);
     fflush(stdout);
@@ -28,12 +39,15 @@ static int cmd_env (optparse_t *p, int ac, char *av[])
 {
     int n = optparse_option_index (p);
     if (av && av[n]) {
+        set_pythonpath ();
         execvp (av[n], av+n); /* no return if successful */
         log_err_exit ("execvp (%s)", av[n]);
     } else {
         struct environment *env = optparse_get_data (p, "env");
+
         if (env == NULL)
             log_msg_exit ("flux-env: failed to get flux environment!");
+        builtin_env_add_pythonpath (env);
         print_environment (env);
     }
     return (0);

src/cmd/builtin/python.c

@@ -14,9 +14,19 @@
 #include <unistd.h>
 
 #include "builtin.h"
-#include "config.h"
 #include "src/common/libutil/environment.h"
 
+static void prepare_environment (void)
+{
+    struct environment *env;
+
+    if (!(env = environment_create ()))
+        log_err_exit ("error creating environment");
+    builtin_env_add_pythonpath (env);
+    environment_apply (env);
+    environment_destroy (env);
+}
+
 static int cmd_python (optparse_t *p, int ac, char *av[])
 {
     /*
@@ -26,6 +36,9 @@ static int cmd_python (optparse_t *p, int ac, char *av[])
      *  that symlink'd binaries in virtualenvs are respected.
      */
     av[0] = PYTHON_INTERPRETER;
+
+    prepare_environment ();
+
     execv (PYTHON_INTERPRETER, av); /* no return if successful */
     log_err_exit ("execvp (%s)", PYTHON_INTERPRETER);
     return (0);

src/cmd/flux.c

@@ -184,12 +184,10 @@ int main (int argc, char *argv[])
                       flux_conf_builtin_get ("lua_path_add", flags));
     environment_push (env, "LUA_PATH", getenv ("FLUX_LUA_PATH_PREPEND"));
 
-    environment_from_env (env, "PYTHONPATH", "", ':');
-    environment_push (env,
-                      "PYTHONPATH",
-                      flux_conf_builtin_get ("python_path", flags));
-    environment_push (env, "PYTHONPATH", getenv ("FLUX_PYTHONPATH_PREPEND"));
-
+    if ((s = getenv ("FLUX_PYTHONPATH_PREPEND"))) {
+        environment_from_env (env, "PYTHONPATH", "", ':');
+        environment_push (env, "PYTHONPATH", s);
+    }
     if ((s = getenv ("MANPATH")) && strlen (s) > 0) {
         environment_from_env (env, "MANPATH", ":", ':');
         environment_push (env,
@@ -333,6 +331,37 @@ void setup_path (struct environment *env, const char *argv0)
         log_msg_exit ("Unable to determine flux executable dir");
 }
 
+void builtin_env_add_pythonpath (struct environment *env)
+{
+    /* prepend to PYTHONPATH, which is no longer done by default:
+     */
+    environment_from_env (env, "PYTHONPATH", "", ':');
+    environment_push (env,
+                      "PYTHONPATH",
+                      flux_conf_builtin_get ("python_path", FLUX_CONF_AUTO));
+    environment_push (env, "PYTHONPATH", getenv ("FLUX_PYTHONPATH_PREPEND"));
+}
+
+static void setup_python_wrapper_environment (void)
+{
+    struct environment *env;
+    const char *val;
+
+    /* Set FLUX_PYTHONPATH_ORIG to the current PYTHONPATH, then
+     * prepend the builtin python_path to PYTHONPATH so the the
+     * python wrapper py-runner.py can find the correct Flux
+     * bindings. The wrapper will then reset PYTHONPATH to
+     * FLUX_PYTHONPATH_ORIG to avoid polluting the user environment.
+     */
+    if (!(env = environment_create ()))
+        log_err_exit ("error creating environment");
+    if ((val = getenv ("PYTHONPATH")))
+        environment_set (env, "FLUX_PYTHONPATH_ORIG", val, ':');
+    builtin_env_add_pythonpath (env);
+    environment_apply (env);
+    environment_destroy (env);
+}
+
 /* Check for a flux-<command>.py in dir and execute it under the configured
  * PYTHON_INTERPRETER if found.
  */
@@ -361,6 +390,9 @@ void exec_subcommand_py (bool vopt,
                      PYTHON_INTERPRETER,
                      wrapper,
                      path);
+
+        setup_python_wrapper_environment ();
+
         execvp (PYTHON_INTERPRETER, av);
     }
     free (path);

src/cmd/py-runner.py

@@ -84,13 +84,27 @@ def prepend_standard_python_paths(patharray):
     sys.path[0:0] = prepend_paths
 
 
+def restore_pythonpath():
+    """
+    Restore PYTHONPATH from the original caller's environment using
+    FLUX_PYTHONPATH_ORIG as set by flux(1)
+    """
+    try:
+        val = os.environ["FLUX_PYTHONPATH_ORIG"]
+        os.environ["PYTHONPATH"] = val
+    except KeyError:
+        # If FLUX_PYTHONPATH_ORIG not set, then unset PYTHONPATH:
+        del os.environ["PYTHONPATH"]
+
+
 if __name__ == "__main__":
     #  Pop first argument which is this script, modify sys.path as noted
     #  above, then invoke target script in this interpreter using
     #  runpy.run_path():
     #
     sys.argv.pop(0)
     prepend_standard_python_paths(sys.path)
+    restore_pythonpath()
     runpy.run_path(sys.argv[0], run_name="__main__")
 
 

t/sharness.d/01-setup.sh

@@ -74,6 +74,15 @@ if ! test -x ${fluxbin}; then
     return 1
 fi
 
+#  flux(1) prepends the correct path(s) to PYTHONPATH when invoking
+#   Python subcommands or when `flux env` and `flux python` are invoked.
+#   However, this is not always the case for Python based test scripts
+#   run under sharness, so these test scripts can end up loading system
+#   installed Flux modules and fail in unpredictable ways. Therefore,
+#   export the correct PYTHONPATH here:
+#
+PYTHONPATH="$($FLUX_BUILD_DIR/src/cmd/flux env printenv PYTHONPATH)"
+
 #  Python's site module won't be able to determine the correct path
 #   for site.USER_SITE because sharness reassigns HOME to a per-test
 #   trash directory. Set up a REAL_HOME here from the passwd database

t/t1102-cmddriver.t

@@ -57,6 +57,30 @@ test_expect_success 'flux env passes cmddriver option to argument' "
 	(FLUX_URI=foo://xyx \
 		flux env sh -c 'echo \$FLUX_URI' | grep '^foo://xyx$')
 "
+test_expect_success 'flux env prepends to PYTHONPATH' '
+	expected=$(flux config builtin python_path | sed "s/:.*//") &&
+	result=$(PYTHONPATH= flux env sh -c "echo \$PYTHONPATH") &&
+	test_debug "echo expecting PYTHONPATH=$expected got $result" &&
+	echo "$result" | grep $expected
+'
+test_expect_success 'flux env outputs PYTHONPATH' '
+	expected=$(flux config builtin python_path | sed "s/:.*//") &&
+	result=$(PYTHONPATH= flux env | grep PYTHONPATH) &&
+	test_debug "echo expecting PYTHONPATH=$expected got $result" &&
+	echo "$result" | grep $expected
+'
+test_expect_success 'flux python prepends to PYTHONPATH' '
+	expected=$(flux config builtin python_path | sed "s/:.*//") &&
+	test_debug "echo expecting PYTHONPATH=$expected" &&
+	PYTHONPATH= \
+		flux python -c "import os; print(os.environ[\"PYTHONPATH\"])" \
+		| grep $expected
+'
+test_expect_success 'flux does not prepend to PYTHONPATH' '
+	printenv=$(which printenv) &&
+	( unset PYTHONPATH &&
+	  test_must_fail flux $printenv PYTHONPATH)
+'
 # push /foo twice onto PYTHONPATH -- ensure it is leftmost position:
 #test_expect_success 'cmddriver pushes dup path elements onto front of PATH' "
 #	flux -P /foo env flux -P /bar env flux -P /foo env \