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

Refactor Incompatible Target Skipping #14096

Closed
wants to merge 60 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
da36c56
EXPERIMENT: Investigate objc_library and target_compatible_with
philsc Feb 13, 2021
793f85d
Add more tests
philsc Oct 21, 2021
63f5339
Add another test
philsc Oct 21, 2021
f2b0191
Add some comments
philsc Oct 21, 2021
1361941
Add another test
philsc Oct 21, 2021
76a70ec
First attempt at cleanup
philsc Nov 12, 2021
8cb3cd8
clean up some more
philsc Nov 12, 2021
de7bfb0
minor cleanup
philsc Nov 12, 2021
00a482e
Clean up some more
philsc Nov 14, 2021
5984a1b
A little more cleanup
philsc Nov 14, 2021
51c924c
Merge remote-tracking branch 'upstream/master' into HEAD
philsc Nov 14, 2021
51e79d6
Fix merge conflicts
philsc Nov 16, 2021
9601882
clean up TestParams
philsc Nov 18, 2021
ce147fc
Merge commit '28c09f29bc' into HEAD
philsc Jun 12, 2022
58fd3d6
Merge commit '3fec0e1870' into HEAD
philsc Jun 12, 2022
055cd7f
Merge commit '33b248366b' into HEAD
philsc Jun 12, 2022
024835c
Merge commit '3badca3f7e' into HEAD
philsc Jun 12, 2022
0642ee4
Fix merge conflict
philsc Jun 12, 2022
3f4bcb7
Merge commit '02ccf03756' into HEAD
philsc Jun 12, 2022
12c0d11
Merge commit 'ab42432a4d' into HEAD
philsc Jun 12, 2022
072af05
Merge commit '29686447b7' into HEAD
philsc Jun 12, 2022
cdd7012
Merge commit '2fdac7e4b0' into HEAD
philsc Jun 12, 2022
b35ee09
Merge commit '8bee70cf52' into HEAD
philsc Jun 12, 2022
f931bb7
Merge commit '691fc5bcc0' into HEAD
philsc Jun 12, 2022
d055d20
Merge commit '1941dc1551' into HEAD
philsc Jun 12, 2022
65958af
Merge commit 'c20f797149' into HEAD
philsc Jun 12, 2022
88eea67
Merge commit 'f0e63c4c93' into HEAD
philsc Jun 12, 2022
97d76d2
Merge remote-tracking branch 'upstream/master' into HEAD
philsc Jul 10, 2022
4dd2c8f
Simplify getAssociatedRule() call
philsc Jul 10, 2022
d6dd691
Start moving stuff into a dedicated file to be renamed properly later
philsc Jul 11, 2022
f4e2d09
Move most logic into dedicated file
philsc Jul 11, 2022
264f4ec
Auto format
philsc Jul 11, 2022
346a1d7
sort deps
philsc Jul 11, 2022
fb51e7f
Add alias target.
philsc Jul 11, 2022
11a07e4
Incorporate feedback
philsc Jul 12, 2022
03196c7
Clean up some more
philsc Jul 12, 2022
81d4092
Merge remote-tracking branch 'upstream/master' into HEAD
philsc Jul 12, 2022
c1e93d8
Delete superfluous dependencies
philsc Jul 12, 2022
6e3dd4b
auto-format
philsc Jul 12, 2022
5331431
rename class/file
philsc Jul 13, 2022
89df7a8
simplify return type
philsc Jul 13, 2022
0d56c93
Switch to ConfiguredTargetKey
philsc Jul 13, 2022
788d70e
add comments
philsc Jul 13, 2022
8132524
tiny refactor tweak
philsc Jul 13, 2022
4951859
Merge remote-tracking branch 'upstream/master' into HEAD
philsc Jul 13, 2022
4fb916f
Format
philsc Jul 13, 2022
af97f66
Merge remote-tracking branch 'upstream/master' into HEAD
philsc Jul 14, 2022
04a6479
Switch to `Optional`
philsc Jul 14, 2022
9a0d1cb
Rename to IncompatibleTargetChecker.java
philsc Jul 14, 2022
572e32c
auto format
philsc Jul 14, 2022
29b5bc2
Clean up a bit
philsc Jul 15, 2022
8137194
Merge remote-tracking branch 'upstream/master' into HEAD
philsc Jul 15, 2022
2e159b6
Simplify fake visibility
philsc Jul 20, 2022
27a732c
Address code review comments
philsc Jul 20, 2022
3728d99
Merge remote-tracking branch 'upstream/master' into HEAD
philsc Jul 20, 2022
16f098b
Incorporate more feedback
philsc Jul 20, 2022
21a5fd1
Merge remote-tracking branch 'upstream/master' into HEAD
philsc Aug 4, 2022
d0974ca
Reference the bug report
philsc Aug 4, 2022
607ab12
format
philsc Aug 4, 2022
ab7bdae
add comment
philsc Aug 4, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions site/en/docs/platforms.md
Original file line number Diff line number Diff line change
Expand Up @@ -245,3 +245,8 @@ def format(target):

