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: report a build's trimmability #13210

Conversation

gregestren
Copy link
Contributor

@gregestren gregestren commented Mar 12, 2021

Example:

Collecting configured targets for //testex:split... done in 1.00 s.

Configurations: 3
Targets: 12
Configured targets: 22 (+83.3% vs. targets)
Targets with multiple configs: 10
Configured targets with optimal trimming: 12 (+0.0% vs. targets)
Trimming impact on configured target graph size: -45.5%

Note the Copybara imports are messed up as checked in. I'll have to figure out (in another PR) how to stop them from doing that.

Testing: $ bazel test //tools/ctexplain:all
Work towards #10613

 - cquery now returns short config hashes (expect shorter lengths)
 - Starlark flags must now use "--//flag=value" syntax
@google-cla google-cla bot added the cla: yes label Mar 12, 2021
@gregestren gregestren requested a review from sdtwigg March 12, 2021 04:42
@gregestren gregestren changed the title ctexplain: report a buil's trimmability ctexplain: report a build's trimmability Mar 12, 2021
@gregestren gregestren added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Mar 12, 2021
@gregestren gregestren added the P2 We'll consider working on this in future. (Assignee optional) label Jul 22, 2021
@nikhilkalige
Copy link
Contributor

@gregestren I was playing around with this change. It fails when you have multiple transitions. I will try to create an example, but to put it in words

All my transitions are on string_flag with single value
Cquery gives me 4 config values ex: [1xxx, 2xxx, 3xxx, 4xxx]

cquery --show_config_fragments.. runs
//:mainA (1xxx) [flagA, flagB, .... ]
.....
bazel config 1xxx
name: user-defined
options: {
  flagA: flagValue
}

With the above data, in _get_required_fragments ,req loops over all fragments (flagA, flagB), but only flagA is defined in the config.

Also should this be changed to

for req in ct.transitive_fragments:
        if (req in ct.config.fragments or req.startswith("--") or req == "CoreOptions"):
user_defined = ct.config.options.get('user-defined', dict)
for req in ct.transitive_fragments:
        if (req in ct.config.fragments or req.startswith("--") or req in user_defined or req == "CoreOptions"):

@gregestren
Copy link
Contributor Author

I really need to land the 1.0 of ctexplain, which I believe is this and another pending PR. From there we can properly examine what does and doesn't work.

I'll admit it's hard for me to prioritize this at the moment. If anyone wanted to assist that could help.

@nikhilkalige
Copy link
Contributor

I can help out. Let me know how I can be useful.

@gregestren
Copy link
Contributor Author

@nikhilkalige still any interest? I realize it's been a long time, so no worries whatever the case.

@nikhilkalige
Copy link
Contributor

@gregestren Sorry.. I missed that.. But I am interested in helping out

@gregestren
Copy link
Contributor Author

We just decided to revive this with @sdtwigg . Taking note of your interest. We'll have to coordinate to agree on next steps, but it'll be basically getting this code properly merged.

@sgowroji sgowroji added the awaiting-user-response Awaiting a response from the author label Oct 19, 2022
@keertk
Copy link
Member

keertk commented Dec 7, 2022

Hi there! Thank you for contributing to the Bazel repository. We appreciate your time and effort. We're doing a clean up of old PRs and will be closing this one since it seems to have stalled. Please feel free to reopen if you’re still interested in pursuing this or if you'd like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.

@keertk keertk closed this Dec 7, 2022
@fmeum
Copy link
Collaborator

fmeum commented Dec 7, 2022

@gregestren I would still be interested in this functionality. Are you planning to work on it again at some point?

@gregestren
Copy link
Contributor Author

I'd love to properly land this but I need reviewer support

I would consider trying to push this through in the new year.

Let's talk about this as one of our subjects at our bi-weekly syncs I'm still responsible for setting up. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-user-response Awaiting a response from the author cla: yes P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants