Make InterpreterOptions configuration option a list (#259)

There are problems associated with `InterpreterOptions` being a string,
chiefly that it isn't safe to assume where individual Python interpreter
command line arguments begin and end simply by splitting the string on
space characters. This isn't so much of a concern while the value of
`InterpreterOptions` is just appended to the main .pex shebang in
please_pex, but it'll become more of a concern when we instead rely on a
native-code .pex preamble to invoke the interpreter because we'll be
passing arguments variadically to `execvp(3)`.

Make `InterpreterOptions` a repeatable configuration option and pass
each value to please_pex as a separate `--interpreter_options` command
line option (which itself also becomes repeatable). This isn't a
breaking change because the values passed via `--interpreter_options`
are for now ultimately just being stringified via a space-delimited join
and appended to the .pex shebang, but the distinction will become
meaningful when we implement a native-code .pex preamble.

Author: chrisnovakovic

.plzconfig

@@ -42,8 +42,9 @@ Help = A path or build label for the pex tool, which is used to create .pex file
 
 [PluginConfig "interpreter_options"]
 ConfigKey = InterpreterOptions
-DefaultValue = ""
-Help = Any additional flags to pass to the python interpreter
+Optional = true
+Repeatable = true
+Help = Additional command line arguments to pass to the Python interpreter.
 
 [PluginConfig "test_runner"]
 ConfigKey = TestRunner

README.md

@@ -62,7 +62,8 @@ The available configuration options are:
 
 ```ini
 [Plugin "python"]
-InterpreterOptions = -b -s
+InterpreterOptions = -b
+InterpreterOptions = -s
 DefaultInterpreter = python3
 PexTool = //tools/please_pex
 TestRunner = unittest

build_defs/python.build_defs

@@ -147,8 +147,9 @@ def python_binary(name:str, main:str, srcs:list=[], resources:list=[], out:str=N
 
     shebang = shebang or _interpreter_cmd(interpreter)
     zipsafe_flag = '' if zip_safe is False else '--zip_safe'
-    cmd = '$TOOLS_PEX -s "%s" -m "%s" %s --interpreter_options="%s"' % (
-        shebang, module_dir, zipsafe_flag, CONFIG.PYTHON.INTERPRETER_OPTIONS)
+    interpreter_opts = _interpreter_opts(CONFIG.PYTHON.INTERPRETER_OPTIONS)
+    cmd = '$TOOLS_PEX -s "%s" -m "%s" %s %s' % (
+        shebang, module_dir, zipsafe_flag, interpreter_opts)
 
     # If content hashing feature flag is enabled, we use the hash of the built
     # dependencies instead of the RULE_HASH as the base of the pex extraction
@@ -270,7 +271,8 @@ def python_test(name:str, srcs:list, data:list|dict=[], resources:list=[], deps:
     """
     test_runner = test_runner or CONFIG.PYTHON.TEST_RUNNER
     shebang = shebang or _interpreter_cmd(interpreter)
-    cmd = f'$TOOLS_PEX -t -s "{shebang}" -m "{module_dir}" -r "{test_runner}" --zip_safe --add_test_runner_deps --interpreter_options="{CONFIG.PYTHON.INTERPRETER_OPTIONS}" --stamp="$RULE_HASH"'
+    interpreter_opts = _interpreter_opts(CONFIG.PYTHON.INTERPRETER_OPTIONS)
+    cmd = f'$TOOLS_PEX -t -s "{shebang}" -m "{module_dir}" -r "{test_runner}" --zip_safe --add_test_runner_deps {interpreter_opts} --stamp="$RULE_HASH"'
     if site:
         cmd += ' -S'
 
@@ -746,6 +748,10 @@ def _interpreter_cmd(interpreter:str):
     return f'$(out {interpreter})' if looks_like_build_label(interpreter) else interpreter
 
 
+def _interpreter_opts(opts:list):
+    return " ".join([f'--interpreter_options="{o}"' for o in opts])
+
+
 def _patch_cmd(patch):
     """Returns a command for the given patch or patches, with OS-specific tweaks where needed."""
     patches = [patch] if isinstance(patch, str) else patch

tools/please_pex/pex/pex.go

@@ -47,7 +47,7 @@ type Writer struct {
 }
 
 // NewWriter constructs a new Writer.
-func NewWriter(entryPoint, interpreter, options, stamp string, zipSafe, noSite bool) *Writer {
+func NewWriter(entryPoint, interpreter string, options []string, stamp string, zipSafe, noSite bool) *Writer {
 	pw := &Writer{
 		zipSafe:        zipSafe,
 		noSite:         noSite,
@@ -59,8 +59,8 @@ func NewWriter(entryPoint, interpreter, options, stamp string, zipSafe, noSite b
 }
 
 // SetShebang sets the leading shebang that will be written to the file.
-func (pw *Writer) SetShebang(shebang string, options string) {
-	shebang = strings.TrimSpace(fmt.Sprintf("%s %s", shebang, options))
+func (pw *Writer) SetShebang(shebang string, options []string) {
+	shebang = strings.TrimSpace(fmt.Sprintf("%s %s", shebang, strings.Join(options, " ")))
 	if !path.IsAbs(shebang) {
 		shebang = "/usr/bin/env " + shebang
 	}

tools/please_pex/pex_main.go

@@ -22,7 +22,7 @@ var opts = struct {
 	TestRunner         string        `short:"r" long:"test_runner" default:"unittest" description:"Test runner to use"`
 	Shebang            string        `short:"s" long:"shebang" description:"Explicitly set shebang to this"`
 	Stamp              string        `long:"stamp" description:"Unique value used to derive cache directory for pex"`
-	InterpreterOptions string        `long:"interpreter_options" description:"Options-string to pass to the python interpreter"`
+	InterpreterOptions []string      `long:"interpreter_options" description:"Additional command line arguments to pass to the Python interpreter"`
 	Test               bool          `short:"t" long:"test" description:"True if we're to build a test"`
 	Debug              pex.Debugger  `short:"d" long:"debug" optional:"true" optional-value:"pdb" choice:"pdb" choice:"debugpy" description:"Debugger to generate a debugging pex"`
 	Site               bool          `short:"S" long:"site" description:"Allow the pex to import site at startup"`