Change ConfiguredTargetQuery to use KeyedConfiguredTarget as a value. #12547
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will allow us to keep the configured target key between lookup rounds. Fixes bazelbuild#11993.
katre
force-pushed
the
i11993-04-keyed-config-target
branch
from
234d1cb
to
4ceaa1e
Compare
katre
changed the title
juliexxia
reviewed
Nov 25, 2020
| @@ -180,14 +176,17 @@ public Target getTargetFromConfiguredTarget(ConfiguredTarget configuredTarget) { | |||
| } | |||
|
|
|||
| /** Returns the rule that generates the given output file. */ | |||
| public RuleConfiguredTarget getGeneratingConfiguredTarget(OutputFileConfiguredTarget oct) | |||
| RuleConfiguredTarget getGeneratingConfiguredTarget(KeyedConfiguredTarget kct) | |||
There was a problem hiding this comment.
Should we assert that this KeyedConfiguredTarget contains an OutputFileConfiguredTarget if we're going to cast on 186?
| .setConfiguredTarget(target) | ||
| .setConfiguration(getConfiguration(target)) | ||
| .build(); | ||
| protected ConfiguredTargetKey getSkyKey(KeyedConfiguredTarget target) { |
There was a problem hiding this comment.
I think this method's assumptions have caused other issues in the past so it's nice we can tighten it up here.
| @@ -120,9 +119,9 @@ public void testLabelFunction_getCorrectlyConfiguredDeps() throws Exception { | |||
| setUpLabelsFunctionTests(); | |||
|
|
|||
| // Test that this retrieves the correctly configured version(s) of the dep(s). | |||
| ConfiguredTarget patchDep = | |||
| KeyedConfiguredTarget patchDep = | |||
There was a problem hiding this comment.
Can we add a test case for the situation that caused #11993 ?
There was a problem hiding this comment.
Good point. Added to //src/test/shell/bazel:toolchain_transition_test
juliexxia
reviewed
Nov 25, 2020
| @@ -355,4 +354,29 @@ EOF | |||
| expect_log 'extra_lib: message: extra_lib foo, target_dep: target, tool_dep: exec-alpha' | |||
| } | |||
|
|
|||
| function test_toolchain_transition_cquery() { | |||
There was a problem hiding this comment.
Do you mind adding a comment about what this seemingly innocuous test case is testing for?
juliexxia
approved these changes
Nov 25, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will allow us to keep the configured target key between lookup rounds.
Fixes #11993.