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

ctexplain: first functional check-in #11829

Closed
wants to merge 10 commits into from

Conversation

gregestren
Copy link
Contributor

@gregestren gregestren commented Jul 23, 2020

#11511 set up basic project structure. This PR adds minimum working functionality.

Specifically, you can run it with a build command and it reports basic stats on the build's graph.

Example:

$ bazel-bin/tools/ctexplain/ctexplain -b "//testapp:foo"
Collecting configured targets for //testapp:foo... done in 0.62 s.

Configurations: 3
Targets: 79
Configured targets: 92 (+16.5% vs. targets)
Targets with multiple configs: 13

Notes:

  • Changed import structure to prefer module imports over function, class imports (style guide recommendation)
  • Set up structure for injecting arbitrary analyses. Each analysis consumes the build's set of configured targets and can output whatever it wants.
  • Implemented one basic analysis
  • Structured code to make it easy to fork output formatters (e.g. for machine-readable output). But tried not to add speculative inheritance / boilerplate too soon

Context: Measuring Configuration Overhead.
Work towards #10613

@meisterT
Copy link
Member

cc @kkress

Copy link
Contributor

@sdtwigg sdtwigg left a comment

Choose a reason for hiding this comment

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

The implementation and approach is fine. Mostly stylistic concerns.

repeated_targets: int


def analyze(cts: Tuple[ConfiguredTarget, ...]):
Copy link
Contributor

Choose a reason for hiding this comment

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

Pytype should let you annotate the return type. This ends up making most of the docstring redundant.

def analyze(cts: Tuple[ConfiguredTarget, ...]) -> _Summary:
  """Runs the analysis on a build's configured targets."""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

I remain confused about when pytype insists on an Args description and when not. Do you know?

Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming you mean pylint but, in my experience, it requires it when the docstring is >1 line.

There was talk about relaxing the requirement when pytype is used; however, do not know if that was ever actually executed.

tools/ctexplain/analyses/summary.py Outdated Show resolved Hide resolved
tools/ctexplain/ctexplain.py Outdated Show resolved Hide resolved
tools/ctexplain/ctexplain.py Outdated Show resolved Hide resolved


def main(argv):
del argv # Satisfy py linter's "unused" warning.
Copy link
Contributor

Choose a reason for hiding this comment

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

Error if unexpected arguments remain? e.g.

      if len(argv) > 1:
        raise app.UsageError('Too many command-line arguments.')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FLAGS already catches unknown arguments. Would this be redundant?

Copy link
Contributor

Choose a reason for hiding this comment

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

Without proof, I think this catches adding arguments after just --. I would need to experiment since nothing showed up on a cursory glance.

tools/ctexplain/lib.py Outdated Show resolved Hide resolved
@bazel-io bazel-io closed this in c1d7087 Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants