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: make cquery parsing more robust #13212

Closed
wants to merge 1 commit into from

Conversation

gregestren
Copy link
Contributor

Specifically:

  • Handle labels with spaces
  • Handle targets with no fragment requirements.

These were both inspired by real analyses.

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

Specifically:

 - Handle labels with spaces
 - Handle targets with no fragment requirements.

These were both inspired by real analyses.

Testing: $ bazel test //tools/ctexplain:all
Work towards bazelbuild#10613
@google-cla google-cla bot added the cla: yes label Mar 12, 2021
@gregestren gregestren added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Mar 12, 2021
@gregestren gregestren requested a review from sdtwigg March 12, 2021 19:05
if fragments_start == fragments_end - 1:
fragments = tuple()
else:
fragments = tuple(line[fragments_start + 1:fragments_end].split(", "))
Copy link
Contributor

Choose a reason for hiding this comment

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

tuple(x.strip() for x in line[fragments_start + 1:fragments_end].split(","))

is a little more resilient to whitespace changes.

config_hash = tokens[1][1:-1]
config_hash_start = line.find("(")
config_hash_end = line.find(")", config_hash_start)
fragments_start = line.find("[", config_hash_end)
Copy link
Contributor

Choose a reason for hiding this comment

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

move under the else: condition below; after verifying not in null configuration

success = res[0]
cts = res[2]
self.assertTrue(success)
self.assertEqual(len(cts), 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

assertLen

success = res[0]
cts = res[2]
self.assertTrue(success)
self.assertEqual(len(cts), 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

assertLen

self.assertTrue(success)
self.assertEqual(len(cts), 1)
self.assertEqual(cts[0].label, '//testapp:alias_to_src')
self.assertEqual(len(cts[0].transitive_fragments), 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

assertEmpty

@gregestren gregestren added the P2 We'll consider working on this in future. (Assignee optional) label Jul 22, 2021
@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 all, 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.

@keertk keertk closed this Dec 7, 2022
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.

4 participants