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

Update ctexplain to run with bazel and newer python versions #17109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion third_party/py/frozendict/frozendict/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,15 @@
except ImportError: # python < 2.7
OrderedDict = NotImplemented

try:
from collections import Mapping
except ImportError:
from collections.abc import Mapping

iteritems = getattr(dict, 'iteritems', dict.items) # py2-3 compatibility


class frozendict(collections.Mapping):
class frozendict(Mapping):
"""
An immutable wrapper around dictionaries that implements the complete :py:class:`collections.Mapping`
interface. It can be used as a drop-in replacement for dictionaries where immutability is desired.
Expand Down
2 changes: 2 additions & 0 deletions tools/ctexplain/analyses/summary.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
from tools.ctexplain.types import ConfiguredTarget
# Do not edit this line. Copybara replaces it with PY2 migration helper..third_party.bazel.tools.ctexplain.util as util

import tools.ctexplain.util as util


@dataclass(frozen=True)
class _Summary():
Expand Down
38 changes: 22 additions & 16 deletions tools/ctexplain/bazel_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@

There's no Python Bazel API so we invoke Bazel as a subprocess.
"""
import functools
import json
import os
import re
import subprocess
from typing import Callable
from typing import List
Expand All @@ -28,8 +30,12 @@
from tools.ctexplain.types import HostConfiguration
from tools.ctexplain.types import NullConfiguration

CQUERY_RESULT_LINE_REGEX = re.compile(r"^(.*?) \((\S+)\) \[([^[]*)\]$")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what official style recommendations are, but can this be colocated closer to its sole consumer for less context jumping?

DEFAULT_BAZEL_BINARY = "bazel"

def run_bazel_in_client(args: List[str]) -> Tuple[int, List[str], List[str]]:

def run_bazel_in_client(args: List[str],
bazel: str = DEFAULT_BAZEL_BINARY) -> Tuple[int, List[str], List[str]]:
"""Calls bazel within the current workspace.

For production use. Tests use an alternative invoker that goes through test
Expand All @@ -42,7 +48,7 @@ def run_bazel_in_client(args: List[str]) -> Tuple[int, List[str], List[str]]:
Tuple of (return code, stdout, stderr)
"""
result = subprocess.run(
["blaze"] + args,
[bazel] + args,
cwd=os.getcwd(),
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
Expand All @@ -57,8 +63,12 @@ class BazelApi():
def __init__(self,
run_bazel: Callable[[List[str]],
Tuple[int, List[str],
List[str]]] = run_bazel_in_client):
List[str]]] = None,
bazel: str = DEFAULT_BAZEL_BINARY):
self.bazel = bazel
self.run_bazel = run_bazel
if run_bazel is None:
self.run_bazel = functools.partial(run_bazel_in_client, bazel=self.bazel)

def cquery(self,
args: List[str]) -> Tuple[bool, str, Tuple[ConfiguredTarget, ...]]:
Expand Down Expand Up @@ -144,20 +154,16 @@ def _parse_cquery_result_line(line: str) -> ConfiguredTarget:
Returns:
Corresponding ConfiguredTarget if the line matches else None.
"""
tokens = line.split(maxsplit=2)
label = tokens[0]
if tokens[1][0] != "(" or tokens[1][-1] != ")":
raise ValueError(f"{tokens[1]} in {line} not surrounded by parentheses")
config_hash = tokens[1][1:-1]
if config_hash == "null":
fragments = ()
result = CQUERY_RESULT_LINE_REGEX.search(line)
Copy link
Contributor

Choose a reason for hiding this comment

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

Implementing the TODO from line 139 would be a great followup, especially if it's not hard (not sure it is).

I know you're just expanding on the parsing logic already implemented, but this (existing) logic is way more brittle than it should be.

if result is None:
raise ValueError(f"{repr(line)} does not match {repr(CQUERY_RESULT_LINE_REGEX.pattern)}")
label, config_hash, fragments_str = result.groups()
if fragments_str:
# The fragments list looks like 'Fragment1, Fragment2, ...'. Split on
# ', ' to convert it to a structured tuple.
fragments = tuple(fragments_str.split(", "))
else:
if tokens[2][0] != "[" or tokens[2][-1] != "]":
raise ValueError(f"{tokens[2]} in {line} not surrounded by [] brackets")
# The fragments list looks like '[Fragment1, Fragment2, ...]'. Split the
# whole line on ' [' to get just this list, then remove the final ']', then
# split again on ', ' to convert it to a structured tuple.
fragments = tuple(line.split(" [")[1][0:-1].split(", "))
fragments = ()
return ConfiguredTarget(
label=label,
config=None, # Not yet available: we'll need `bazel config` to get this.
Expand Down
16 changes: 12 additions & 4 deletions tools/ctexplain/ctexplain.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,15 @@
from dataclasses import dataclass

# Do not edit this line. Copybara replaces it with PY2 migration helper..third_party.bazel.tools.ctexplain.analyses.summary as summary
from tools.ctexplain.bazel_api import BazelApi
from tools.ctexplain.bazel_api import DEFAULT_BAZEL_BINARY, BazelApi
# Do not edit this line. Copybara replaces it with PY2 migration helper..third_party.bazel.tools.ctexplain.lib as lib
from tools.ctexplain.types import ConfiguredTarget
# Do not edit this line. Copybara replaces it with PY2 migration helper..third_party.bazel.tools.ctexplain.util as util

import tools.ctexplain.lib as lib
import tools.ctexplain.util as util
import tools.ctexplain.analyses.summary as summary

FLAGS = flags.FLAGS


Expand Down Expand Up @@ -112,6 +116,8 @@ def _render_analysis_help_text() -> str:
lambda flag_value: all(name in analyses for name in flag_value),
message=f'available analyses: {", ".join(analyses.keys())}')

flags.DEFINE_string("bazel", DEFAULT_BAZEL_BINARY, "Path to bazel binary")

flags.DEFINE_multi_string(
"build", [],
"""command-line invocation of the build to analyze. For example:
Expand All @@ -133,8 +139,8 @@ def _get_build_flags(cmdline: str) -> Tuple[Tuple[str, ...], Tuple[str, ...]]:
Tuple of ((target labels to build), (build flags))
"""
cmdlist = cmdline.split()
labels = [arg for arg in cmdlist if arg.startswith("//")]
build_flags = [arg for arg in cmdlist if not arg.startswith("//")]
labels = [arg for arg in cmdlist if arg.startswith("//") or arg.startswith("@")]
build_flags = [arg for arg in cmdlist if not arg.startswith("//") and not arg.startswith("@")]
return (tuple(labels), tuple(build_flags))


Expand All @@ -148,8 +154,10 @@ def main(argv):

(labels, build_flags) = _get_build_flags(FLAGS.build[0])
build_desc = ",".join(labels)

with util.ProgressStep(f"Collecting configured targets for {build_desc}"):
cts = lib.analyze_build(BazelApi(), labels, build_flags)
cts = lib.analyze_build(BazelApi(bazel=FLAGS.bazel), labels, build_flags)

for analysis in FLAGS.analysis:
analyses[analysis].exec(cts)

Expand Down
1 change: 1 addition & 0 deletions tools/ctexplain/lib.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

# Do not edit this line. Copybara replaces it with PY2 migration helper..third_party.bazel.tools.ctexplain.bazel_api as bazel_api
from tools.ctexplain.types import ConfiguredTarget
import tools.ctexplain.bazel_api as bazel_api


def analyze_build(bazel: bazel_api.BazelApi, labels: Tuple[str, ...],
Expand Down