$ bazel cquery //... --output=starlark --starlark:file=example.cquery
```

### Known Issues

Incompatible targets [ignore visibility
restrictions](https://github.com/bazelbuild/bazel/issues/16044).
35 changes: 35 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -2208,6 +2208,41 @@ java_library(
],
)

java_library(
name = "constraints/incompatible_target_checker",
srcs = ["constraints/IncompatibleTargetChecker.java"],
deps = [
":analysis_cluster",
":file_provider",
":incompatible_platform_provider",
":test/test_configuration",
":transitive_info_provider_map_builder",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/analysis/platform",
"//src/main/java/com/google/devtools/build/lib/analysis/platform:utils",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:config/config_conditions",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value",
"//src/main/java/com/google/devtools/build/lib/analysis:dependency",
"//src/main/java/com/google/devtools/build/lib/analysis:dependency_kind",
"//src/main/java/com/google/devtools/build/lib/analysis:target_and_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_collection",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper",
"//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
"//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key",
"//src/main/java/com/google/devtools/build/lib/skyframe:rule_configured_target_value",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/skyframe",
"//third_party:auto_value",
"//third_party:guava",
],
)

# TODO(b/144899336): This should be analysis/starlark/BUILD
java_library(
name = "starlark/args",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -403,8 +403,8 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
.add(
attr("distribs", DISTRIBUTIONS)
.nonconfigurable("Used in core loading phase logic with no access to configs"))
// Any rule that has provides its own meaning for the "target_compatible_with" attribute
// has to be excluded in `RuleContextConstraintSemantics.incompatibleConfiguredTarget()`.
// Any rule that provides its own meaning for the "target_compatible_with" attribute
// has to be excluded in `IncompatibleTargetChecker`.
.add(
attr(RuleClass.TARGET_COMPATIBLE_WITH_ATTR, LABEL_LIST)
.mandatoryProviders(ConstraintValueInfo.PROVIDER.id())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import com.google.devtools.build.lib.analysis.configuredtargets.OutputFileConfiguredTarget;
import com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
import com.google.devtools.build.lib.analysis.constraints.RuleContextConstraintSemantics;
import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleConfiguredTargetUtil;
import com.google.devtools.build.lib.analysis.test.AnalysisFailure;
import com.google.devtools.build.lib.analysis.test.AnalysisFailureInfo;
Expand Down Expand Up @@ -325,12 +324,6 @@ private ConfiguredTarget createRule(
return erroredConfiguredTarget(ruleContext);
}

ConfiguredTarget incompatibleTarget =
RuleContextConstraintSemantics.incompatibleConfiguredTarget(ruleContext, prerequisiteMap);
if (incompatibleTarget != null) {
return incompatibleTarget;
}

try {
Class<?> missingFragmentClass = null;
for (Class<? extends Fragment> fragmentClass :
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@
import com.google.devtools.build.lib.analysis.config.FragmentCollection;
import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
import com.google.devtools.build.lib.analysis.constraints.RuleContextConstraintSemantics;
import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleContext;
Expand Down Expand Up @@ -2180,14 +2179,6 @@ private static boolean checkRuleDependencyMandatoryProviders(

private void validateDirectPrerequisite(
Attribute attribute, ConfiguredTargetAndData prerequisite) {
if (RuleContextConstraintSemantics.checkForIncompatibility(prerequisite.getConfiguredTarget())
.isIncompatible()) {
// If the prerequisite is incompatible (e.g. has an incompatible provider), we pretend that
// there is no further validation needed. Otherwise, it would be difficult to make the
// incompatible target satisfy things like required providers and file extensions.
return;
}

validateDirectPrerequisiteType(prerequisite, attribute);
validateDirectPrerequisiteFileTypes(prerequisite, attribute);
if (attribute.performPrereqValidatorCheck()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -448,22 +448,6 @@ public static RunfilesSupport withExecutable(
computeActionEnvironment(ruleContext));
}

/**
* Creates and returns a {@link RunfilesSupport} object for the given rule and executable. This
* version discards all arguments. Only use this for <a
* href="https://bazel.build/docs/platforms#skipping-incompatible-targets">Incompatible Target
* Skipping</a>.
*/
public static RunfilesSupport withExecutableButNoArgs(
RuleContext ruleContext, Runfiles runfiles, Artifact executable) {
return RunfilesSupport.create(
ruleContext,
executable,
runfiles,
CommandLine.EMPTY,
computeActionEnvironment(ruleContext));
}

private static CommandLine computeArgs(RuleContext ruleContext, CommandLine additionalArgs) {
if (!ruleContext.getRule().isAttrDefined("args", Type.STRING_LIST)) {
// Some non-_binary rules create RunfilesSupport instances; it is fine to not have an args
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.FileProvider;
import com.google.devtools.build.lib.analysis.FilesToRunProvider;
import com.google.devtools.build.lib.analysis.IncompatiblePlatformProvider;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.RunfilesProvider;
import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
Expand Down Expand Up @@ -155,6 +156,28 @@ public RuleConfiguredTarget(
}
}

/** Use this constructor for creating incompatible ConfiguredTarget instances. */
public RuleConfiguredTarget(
Label label,
BuildConfigurationKey configurationKey,
NestedSet<PackageGroupContents> visibility,
TransitiveInfoProviderMap providers,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
String ruleClassString) {
this(
label,
configurationKey,
visibility,
providers,
configConditions,
ImmutableSet.<ConfiguredTargetKey>of(),
ruleClassString,
ImmutableList.<ActionAnalysisMetadata>of(),
ImmutableMap.<Label, Artifact>of());

Preconditions.checkState(providers.get(IncompatiblePlatformProvider.PROVIDER) != null, label);
}

/** The configuration conditions that trigger this rule's configurable attributes. */
@Override
public ImmutableMap<Label, ConfigMatchingProvider> getConfigConditions() {
Expand Down
Loading