Fix incompatibility with Python Toolchains

compiler/cli.py

@@ -53,7 +53,7 @@ def make_command_line_parser():
         help='Root directory of all relative paths in manifest file.',
         default=os.getcwd())
     parser.add_argument(
-        '--outputpar',
+        '--output_par',
         help='Filename of generated par file.',
         required=True)
     parser.add_argument(
@@ -69,7 +69,7 @@ def make_command_line_parser():
     # "Seconds since Unix epoch" was chosen to be compatible with
     # the SOURCE_DATE_EPOCH standard
     #
-    # Numeric calue is from running this:
+    # Numeric value is from running this:
     #   "date --date='Jan 1 1980 00:00:00 utc' --utc +%s"
     parser.add_argument(
         '--timestamp',
@@ -87,68 +87,85 @@ def make_command_line_parser():
         'directory at the start of execution.',
         type=bool_from_string,
         required=True)
+    parser.add_argument(
+        '--import_root',
+        help='Path to add to sys.path, may be repeated to provide multiple roots.',
+        action='append',
+        default=[],
+        dest='import_roots')
     return parser
 
 
 def parse_stub(stub_filename):
-    """Parse the imports and interpreter path from a py_binary() stub.
+    """Parse interpreter path from a py_binary() stub.
 
     We assume the stub is utf-8 encoded.
 
-    TODO(b/29227737): Remove this once we can access imports from skylark.
+    TODO(bazelbuild/bazel#7805): Remove this once we can access the py_runtime from Starlark.
 
-    Returns (list of relative paths, path to Python interpreter)
+    Returns path to Python interpreter
     """
 
-    # Find the list of import roots
-    imports_regex = re.compile(r'''^  python_imports = '([^']*)'$''')
+    # Find the interpreter
     interpreter_regex = re.compile(r'''^PYTHON_BINARY = '([^']*)'$''')
-    import_roots = None
     interpreter = None
     with io.open(stub_filename, 'rt', encoding='utf8') as stub_file:
         for line in stub_file:
-            importers_match = imports_regex.match(line)
-            if importers_match:
-                import_roots = importers_match.group(1).split(':')
-                # Filter out empty paths
-                import_roots = [x for x in import_roots if x]
             interpreter_match = interpreter_regex.match(line)
             if interpreter_match:
                 interpreter = interpreter_match.group(1)
-    if import_roots is None or not interpreter:
+    if not interpreter:
         raise error.Error('Failed to parse stub file [%s]' % stub_filename)
 
-    # Find the Python interpreter, matching the search logic in
-    # stub_template.txt
+    # Determine the Python interpreter, checking for default toolchain.
+    #
+    # This somewhat mirrors the logic in python_stub_template.txt, but we don't support
+    # relative paths (i.e., in-workspace interpreters). This is because the interpreter
+    # will be used in the .par file's shebang, and putting a relative path in a shebang
+    # is extremely brittle and non-relocatable. (The reason the standard py_binary rule
+    # can use an in-workspace interpreter is that its stub script runs in a separate
+    # process and has a shebang referencing the system interpreter). As a special case,
+    # if the Python target is using the autodetecting Python toolchain, which is
+    # technically an in-workspace runtime, we rewrite it to "/usr/bin/env python[2|3]"
+    # rather than fail.
     if interpreter.startswith('//'):
         raise error.Error('Python interpreter must not be a label [%s]' %
                           stub_filename)
     elif interpreter.startswith('/'):
         pass
+    elif interpreter == 'bazel_tools/tools/python/py3wrapper.sh': # Default toolchain
+        # Replace default toolchain python3 wrapper with default python3 on path
+        interpreter = '/usr/bin/env python3'
+    elif interpreter == 'bazel_tools/tools/python/py2wrapper.sh': # Default toolchain
+        # Replace default toolchain python2 wrapper with default python2 on path
+        interpreter = '/usr/bin/env python2'
     elif '/' in interpreter:
-        pass
+        raise error.Error(
+            'par files require a Python runtime that is ' +
+            'installed on the system, not defined inside the workspace. Use ' +
+            'a `py_runtime` with an absolute path, not a label.')
     else:
         interpreter = '/usr/bin/env %s' % interpreter
 
-    return (import_roots, interpreter)
+    return interpreter
 
 
 def main(argv):
     """Command line interface to Subpar"""
     parser = make_command_line_parser()
     args = parser.parse_args(argv[1:])
 
-    # Parse information from stub file that's too hard to compute in Skylark
-    import_roots, interpreter = parse_stub(args.stub_file)
+    # Parse interpreter from stub file that's not available in Starlark
+    interpreter = parse_stub(args.stub_file)
 
     if args.interpreter:
         interpreter = args.interpreter
 
     par = python_archive.PythonArchive(
         main_filename=args.main_filename,
-        import_roots=import_roots,
+        import_roots=args.import_roots,
         interpreter=interpreter,
-        output_filename=args.outputpar,
+        output_filename=args.output_par,
         manifest_filename=args.manifest_file,
         manifest_root=args.manifest_root,
         timestamp=args.timestamp,

compiler/cli_test.py

@@ -35,19 +35,27 @@ def test_make_command_line_parser(self):
         args = parser.parse_args([
             '--manifest_file=bar',
             '--manifest_root=bazz',
-            '--outputpar=baz',
+            '--output_par=baz',
             '--stub_file=quux',
             '--zip_safe=False',
+            '--import_root=root1',
+            '--import_root=root2',
             'foo',
         ])
         self.assertEqual(args.manifest_file, 'bar')
+        self.assertEqual(args.manifest_root, 'bazz')
+        self.assertEqual(args.output_par, 'baz')
+        self.assertEqual(args.stub_file, 'quux')
+        self.assertEqual(args.zip_safe, False)
+        self.assertEqual(args.import_roots, ['root1', 'root2'])
+        self.assertEqual(args.main_filename, 'foo')
 
     def test_make_command_line_parser_for_interprerter(self):
         parser = cli.make_command_line_parser()
         args = parser.parse_args([
             '--manifest_file=bar',
             '--manifest_root=bazz',
-            '--outputpar=baz',
+            '--output_par=baz',
             '--stub_file=quux',
             '--zip_safe=False',
             '--interpreter=foobar',
@@ -57,52 +65,40 @@ def test_make_command_line_parser_for_interprerter(self):
 
     def test_stub(self):
         valid_cases = [
-            # Empty list
+            # Absolute path to interpreter
             [b"""
-  python_imports = ''
 PYTHON_BINARY = '/usr/bin/python'
 """,
-             ([], '/usr/bin/python')],
-            # Single import
-            [b"""
-  python_imports = 'myworkspace/spam/eggs'
-PYTHON_BINARY = '/usr/bin/python'
-""",
-             (['myworkspace/spam/eggs'], '/usr/bin/python')],
-            # Multiple imports
+             '/usr/bin/python'],
+            # Search for interpreter on $PATH
             [b"""
-  python_imports = 'myworkspace/spam/eggs:otherworkspace'
-PYTHON_BINARY = '/usr/bin/python'
+PYTHON_BINARY = 'python'
 """,
-             (['myworkspace/spam/eggs', 'otherworkspace'], '/usr/bin/python')],
-            # Relative path to interpreter
+             '/usr/bin/env python'],
+            # Default toolchain wrapped python3 interpreter
             [b"""
-  python_imports = ''
-PYTHON_BINARY = 'mydir/python'
+PYTHON_BINARY = 'bazel_tools/tools/python/py3wrapper.sh'
 """,
-             ([], 'mydir/python')],
-            # Search for interpreter on $PATH
+             '/usr/bin/env python3'],
+            # Default toolchain wrapped python2 interpreter
             [b"""
-  python_imports = ''
-PYTHON_BINARY = 'python'
+PYTHON_BINARY = 'bazel_tools/tools/python/py2wrapper.sh'
 """,
-             ([], '/usr/bin/env python')],
+             '/usr/bin/env python2'],
         ]
         for content, expected in valid_cases:
             with test_utils.temp_file(content) as stub_file:
                 actual = cli.parse_stub(stub_file.name)
                 self.assertEqual(actual, expected)
 
         invalid_cases = [
+            # No interpreter
             b'',
             b'\n\n',
-            # No interpreter
-            b"  python_imports = 'myworkspace/spam/eggs'",
-            # No imports
-            b"PYTHON_BINARY = 'python'\n",
+            # Relative interpreter path
+            b"PYTHON_BINARY = 'mydir/python'",
             # Interpreter is label
             b"""
-  python_imports = ''
 PYTHON_BINARY = '//mypackage:python'
 """,
             ]

compiler/python_archive.py

@@ -30,6 +30,7 @@
 
 from datetime import datetime
 import contextlib
+import errno
 import io
 import logging
 import os
@@ -290,9 +291,14 @@ def create_final_from_temp(self, temp_parfile_name):
 
 
 def remove_if_present(filename):
-    """Delete any existing file"""
-    if os.path.exists(filename):
+    """Delete a file if it exists"""
+    try:
+        # Remove atomically
         os.remove(filename)
+    except OSError as exc:
+        # Ignore failure if file does not exist
+        if exc.errno != errno.ENOENT:
+            raise
 
 
 def fetch_support_file(name, timestamp_tuple):

subpar.bzl

@@ -64,8 +64,7 @@ def _parfile_impl(ctx):
     )
 
     # Find the list of directories to add to sys.path
-    # TODO(b/29227737): Use 'imports' provider from Bazel
-    stub_file = ctx.attr.src.files_to_run.executable.path
+    import_roots = ctx.attr.src[PyInfo].imports.to_list()
 
     # Inputs to the action, but don't actually get stored in the .par file
     extra_inputs = [
@@ -79,14 +78,18 @@ def _parfile_impl(ctx):
     args = ctx.attr.compiler_args + [
         "--manifest_file",
         sources_file.path,
-        "--outputpar",
+        "--output_par",
         ctx.outputs.executable.path,
         "--stub_file",
-        stub_file,
+        ctx.attr.src.files_to_run.executable.path,
         "--zip_safe",
         str(zip_safe),
-        main_py_file.path,
     ]
+    for import_root in import_roots:
+        args.extend(['--import_root', import_root])
+    args.append(main_py_file.path)
+
+    # Run compiler
     ctx.actions.run(
         inputs = inputs + extra_inputs,
         tools = [ctx.attr.src.files_to_run.executable],