Skip to content

feat: add debug logging of repo sub processes#1735

Merged
rickeylev merged 5 commits intobazel-contrib:mainfrom
rickeylev:repos.cmds
Jan 31, 2024
Merged

feat: add debug logging of repo sub processes#1735
rickeylev merged 5 commits intobazel-contrib:mainfrom
rickeylev:repos.cmds

Conversation

@rickeylev
Copy link
Copy Markdown
Collaborator

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 requested a review from f0rmiga as a code owner 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator 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
Copy Markdown
Collaborator 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
Copy Markdown
Collaborator 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 enabled auto-merge January 31, 2024 16:45
@rickeylev rickeylev added this pull request to the merge queue Jan 31, 2024
Merged via the queue into bazel-contrib:main with commit e53b0b7 Jan 31, 2024
@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