From 90baa6bcf953ce2f34e517d7cd6c9a0ab4ee6304 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Sun, 21 Jul 2024 21:45:13 -0700 Subject: [PATCH] refactor: use logger in repo_utils.execute_* functions (#2082) This is to unify how it handles printing log messages. This also updates repo rules using repo_utils to create loggers and set the `_rule_name` attribute for the logger to use. * Also removes defunct repo_utils.debug_print * Also makes some functions private that aren't used elsewhere --- python/private/local_runtime_repo.bzl | 1 + python/private/pypi/whl_library.bzl | 15 +++++++++----- python/private/repo_utils.bzl | 29 ++++++++++----------------- python/private/toolchains_repo.bzl | 18 +++++++++++------ python/repositories.bzl | 12 ++++++++++- 5 files changed, 45 insertions(+), 30 deletions(-) diff --git a/python/private/local_runtime_repo.bzl b/python/private/local_runtime_repo.bzl index a206e6d7dd..4e7edde9d8 100644 --- a/python/private/local_runtime_repo.bzl +++ b/python/private/local_runtime_repo.bzl @@ -69,6 +69,7 @@ def _local_runtime_repo_impl(rctx): rctx.path(rctx.attr._get_local_runtime_info), ], quiet = True, + logger = logger, ) if exec_result.return_code != 0: if on_failure == "fail": diff --git a/python/private/pypi/whl_library.bzl b/python/private/pypi/whl_library.bzl index 0419926c7a..c4e1f76897 100644 --- a/python/private/pypi/whl_library.bzl +++ b/python/private/pypi/whl_library.bzl @@ -60,7 +60,7 @@ def _get_xcode_location_cflags(rctx): "-isysroot {}/SDKs/MacOSX.sdk".format(xcode_root), ] -def _get_toolchain_unix_cflags(rctx, python_interpreter): +def _get_toolchain_unix_cflags(rctx, python_interpreter, logger = None): """Gather cflags from a standalone toolchain for unix systems. Pip won't be able to compile c extensions from sdists with the pre built python distributions from indygreg @@ -72,7 +72,7 @@ def _get_toolchain_unix_cflags(rctx, python_interpreter): return [] # Only update the location when using a standalone toolchain. - if not is_standalone_interpreter(rctx, python_interpreter): + if not is_standalone_interpreter(rctx, python_interpreter, logger = logger): return [] stdout = repo_utils.execute_checked_stdout( @@ -147,12 +147,13 @@ def _parse_optional_attrs(rctx, args, extra_pip_args = None): return args -def _create_repository_execution_environment(rctx, python_interpreter): +def _create_repository_execution_environment(rctx, python_interpreter, logger = None): """Create a environment dictionary for processes we spawn with rctx.execute. Args: rctx (repository_ctx): The repository context. python_interpreter (path): The resolved python interpreter. + logger: Optional logger to use for operations. Returns: Dictionary of environment variable suitable to pass to rctx.execute. """ @@ -160,7 +161,7 @@ def _create_repository_execution_environment(rctx, python_interpreter): # Gather any available CPPFLAGS values cppflags = [] cppflags.extend(_get_xcode_location_cflags(rctx)) - cppflags.extend(_get_toolchain_unix_cflags(rctx, python_interpreter)) + cppflags.extend(_get_toolchain_unix_cflags(rctx, python_interpreter, logger = logger)) env = { "PYTHONPATH": pypi_repo_utils.construct_pythonpath( @@ -173,6 +174,7 @@ def _create_repository_execution_environment(rctx, python_interpreter): return env def _whl_library_impl(rctx): + logger = repo_utils.logger(rctx) python_interpreter = pypi_repo_utils.resolve_python_interpreter( rctx, python_interpreter = rctx.attr.python_interpreter, @@ -189,7 +191,7 @@ def _whl_library_impl(rctx): extra_pip_args.extend(rctx.attr.extra_pip_args) # Manually construct the PYTHONPATH since we cannot use the toolchain here - environment = _create_repository_execution_environment(rctx, python_interpreter) + environment = _create_repository_execution_environment(rctx, python_interpreter, logger = logger) whl_path = None if rctx.attr.whl_file: @@ -245,6 +247,7 @@ def _whl_library_impl(rctx): environment = environment, quiet = rctx.attr.quiet, timeout = rctx.attr.timeout, + logger = logger, ) whl_path = rctx.path(json.decode(rctx.read("whl_file.json"))["whl_file"]) @@ -292,6 +295,7 @@ def _whl_library_impl(rctx): environment = environment, quiet = rctx.attr.quiet, timeout = rctx.attr.timeout, + logger = logger, ) metadata = json.decode(rctx.read("metadata.json")) @@ -440,6 +444,7 @@ attr makes `extra_pip_args` and `download_only` ignored.""", for repo in all_repo_names ], ), + "_rule_name": attr.string(default = "whl_library"), }, **ATTRS) whl_library_attrs.update(AUTH_ATTRS) diff --git a/python/private/repo_utils.bzl b/python/private/repo_utils.bzl index 3c07027663..1c50ac6bf4 100644 --- a/python/private/repo_utils.bzl +++ b/python/private/repo_utils.bzl @@ -31,17 +31,6 @@ def _is_repo_debug_enabled(rctx): """ return _getenv(rctx, REPO_DEBUG_ENV_VAR) == "1" -def _debug_print(rctx, message_cb): - """Prints a message if repo debugging is enabled. - - Args: - rctx: repository_ctx - message_cb: Callable that returns the string to print. Takes - no arguments. - """ - if _is_repo_debug_enabled(rctx): - print(message_cb()) # buildifier: disable=print - def _logger(ctx, name = None): """Creates a logger instance for printing messages. @@ -73,7 +62,7 @@ def _logger(ctx, name = None): elif not name: fail("The name has to be specified when using the logger with `module_ctx`") - def _log(enabled_on_verbosity, level, message_cb_or_str): + def _log(enabled_on_verbosity, level, message_cb_or_str, printer = print): if verbosity < enabled_on_verbosity: return @@ -82,7 +71,8 @@ def _logger(ctx, name = None): else: message = message_cb_or_str() - print("\nrules_python:{} {}:".format( + # NOTE: printer may be the `fail` function. + printer("\nrules_python:{} {}:".format( name, level.upper(), ), message) # buildifier: disable=print @@ -92,6 +82,7 @@ def _logger(ctx, name = None): debug = lambda message_cb: _log(2, "DEBUG", message_cb), info = lambda message_cb: _log(1, "INFO", message_cb), warn = lambda message_cb: _log(0, "WARNING", message_cb), + fail = lambda message_cb: _log(-1, "FAIL", message_cb, fail), ) def _execute_internal( @@ -101,6 +92,7 @@ def _execute_internal( fail_on_error = False, arguments, environment = {}, + logger = None, **kwargs): """Execute a subprocess with debugging instrumentation. @@ -114,12 +106,15 @@ def _execute_internal( arguments: list of arguments; see rctx.execute#arguments. environment: optional dict of the environment to run the command in; see rctx.execute#environment. + logger: optional `Logger` to use for logging execution details. If + not specified, a default will be created. **kwargs: additional kwargs to pass onto rctx.execute Returns: exec_result object, see repository_ctx.execute return type. """ - _debug_print(rctx, lambda: ( + logger = logger or _logger(rctx) + logger.debug(lambda: ( "repo.execute: {op}: start\n" + " command: {cmd}\n" + " working dir: {cwd}\n" + @@ -137,7 +132,7 @@ def _execute_internal( result = rctx.execute(arguments, environment = environment, **kwargs) if fail_on_error and result.return_code != 0: - fail(( + logger.fail(( "repo.execute: {op}: end: failure:\n" + " command: {cmd}\n" + " return code: {return_code}\n" + @@ -155,8 +150,7 @@ def _execute_internal( output = _outputs_to_str(result), )) elif _is_repo_debug_enabled(rctx): - # buildifier: disable=print - print(( + logger.debug(( "repo.execute: {op}: end: {status}\n" + " return code: {return_code}\n" + "{output}" @@ -413,7 +407,6 @@ def _watch_tree(rctx, *args, **kwargs): repo_utils = struct( # keep sorted - debug_print = _debug_print, execute_checked = _execute_checked, execute_checked_stdout = _execute_checked_stdout, execute_unchecked = _execute_unchecked, diff --git a/python/private/toolchains_repo.bzl b/python/private/toolchains_repo.bzl index dd59e477d8..df16fb8cf7 100644 --- a/python/private/toolchains_repo.bzl +++ b/python/private/toolchains_repo.bzl @@ -120,9 +120,10 @@ toolchains_repo = repository_rule( ) def _toolchain_aliases_impl(rctx): - (os_name, arch) = get_host_os_arch(rctx) + logger = repo_utils.logger(rctx) + (os_name, arch) = _get_host_os_arch(rctx, logger) - host_platform = get_host_platform(os_name, arch) + host_platform = _get_host_platform(os_name, arch) is_windows = (os_name == WINDOWS_NAME) python3_binary_path = "python.exe" if is_windows else "bin/python3" @@ -236,14 +237,15 @@ actions.""", ) def _host_toolchain_impl(rctx): + logger = repo_utils.logger(rctx) rctx.file("BUILD.bazel", """\ # Generated by python/private/toolchains_repo.bzl exports_files(["python"], visibility = ["//visibility:public"]) """) - (os_name, arch) = get_host_os_arch(rctx) - host_platform = get_host_platform(os_name, arch) + (os_name, arch) = _get_host_os_arch(rctx, logger) + host_platform = _get_host_platform(os_name, arch) repo = "@@{py_repository}_{host_platform}".format( py_repository = rctx.attr.name[:-len("_host")], host_platform = host_platform, @@ -320,6 +322,7 @@ this repo causes an eager fetch of the toolchain for the host platform. mandatory = True, doc = "The base name for all created repositories, like 'python38'.", ), + "_rule_name": attr.string(default = "host_toolchain"), "_rules_python_workspace": attr.label(default = Label("//:WORKSPACE")), }, ) @@ -384,7 +387,7 @@ multi_toolchain_aliases = repository_rule( def sanitize_platform_name(platform): return platform.replace("-", "_") -def get_host_platform(os_name, arch): +def _get_host_platform(os_name, arch): """Gets the host platform. Args: @@ -401,11 +404,13 @@ def get_host_platform(os_name, arch): fail("No platform declared for host OS {} on arch {}".format(os_name, arch)) return host_platform -def get_host_os_arch(rctx): +def _get_host_os_arch(rctx, logger): """Infer the host OS name and arch from a repository context. Args: rctx: Bazel's repository_ctx. + logger: Logger to use for operations. + Returns: A tuple with the host OS name and arch. """ @@ -423,6 +428,7 @@ def get_host_os_arch(rctx): rctx, op = "GetUname", arguments = [repo_utils.which_checked(rctx, "uname"), "-m"], + logger = logger, ).stdout.strip() # Normalize the os_name. diff --git a/python/repositories.bzl b/python/repositories.bzl index d58feefd31..baa5f5ba70 100644 --- a/python/repositories.bzl +++ b/python/repositories.bzl @@ -76,12 +76,13 @@ def py_repositories(): STANDALONE_INTERPRETER_FILENAME = "STANDALONE_INTERPRETER" -def is_standalone_interpreter(rctx, python_interpreter_path): +def is_standalone_interpreter(rctx, python_interpreter_path, *, logger = None): """Query a python interpreter target for whether or not it's a rules_rust provided toolchain Args: rctx (repository_ctx): The repository rule's context object. python_interpreter_path (path): A path representing the interpreter. + logger: Optional logger to use for operations. Returns: bool: Whether or not the target is from a rules_python generated toolchain. @@ -102,6 +103,7 @@ def is_standalone_interpreter(rctx, python_interpreter_path): STANDALONE_INTERPRETER_FILENAME, ), ], + logger = logger, ).return_code == 0 def _python_repository_impl(rctx): @@ -110,6 +112,8 @@ def _python_repository_impl(rctx): if bool(rctx.attr.url) == bool(rctx.attr.urls): fail("Exactly one of (url, urls) must be set.") + logger = repo_utils.logger(rctx) + platform = rctx.attr.platform python_version = rctx.attr.python_version python_version_info = python_version.split(".") @@ -145,6 +149,7 @@ def _python_repository_impl(rctx): timeout = 600, quiet = True, working_directory = working_directory, + logger = logger, ) zstd = "{working_directory}/zstd".format(working_directory = working_directory) unzstd = "./unzstd" @@ -160,6 +165,7 @@ def _python_repository_impl(rctx): "--use-compress-program={unzstd}".format(unzstd = unzstd), "--file={}".format(release_filename), ], + logger = logger, ) else: rctx.download_and_extract( @@ -197,11 +203,13 @@ def _python_repository_impl(rctx): rctx, op = "python_repository.MakeReadOnly", arguments = [repo_utils.which_checked(rctx, "chmod"), "-R", "ugo-w", lib_dir], + logger = logger, ) exec_result = repo_utils.execute_unchecked( rctx, op = "python_repository.TestReadOnly", arguments = [repo_utils.which_checked(rctx, "touch"), "{}/.test".format(lib_dir)], + logger = logger, ) # The issue with running as root is the installation is no longer @@ -211,6 +219,7 @@ def _python_repository_impl(rctx): rctx, op = "python_repository.GetUserId", arguments = [repo_utils.which_checked(rctx, "id"), "-u"], + logger = logger, ) uid = int(stdout.strip()) if uid == 0: @@ -529,6 +538,7 @@ For more information see the official bazel docs "zstd_version": attr.string( default = "1.5.2", ), + "_rule_name": attr.string(default = "python_repository"), }, environ = [REPO_DEBUG_ENV_VAR], )