Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add debug logging of repo sub processes #1735

Merged
merged 5 commits into from
Jan 31, 2024

Conversation

rickeylev
Copy link
Contributor

Debugging what the repo rules are up to can be difficult because Bazel doesn't provide many facilities to inspect what they're up to. This adds an environment variable, RULES_PYTHON_REPO_DEBUG, that, when set, will make our repo rules print out detailed information about the subprocesses they are running.

This also makes failed commands dump much more comprehensive information.

This was driven by the recent report of failures on Windows during a repo rule.

Debugging what the repo rules are up to can be difficult because Bazel
doesn't provide many facilities to inspect what they're up to. This
adds an environment variable, `RULES_PYTHON_REPO_DEBUG`, that, when
set, will make our repo rules print out detailed information about
the subprocesses they are running.

This also makes failed commands dump much more comprehensive
information.
@rickeylev rickeylev requested a review from aignas January 30, 2024 23:09
@rickeylev rickeylev changed the title feat: repos debug logging of sub processes feat: add debug logging of repo sub processes Jan 30, 2024
Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is really great!

args,
repo_utils.execute_checked(
rctx,
op = "whl_library.ResolveRequirement({}, {})".format(rctx.attr.name, rctx.attr.requirement),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we could also have this semantics for the op, where the first item is the function name and the remaining are parameters which will be formated to: op[0] + "(" + ",".join(op[1:]) + ")"

Suggested change
op = "whl_library.ResolveRequirement({}, {})".format(rctx.attr.name, rctx.attr.requirement),
op = ["whl_library.ResolveRequirement", rctx.attr.name, rctx.attr.requirement],

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea. It also gave me another idea: automatically add the "whl_library" part (the repo rule name) by looking for a _repo_name attribute we set on all our repo rules.

But, I'm strapped for time today, so I'm going to merge this as-is.

Copy link
Contributor Author

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried writing some tests for this but it started to spiral out of control pretty quickly (I tried mocking rctx, fail, and print 😵‍💫) and didn't test very well. I manually ran bazel build ... with the env set and forcing failures instead.

args,
repo_utils.execute_checked(
rctx,
op = "whl_library.ResolveRequirement({}, {})".format(rctx.attr.name, rctx.attr.requirement),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea. It also gave me another idea: automatically add the "whl_library" part (the repo rule name) by looking for a _repo_name attribute we set on all our repo rules.

But, I'm strapped for time today, so I'm going to merge this as-is.

@rickeylev rickeylev added this pull request to the merge queue Jan 31, 2024
Merged via the queue into bazelbuild:main with commit e53b0b7 Jan 31, 2024
4 checks passed
@rickeylev rickeylev deleted the repos.cmds branch February 1, 2024 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants