refactor: use logger in repo_utils.execute_* functions #2082
Conversation
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
…o refactor.execute.uses.logger
|
Piping |
There was a problem hiding this comment.
LGTM.
I like the suggestion to wrap the context and have extra methods, like log.warn or execute_unchecked. I would say that we can add a note in our issue list and consider it as a followup to this PR as this PR already improves things.
We could also unify the API by having watch method for bazel 6, so our code would become more modern.
| @@ -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), | |||
There was a problem hiding this comment.
The only annoying thing about the fail method is that one has to do a return in the code afterwards to satisfy buildifier.
There was a problem hiding this comment.
oh? I didn't see that buildifier error pop up.
| 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) |
There was a problem hiding this comment.
If I remember correctly this code was written before rctx and mctx had the architecture. I think we could look into removing the dependency on uname here.
And just use the repo utils equivalents.
There was a problem hiding this comment.
Ahh, that would explain a bit. I always wondered why we were calling uname when rctx seemed to have the equivalent.
…les_python into refactor.execute.uses.logger
|
Maybe it is just a warning... I'll paste a concrete example if I see it.
…On 22 July 2024 13:22:48 GMT+09:00, Richard Levasseur ***@***.***> wrote:
@rickeylev commented on this pull request.
> @@ -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),
oh? I didn't see that buildifier error pop up.
--
Reply to this email directly or view it on GitHub:
#2082 (comment)
You are receiving this because your review was requested.
Message ID: ***@***.***>
|
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_nameattribute for the logger to use.