-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Enforce visibility on select() keys.
Example:
my_rule(
name = "buildme",
deps = select({
"//other/package:some_config": [":mydeps"]
}))
Today, //other/package:some_config is exempt from visibility checking, even
though it's technically a target dep of "buildme". While this dep is "special"
vs. other deps in various ways, there's no obvious reason why it needs to
be special in this way. It adds an unclear corner case exception to
visibility's API.
Implementation:
---------------
select() keys are not "normal" dependencies and don't generally follow the same code path. Hence them not being automatically visibility checked like others.
In particular, normal dependencies are found in ConfiguredTargetFunction and validity-checked in RuleContext.Builder. select() keys' only purpose is to figure out which other normal dependencies should exist. So there's generally no need to pass them to RuleContext.Builder. Instead, Blaze passes their ConfigMatchingProviders, which remain useful for analysis phase attribute lookups.
RuleContext.Builder needs a ConfiguredTargetAndData to do validity-checking. This patch passes that information along for select() keys too.
We could alternatively refactor the validity checking logic. But that's an even more invasive change. Or do ad hoc validity checking directly in ConfiguredTargetFunction. But that's duplicating logic we really don't want to keep consistent.
Backward compatibility:
-----------------------
This has a very good chance of breaking builds, since all targets default to private visibility and it's unlikely existing config_settings have changed that. Pushing this forward requires an incompatible change flag.
PiperOrigin-RevId: 353131136
Change-Id: Ia5056745d135732bd35ba0ab099800d982f33f74- Loading branch information
1 parent
cc3a922
commit 70b15cb
Showing
10 changed files
with
146 additions
and
56 deletions.
There are no files selected for viewing
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 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 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
81 changes: 81 additions & 0 deletions
81
src/main/java/com/google/devtools/build/lib/analysis/config/ConfigConditions.java
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| // Copyright 2021 The Bazel Authors. All rights reserved. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| package com.google.devtools.build.lib.analysis.config; | ||
|
|
||
| import com.google.auto.value.AutoValue; | ||
| import com.google.common.collect.ImmutableMap; | ||
| import com.google.devtools.build.lib.analysis.ConfiguredTarget; | ||
| import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo; | ||
| import com.google.devtools.build.lib.analysis.platform.PlatformInfo; | ||
| import com.google.devtools.build.lib.cmdline.Label; | ||
| import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; | ||
|
|
||
| /** | ||
| * Utility class for temporarily tracking {@code select()} keys' {@link ConfigMatchingProvider}s and | ||
| * {@link ConfiguredTarget}s. | ||
| * | ||
| * <p>This is a utility class because its only purpose is to maintain {@link ConfiguredTarget} long | ||
| * enough for {@link RuleContext.Builder} to do prerequisite validation on it (like visibility | ||
| * checking). | ||
| * | ||
| * <p>Once {@link RuleContext} is instantiated, it should only have access to {@link | ||
| * ConfigMatchingProvider}, on the principle that providers are the correct interfaces for storing | ||
| * and sharing target metadata. {@link ConfiguredTarget} isn't meant to persist that long. | ||
| */ | ||
| @AutoValue | ||
| public abstract class ConfigConditions { | ||
| public abstract ImmutableMap<Label, ConfiguredTargetAndData> asConfiguredTargets(); | ||
|
|
||
| public abstract ImmutableMap<Label, ConfigMatchingProvider> asProviders(); | ||
|
|
||
| public static ConfigConditions create( | ||
| ImmutableMap<Label, ConfiguredTargetAndData> asConfiguredTargets, | ||
| ImmutableMap<Label, ConfigMatchingProvider> asProviders) { | ||
| return new AutoValue_ConfigConditions(asConfiguredTargets, asProviders); | ||
| } | ||
|
|
||
| public static ConfigConditions EMPTY = | ||
| ConfigConditions.create(ImmutableMap.of(), ImmutableMap.of()); | ||
|
|
||
| /** Exception for when a {@code select()} has an invalid key (like wrong target type). */ | ||
| public static class InvalidConditionException extends Exception {} | ||
|
|
||
| /** | ||
| * Returns a {@link ConfigMatchingProvider} from the given configured target if appropriate, else | ||
| * triggers a {@link InvalidConditionException}. | ||
| * | ||
| * <p>This is the canonical place to extract {@link ConfigMatchingProvider}s from configured | ||
| * targets. It's not as simple as {@link ConfiguredTarget#getProvider}. | ||
| */ | ||
| public static ConfigMatchingProvider fromConfiguredTarget( | ||
| ConfiguredTargetAndData selectKey, PlatformInfo targetPlatform) | ||
| throws InvalidConditionException { | ||
| ConfiguredTarget selectable = selectKey.getConfiguredTarget(); | ||
| // The below handles config_setting (which natively provides ConfigMatchingProvider) and | ||
| // constraint_value (which needs a custom-built ConfigMatchingProvider). | ||
| ConfigMatchingProvider matchingProvider = selectable.getProvider(ConfigMatchingProvider.class); | ||
| ConstraintValueInfo constraintValueInfo = selectable.get(ConstraintValueInfo.PROVIDER); | ||
| if (matchingProvider != null) { | ||
| return matchingProvider; | ||
| } | ||
| if (constraintValueInfo != null && targetPlatform != null) { | ||
| // If platformInfo == null, that means the owning target doesn't invoke toolchain | ||
| // resolution, in which case depending on a constraint_value is nonsensical. | ||
| return constraintValueInfo.configMatchingProvider(targetPlatform); | ||
| } | ||
|
|
||
| // Not a valid provider for configuration conditions. | ||
| throw new InvalidConditionException(); | ||
| } | ||
| } |
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 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
Oops, something went wrong.