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

Fix incompatible target skipping for skylib's analysistest #12368

Conversation

philsc
Copy link
Contributor

@philsc philsc commented Oct 28, 2020

In #12366, Keith Smiley (@keith) reported that he was having trouble
using the new target_compatible_with attribute in rules_swift.
Specifically, setting it on an analysistest test target was giving
him the following error:

ERROR: /workdir/test/BUILD:13:29: in coverage_xcode_prefix_map_test rule //test:coverage_settings_xcode_prefix_map: rules with analysis_test=true must return an instance of AnalysisTestResultInfo
ERROR: Analysis of target '//test:coverage_settings_xcode_prefix_map' failed; build aborted: Analysis of target '//test:coverage_settings_xcode_prefix_map' failed

This was caused by the fact that for incompatible targets we create a
dummy ConfiguredTarget that only provides the bare minimum to make
the rest of bazel happy before it gets skipped. It turns out that
analysistest rules have additional checks that need to be satisfied.
This patch aims to satisfy those additional checks without impacting
functionality.

Fixes #12366

In bazelbuild#12366, Keith Smiley (@keith) reported that he was having trouble
using the new `target_compatible_with` attribute in `rules_swift`.
Specifically, setting it on an `analysistest` test target was giving
him the following error:

    ERROR: /workdir/test/BUILD:13:29: in coverage_xcode_prefix_map_test rule //test:coverage_settings_xcode_prefix_map: rules with analysis_test=true must return an instance of AnalysisTestResultInfo
    ERROR: Analysis of target '//test:coverage_settings_xcode_prefix_map' failed; build aborted: Analysis of target '//test:coverage_settings_xcode_prefix_map' failed

This was caused by the fact that for incompatible targets we create a
dummy `ConfiguredTarget` that only provides the bare minimum to make
the rest of bazel happy before it gets skipped. It turns out that
`analysistest` rules have additional checks that need to be satisfied.
This patch aims to satisfy those additional checks without impacting
functionality.

Fixes bazelbuild#12366
@google-cla google-cla bot added the cla: yes label Oct 28, 2020
@philsc
Copy link
Contributor Author

philsc commented Oct 28, 2020

@keith , when you have a chance, could you do me a favor and give this a try?

I realized just now that the test case I used is slightly different from your use case, but I'm hoping it'll still work.

If it doesn't work, I'll take another look tomorrow or Thursday.

@philsc
Copy link
Contributor Author

philsc commented Oct 28, 2020

@gregestren , does this look reasonable? Using addNativeDeclaredProvider feels a bit invasive, but I couldn't find anything better.

@gregestren
Copy link
Contributor

It looks reasonable to me given the context. What about it do you find invasive?

@philsc
Copy link
Contributor Author

philsc commented Oct 28, 2020

"invasive" was definitely the wrong word to use, apologies.
It just seemed a bit too low-level, "reachy".

Looking at it with fresh eyes, it's not looking that bad. Sorry for the confusion.

@keith
Copy link
Member

keith commented Oct 28, 2020

I validated this now correctly skips the test:

//test:coverage_settings_xcode_prefix_map                               SKIPPED
Executed 42 out of 43 tests: 42 tests pass and 1 was skipped.

@keith
Copy link
Member

keith commented Nov 2, 2020

@gregestren can you help land this?

@gregestren
Copy link
Contributor

I started merging on Friday but ran into some CI silliness. Stay tuned...

@bazel-io bazel-io closed this in 20b0563 Nov 2, 2020
@philsc philsc deleted the incompatible-target-skipping-analysistest-fix branch November 3, 2020 04:11
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.

target_compatible_with failing with analysis_test
3 participants