From c0debd03d5e0866ba1ae7d498e1efe94ec3dca87 Mon Sep 17 00:00:00 2001 From: Adam Liddell Date: Sat, 4 May 2019 21:01:44 +0000 Subject: [PATCH 1/8] Make remove_if_present atomic --- compiler/python_archive.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/compiler/python_archive.py b/compiler/python_archive.py index fc810c6..2c69c58 100755 --- a/compiler/python_archive.py +++ b/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): From 20255559f1d1e433d54aca43830ca03de0250482 Mon Sep 17 00:00:00 2001 From: Adam Liddell Date: Sun, 5 May 2019 13:23:46 +0000 Subject: [PATCH 2/8] Fix typo --- compiler/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/cli.py b/compiler/cli.py index 7a34b4e..c506028 100644 --- a/compiler/cli.py +++ b/compiler/cli.py @@ -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', From 7c92596972a65c476941cce08d28a71715fa175e Mon Sep 17 00:00:00 2001 From: Adam Liddell Date: Sun, 5 May 2019 13:24:50 +0000 Subject: [PATCH 3/8] Pass import roots directly from Bazel to compiler via args --- compiler/cli.py | 30 +++++++++++++----------------- subpar.bzl | 11 +++++++---- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/compiler/cli.py b/compiler/cli.py index c506028..82f8f6a 100644 --- a/compiler/cli.py +++ b/compiler/cli.py @@ -87,35 +87,31 @@ 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=[]) 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. - - 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 @@ -130,7 +126,7 @@ def parse_stub(stub_filename): else: interpreter = '/usr/bin/env %s' % interpreter - return (import_roots, interpreter) + return interpreter def main(argv): @@ -138,15 +134,15 @@ def main(argv): 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 Skylark + 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_root, interpreter=interpreter, output_filename=args.outputpar, manifest_filename=args.manifest_file, diff --git a/subpar.bzl b/subpar.bzl index 1b70b5d..5b4d92a 100644 --- a/subpar.bzl +++ b/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 = [ @@ -82,11 +81,15 @@ def _parfile_impl(ctx): "--outputpar", 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], From 2e92c8685292a6ece5e6ebc273b58bc1ef80fd8b Mon Sep 17 00:00:00 2001 From: Adam Liddell Date: Sun, 5 May 2019 13:31:55 +0000 Subject: [PATCH 4/8] Replace default python toolchain wrappers with /usr/bin/env python[23] --- compiler/cli.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/compiler/cli.py b/compiler/cli.py index 82f8f6a..3810ea9 100644 --- a/compiler/cli.py +++ b/compiler/cli.py @@ -115,12 +115,18 @@ def parse_stub(stub_filename): raise error.Error('Failed to parse stub file [%s]' % stub_filename) # Find the Python interpreter, matching the search logic in - # stub_template.txt + # stub_template.txt and checking for default toolchain 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 else: From a09d3810015224976efd559dad25d78abebc29fd Mon Sep 17 00:00:00 2001 From: Adam Liddell Date: Sun, 5 May 2019 13:47:21 +0000 Subject: [PATCH 5/8] Replace Skylark with Starlark --- compiler/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/cli.py b/compiler/cli.py index 3810ea9..1953772 100644 --- a/compiler/cli.py +++ b/compiler/cli.py @@ -140,7 +140,7 @@ def main(argv): parser = make_command_line_parser() args = parser.parse_args(argv[1:]) - # Parse interpreter from stub file that's not available in Skylark + # Parse interpreter from stub file that's not available in Starlark interpreter = parse_stub(args.stub_file) if args.interpreter: From a9d43bd564d5d9fb476a4b668ef56d8907336f54 Mon Sep 17 00:00:00 2001 From: Adam Liddell Date: Sun, 5 May 2019 14:21:25 +0000 Subject: [PATCH 6/8] Update tests for CLI --- compiler/cli_test.py | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/compiler/cli_test.py b/compiler/cli_test.py index a30c067..c9a594d 100644 --- a/compiler/cli_test.py +++ b/compiler/cli_test.py @@ -38,9 +38,17 @@ def test_make_command_line_parser(self): '--outputpar=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.outputpar, 'baz') + self.assertEqual(args.stub_file, 'quux') + self.assertEqual(args.zip_safe, False) + self.assertEqual(args.import_root, ['root1', 'root2']) + self.assertEqual(args.main_filename, 'foo') def test_make_command_line_parser_for_interprerter(self): parser = cli.make_command_line_parser() @@ -57,36 +65,31 @@ 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 - [b""" - python_imports = 'myworkspace/spam/eggs:otherworkspace' -PYTHON_BINARY = '/usr/bin/python' -""", - (['myworkspace/spam/eggs', 'otherworkspace'], '/usr/bin/python')], + '/usr/bin/python'], # Relative path to interpreter [b""" - python_imports = '' PYTHON_BINARY = 'mydir/python' """, - ([], 'mydir/python')], + 'mydir/python'], # Search for interpreter on $PATH [b""" - python_imports = '' PYTHON_BINARY = 'python' """, - ([], '/usr/bin/env python')], + '/usr/bin/env python'], + # Default toolchain wrapped python3 interpreter + [b""" +PYTHON_BINARY = 'bazel_tools/tools/python/py3wrapper.sh' +""", + '/usr/bin/env python3'], + # Default toolchain wrapped python2 interpreter + [b""" +PYTHON_BINARY = 'bazel_tools/tools/python/py2wrapper.sh' +""", + '/usr/bin/env python2'], ] for content, expected in valid_cases: with test_utils.temp_file(content) as stub_file: @@ -98,11 +101,8 @@ def test_stub(self): b'\n\n', # No interpreter b" python_imports = 'myworkspace/spam/eggs'", - # No imports - b"PYTHON_BINARY = 'python'\n", # Interpreter is label b""" - python_imports = '' PYTHON_BINARY = '//mypackage:python' """, ] From 44ba346f17760fda4bf230857735ef6a511c149a Mon Sep 17 00:00:00 2001 From: Adam Liddell Date: Sun, 5 May 2019 14:28:20 +0000 Subject: [PATCH 7/8] Update CLI args for naming consistency --- compiler/cli.py | 9 +++++---- compiler/cli_test.py | 8 ++++---- subpar.bzl | 2 +- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/compiler/cli.py b/compiler/cli.py index 1953772..1a9ab51 100644 --- a/compiler/cli.py +++ b/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( @@ -91,7 +91,8 @@ def make_command_line_parser(): '--import_root', help='Path to add to sys.path, may be repeated to provide multiple roots.', action='append', - default=[]) + default=[], + dest='import_roots') return parser @@ -148,9 +149,9 @@ def main(argv): par = python_archive.PythonArchive( main_filename=args.main_filename, - import_roots=args.import_root, + 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, diff --git a/compiler/cli_test.py b/compiler/cli_test.py index c9a594d..9d8bcc8 100644 --- a/compiler/cli_test.py +++ b/compiler/cli_test.py @@ -35,7 +35,7 @@ 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', @@ -44,10 +44,10 @@ def test_make_command_line_parser(self): ]) self.assertEqual(args.manifest_file, 'bar') self.assertEqual(args.manifest_root, 'bazz') - self.assertEqual(args.outputpar, 'baz') + self.assertEqual(args.output_par, 'baz') self.assertEqual(args.stub_file, 'quux') self.assertEqual(args.zip_safe, False) - self.assertEqual(args.import_root, ['root1', 'root2']) + self.assertEqual(args.import_roots, ['root1', 'root2']) self.assertEqual(args.main_filename, 'foo') def test_make_command_line_parser_for_interprerter(self): @@ -55,7 +55,7 @@ def test_make_command_line_parser_for_interprerter(self): args = parser.parse_args([ '--manifest_file=bar', '--manifest_root=bazz', - '--outputpar=baz', + '--output_par=baz', '--stub_file=quux', '--zip_safe=False', '--interpreter=foobar', diff --git a/subpar.bzl b/subpar.bzl index 5b4d92a..ae685bf 100644 --- a/subpar.bzl +++ b/subpar.bzl @@ -78,7 +78,7 @@ def _parfile_impl(ctx): args = ctx.attr.compiler_args + [ "--manifest_file", sources_file.path, - "--outputpar", + "--output_par", ctx.outputs.executable.path, "--stub_file", ctx.attr.src.files_to_run.executable.path, From 76b40f126c9f9fe890b818c89d1acd7748a52dd3 Mon Sep 17 00:00:00 2001 From: Adam Liddell Date: Tue, 7 May 2019 16:42:34 +0000 Subject: [PATCH 8/8] Fail when python interpreter is a relative path and improve comments --- compiler/cli.py | 20 +++++++++++++++++--- compiler/cli_test.py | 10 +++------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/compiler/cli.py b/compiler/cli.py index 1a9ab51..d1fcb59 100644 --- a/compiler/cli.py +++ b/compiler/cli.py @@ -101,6 +101,8 @@ def parse_stub(stub_filename): We assume the stub is utf-8 encoded. + TODO(bazelbuild/bazel#7805): Remove this once we can access the py_runtime from Starlark. + Returns path to Python interpreter """ @@ -115,8 +117,17 @@ def parse_stub(stub_filename): 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 and checking for default toolchain + # 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) @@ -129,7 +140,10 @@ def parse_stub(stub_filename): # 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 diff --git a/compiler/cli_test.py b/compiler/cli_test.py index 9d8bcc8..4bdaf34 100644 --- a/compiler/cli_test.py +++ b/compiler/cli_test.py @@ -70,11 +70,6 @@ def test_stub(self): PYTHON_BINARY = '/usr/bin/python' """, '/usr/bin/python'], - # Relative path to interpreter - [b""" -PYTHON_BINARY = 'mydir/python' -""", - 'mydir/python'], # Search for interpreter on $PATH [b""" PYTHON_BINARY = 'python' @@ -97,10 +92,11 @@ def test_stub(self): self.assertEqual(actual, expected) invalid_cases = [ + # No interpreter b'', b'\n\n', - # No interpreter - b" python_imports = 'myworkspace/spam/eggs'", + # Relative interpreter path + b"PYTHON_BINARY = 'mydir/python'", # Interpreter is label b""" PYTHON_BINARY = '//mypackage:python'