From cdb162988a88c7484d89f721176fa21128ca1681 Mon Sep 17 00:00:00 2001 From: Anthony DiGirolamo Date: Sat, 30 Jul 2022 00:01:52 +0000 Subject: [PATCH] pw_build: Python package fixes - Better error messaging when a python package can't be found. - Sort pw_python_package listing in the generated requirements output. - Switch the loop order when merging python packages as part of pw_create_python_source_tree. Before all package build output shared the same /tmp location which lead to problems if directories with the same name exist in multiple python packages. Now each python package gets its own /tmp build dir. - Merge unique options.package_data entries that share the same package name. This prevents merged packages from clobbering eachothers entries. - Add import error handling to pw_build_mcuxpresso. It is often run during gn gen stage when it may not be installed as a Python package. - Remove python_dep on $dir_pw_protobuf_compiler/py for proto ._gen targets. This isn't required for running generate_protos.py and was propagating unnecessary deps. Change-Id: I8b66423e464e1e1f7144841194d73a966b6391d6 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/103480 Reviewed-by: Armando Montanez Commit-Queue: Anthony DiGirolamo Pigweed-Auto-Submit: Anthony DiGirolamo --- pw_build/py/pw_build/create_python_tree.py | 22 ++++++----- .../pw_build/generate_python_requirements.py | 4 +- pw_build/py/pw_build/python_package.py | 37 +++++++++++++++---- .../py/pw_build_mcuxpresso/__main__.py | 6 ++- pw_protobuf_compiler/proto.gni | 18 ++++++++- 5 files changed, 67 insertions(+), 20 deletions(-) diff --git a/pw_build/py/pw_build/create_python_tree.py b/pw_build/py/pw_build/create_python_tree.py index ca641d5c23..a0bba93d6c 100644 --- a/pw_build/py/pw_build/create_python_tree.py +++ b/pw_build/py/pw_build/create_python_tree.py @@ -175,7 +175,13 @@ def update_config_with_packages( # Collect package_data if pkg.config.has_section('options.package_data'): for key, value in pkg.config['options.package_data'].items(): - config['options.package_data'][key] = value + existing_values = config['options.package_data'].get( + key, '').splitlines() + new_value = '\n'.join( + sorted(set(existing_values + value.splitlines()))) + # Remove any empty lines + new_value = new_value.replace('\n\n', '\n') + config['options.package_data'][key] = new_value # Collect entry_points if pkg.config.has_section('options.entry_points'): @@ -257,21 +263,17 @@ def build_python_tree(python_packages: Iterable[PythonPackage], shutil.rmtree(destination_path, ignore_errors=True) destination_path.mkdir(exist_ok=True) - # Define a temporary location to run setup.py build in. - with tempfile.TemporaryDirectory() as build_base_name: - build_base = Path(build_base_name) + for pkg in python_packages: + # Define a temporary location to run setup.py build in. + with tempfile.TemporaryDirectory() as build_base_name: + build_base = Path(build_base_name) - for pkg in python_packages: lib_dir_path = setuptools_build_with_base( pkg, build_base, include_tests=include_tests) # Move installed files from the temp build-base into # destination_path. - for new_file in lib_dir_path.glob('*'): - # Use str(Path) since shutil.move only accepts path-like objects - # in Python 3.9 and up: - # https://docs.python.org/3/library/shutil.html#shutil.move - shutil.move(str(new_file), str(destination_path)) + shutil.copytree(lib_dir_path, destination_path, dirs_exist_ok=True) # Clean build base lib folder for next install shutil.rmtree(lib_dir_path, ignore_errors=True) diff --git a/pw_build/py/pw_build/generate_python_requirements.py b/pw_build/py/pw_build/generate_python_requirements.py index c20766ebc4..3a73f8da6e 100644 --- a/pw_build/py/pw_build/generate_python_requirements.py +++ b/pw_build/py/pw_build/generate_python_requirements.py @@ -111,7 +111,9 @@ def main( '# Auto-generated requirements.txt from the following packages:\n' '#\n') output += '\n'.join('# ' + pkg.gn_target_name - for pkg in target_py_packages) + for pkg in sorted(target_py_packages, + key=lambda pkg: pkg.gn_target_name)) + output += config['options']['install_requires'] output += '\n' requirement.write_text(output) diff --git a/pw_build/py/pw_build/python_package.py b/pw_build/py/pw_build/python_package.py index f3b0aa9d2d..49376fdcd6 100644 --- a/pw_build/py/pw_build/python_package.py +++ b/pw_build/py/pw_build/python_package.py @@ -16,13 +16,17 @@ import configparser from contextlib import contextmanager import copy -from dataclasses import dataclass +from dataclasses import dataclass, asdict +import io import json import os from pathlib import Path +import pprint import re import shutil -from typing import Dict, List, Optional, Iterable +from typing import Any, Dict, List, Optional, Iterable + +_pretty_format = pprint.PrettyPrinter(indent=1, width=120).pformat # List of known environment markers supported by pip. # https://peps.python.org/pep-0508/#environment-markers @@ -132,25 +136,44 @@ def setup_cfg(self) -> Optional[Path]: return None return setup_cfg[0] + def as_dict(self) -> Dict[Any, Any]: + """Return a dict representation of this class.""" + self_dict = asdict(self) + if self.config: + # Expand self.config into text. + setup_cfg_text = io.StringIO() + self.config.write(setup_cfg_text) + self_dict['config'] = setup_cfg_text.getvalue() + return self_dict + @property def package_name(self) -> str: + unknown_package_message = ( + 'Cannot determine the package_name for the Python ' + f'library/package: {self.gn_target_name}\n\n' + 'This could be due to a missing python dependency in GN for:\n' + f'{self.gn_target_name}\n\n') + if self.config: - return self.config['metadata']['name'] + try: + name = self.config['metadata']['name'] + except KeyError: + raise UnknownPythonPackageName(unknown_package_message + + _pretty_format(self.as_dict())) + return name top_level_source_dir = self.top_level_source_dir if top_level_source_dir: return top_level_source_dir.name actual_gn_target_name = self.gn_target_name.split(':') if len(actual_gn_target_name) < 2: - raise UnknownPythonPackageName( - 'Cannot determine the package_name for the Python ' - f'library/package: {self}') + raise UnknownPythonPackageName(unknown_package_message) return actual_gn_target_name[-1] @property def package_dir(self) -> Path: - if self.setup_cfg: + if self.setup_cfg and self.setup_cfg.is_file(): return self.setup_cfg.parent / self.package_name root_source_dir = self.top_level_source_dir if root_source_dir: diff --git a/pw_build_mcuxpresso/py/pw_build_mcuxpresso/__main__.py b/pw_build_mcuxpresso/py/pw_build_mcuxpresso/__main__.py index 1a39e50257..83ad13ba4a 100644 --- a/pw_build_mcuxpresso/py/pw_build_mcuxpresso/__main__.py +++ b/pw_build_mcuxpresso/py/pw_build_mcuxpresso/__main__.py @@ -18,7 +18,11 @@ import pathlib import sys -from pw_build_mcuxpresso import components +try: + from pw_build_mcuxpresso import components +except ImportError: + # Load from this directory if pw_build_mcuxpresso is not available. + import components # type: ignore def _parse_args() -> argparse.Namespace: diff --git a/pw_protobuf_compiler/proto.gni b/pw_protobuf_compiler/proto.gni index d8cc6aaed2..60b7d23df9 100644 --- a/pw_protobuf_compiler/proto.gni +++ b/pw_protobuf_compiler/proto.gni @@ -57,7 +57,23 @@ template("_pw_invoke_protoc") { script = "$dir_pw_protobuf_compiler/py/pw_protobuf_compiler/generate_protos.py" - python_deps = [ "$dir_pw_protobuf_compiler/py" ] + if (pw_build_USE_NEW_PYTHON_BUILD) { + # NOTE: A python_dep on "$dir_pw_protobuf_compiler/py" should not be + # included when using the new Python build. It triggers building that + # Python package which requires the build venv to be created. The venv + # creation will drag in many unnecessary dependencies that may not be + # available when this proto is generated. + python_deps = [] + + # Add pw_protobuf_compiler and its dependencies to the PYTHONPATH when + # running this action. + python_metadata_deps = + [ "$dir_pw_protobuf_compiler/py:py._package_metadata" ] + } else { + python_deps = [ "$dir_pw_protobuf_compiler/py" ] + } + + python_deps = [] if (defined(invoker.python_deps)) { python_deps += invoker.python_deps }