Skip to content

Commit

Permalink
Plumb transitive packages to RuleContext
Browse files Browse the repository at this point in the history
To fix #16124, we need to collect all the repos/repo mappings relevant for a runfiles directory and write that into a manifest. One way to do that is to utilize the existing `transitivePackagesForPackageRootResolution` thing that we collect in most cases.

This CL plumbs this information through to RuleContext (whereafter it can be used by RunfilesSupport to write a manifest), without any logic changes. The field `transitivePackagesForPackageRootResolution` is renamed to simply `transitivePackages` since it's now used for multiple purposes.

Closes #16278.

PiperOrigin-RevId: 474778287
Change-Id: I7158c8adacc0b1db7f7bdda3b621d49c1fdb491c
  • Loading branch information
Wyverald authored and copybara-github committed Sep 16, 2022
1 parent eb18166 commit 720dc5f
Show file tree
Hide file tree
Showing 14 changed files with 151 additions and 159 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,7 @@ public static OrderedSetMultimap<Dependency, ConfiguredAspect> resolveAspectDepe
result.put(dep, aspectValue.getConfiguredAspect());
if (transitivePackages != null) {
transitivePackages.addTransitive(
Preconditions.checkNotNull(
aspectValue.getTransitivePackagesForPackageRootResolution()));
Preconditions.checkNotNull(aspectValue.getTransitivePackages()));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,20 +35,20 @@ public final class AspectValue extends BasicActionLookupValue implements RuleCon
@Nullable private AspectKey key;
@Nullable private ConfiguredAspect configuredAspect;
// May be null either after clearing or because transitive packages are not tracked.
@Nullable private transient NestedSet<Package> transitivePackagesForPackageRootResolution;
@Nullable private transient NestedSet<Package> transitivePackages;

public AspectValue(
AspectKey key,
Aspect aspect,
Location location,
ConfiguredAspect configuredAspect,
NestedSet<Package> transitivePackagesForPackageRootResolution) {
NestedSet<Package> transitivePackages) {
super(configuredAspect.getActions());
this.key = key;
this.aspect = Preconditions.checkNotNull(aspect, location);
this.location = Preconditions.checkNotNull(location, aspect);
this.configuredAspect = Preconditions.checkNotNull(configuredAspect, location);
this.transitivePackagesForPackageRootResolution = transitivePackagesForPackageRootResolution;
this.transitivePackages = transitivePackages;
}

public ConfiguredAspect getConfiguredAspect() {
Expand Down Expand Up @@ -79,13 +79,13 @@ public void clear(boolean clearEverything) {
key = null;
configuredAspect = null;
}
transitivePackagesForPackageRootResolution = null;
transitivePackages = null;
}

@Nullable
@Override
public NestedSet<Package> getTransitivePackagesForPackageRootResolution() {
return transitivePackagesForPackageRootResolution;
public NestedSet<Package> getTransitivePackages() {
return transitivePackages;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,20 @@ public interface ConfiguredObjectValue extends NotComparableSkyValue {
ProviderCollection getConfiguredObject();

/**
* Returns the set of packages transitively loaded by this value. Must only be used for
* constructing the package -> source root map needed for some builds. If the caller has not
* specified that this map needs to be constructed (via the constructor argument in {@link
* Returns the set of packages transitively loaded by this value. Must only be used for:
*
* <ul>
* <li>constructing the package -> source root map needed for some builds, OR
* <li>building the repo mapping manifest for runfiles
* </ul>
*
* If the caller has not specified that this map needs to be constructed (via the constructor
* argument in {@link
* com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction#ConfiguredTargetFunction} or
* {@link com.google.devtools.build.lib.skyframe.AspectFunction#AspectFunction}), calling this
* will crash.
*/
NestedSet<Package> getTransitivePackagesForPackageRootResolution();
NestedSet<Package> getTransitivePackages();

/**
* Clears data from this value.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
import com.google.devtools.build.lib.packages.EnvironmentGroup;
import com.google.devtools.build.lib.packages.InputFile;
import com.google.devtools.build.lib.packages.OutputFile;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.PackageGroup;
import com.google.devtools.build.lib.packages.PackageGroupsRuleVisibility;
import com.google.devtools.build.lib.packages.PackageSpecification;
Expand Down Expand Up @@ -181,6 +182,7 @@ public ConfiguredTarget createConfiguredTarget(
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
ConfigConditions configConditions,
@Nullable ToolchainCollection<ResolvedToolchainContext> toolchainContexts,
@Nullable NestedSet<Package> transitivePackages,
ExecGroupCollection.Builder execGroupCollectionBuilder)
throws InterruptedException, ActionConflictException, InvalidExecGroupException,
AnalysisFailurePropagationException {
Expand All @@ -196,6 +198,7 @@ public ConfiguredTarget createConfiguredTarget(
prerequisiteMap,
configConditions,
toolchainContexts,
transitivePackages,
execGroupCollectionBuilder);
} finally {
CurrentRuleTracker.endConfiguredTarget();
Expand Down Expand Up @@ -290,6 +293,7 @@ private ConfiguredTarget createRule(
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
ConfigConditions configConditions,
@Nullable ToolchainCollection<ResolvedToolchainContext> toolchainContexts,
@Nullable NestedSet<Package> transitivePackages,
ExecGroupCollection.Builder execGroupCollectionBuilder)
throws InterruptedException, ActionConflictException, InvalidExecGroupException,
AnalysisFailurePropagationException {
Expand All @@ -316,6 +320,7 @@ private ConfiguredTarget createRule(
ruleClassProvider.getFragmentRegistry().getUniversalFragments(),
configConditions,
prerequisiteMap.values()))
.setTransitivePackagesForRunfileRepoMappingManifest(transitivePackages)
.build();

List<NestedSet<AnalysisFailure>> analysisFailures =
Expand Down Expand Up @@ -506,6 +511,7 @@ public ConfiguredAspect createAspect(
@Nullable ExecGroupCollection.Builder execGroupCollectionBuilder,
BuildConfigurationValue aspectConfiguration,
BuildConfigurationValue hostConfiguration,
@Nullable NestedSet<Package> transitivePackages,
AspectKeyCreator.AspectKey aspectKey)
throws InterruptedException, ActionConflictException, InvalidExecGroupException {
RuleContext ruleContext =
Expand Down Expand Up @@ -533,6 +539,7 @@ public ConfiguredAspect createAspect(
ruleClassProvider.getFragmentRegistry().getUniversalFragments(),
configConditions,
Iterables.concat(prerequisiteMap.values(), ImmutableList.of(associatedTarget))))
.setTransitivePackagesForRunfileRepoMappingManifest(transitivePackages)
.build();

// If allowing analysis failures, targets should be created as normal as possible, and errors
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
import com.google.devtools.build.lib.packages.Info;
import com.google.devtools.build.lib.packages.InputFile;
import com.google.devtools.build.lib.packages.OutputFile;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy;
import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
import com.google.devtools.build.lib.packages.RawAttributeMapper;
Expand Down Expand Up @@ -165,6 +166,7 @@ void validate(
@Nullable private final ToolchainCollection<ResolvedToolchainContext> toolchainContexts;
private final ExecGroupCollection execGroupCollection;
@Nullable private final RequiredConfigFragmentsProvider requiredConfigFragments;
@Nullable private final NestedSet<Package> transitivePackagesForRunfileRepoMappingManifest;
private final List<Expander> makeVariableExpanders = new ArrayList<>();

/** Map of exec group names to ActionOwners. */
Expand Down Expand Up @@ -222,6 +224,8 @@ private RuleContext(
this.toolchainContexts = builder.toolchainContexts;
this.execGroupCollection = execGroupCollection;
this.requiredConfigFragments = builder.requiredConfigFragments;
this.transitivePackagesForRunfileRepoMappingManifest =
builder.transitivePackagesForRunfileRepoMappingManifest;
this.starlarkThread = createStarlarkThread(builder.mutability); // uses above state
}

Expand Down Expand Up @@ -1285,6 +1289,17 @@ public RequiredConfigFragmentsProvider getRequiredConfigFragments() {
return merged == null ? requiredConfigFragments : merged.build();
}

/**
* Returns the set of transitive packages. This is only intended to be used to create the repo
* mapping manifest for the runfiles tree. Can be null if transitive packages are not tracked (see
* {@link
* com.google.devtools.build.lib.skyframe.SkyframeExecutor#shouldStoreTransitivePackagesInLoadingAndAnalysis}).
*/
@Nullable
public NestedSet<Package> getTransitivePackagesForRunfileRepoMappingManifest() {
return transitivePackagesForRunfileRepoMappingManifest;
}

private boolean isUserDefinedMakeVariable(String makeVariable) {
// User-defined make values may be set either in "--define foo=bar" or in a vardef in the rule's
// package. Both are equivalent for these purposes, since in both cases setting
Expand Down Expand Up @@ -1651,6 +1666,7 @@ public static final class Builder implements RuleErrorConsumer {
private ExecGroupCollection.Builder execGroupCollectionBuilder;
private ImmutableMap<String, String> rawExecProperties;
@Nullable private RequiredConfigFragmentsProvider requiredConfigFragments;
@Nullable private NestedSet<Package> transitivePackagesForRunfileRepoMappingManifest;

@VisibleForTesting
public Builder(
Expand Down Expand Up @@ -1877,6 +1893,12 @@ public Builder setRequiredConfigFragments(
return this;
}

@CanIgnoreReturnValue
public Builder setTransitivePackagesForRunfileRepoMappingManifest(
@Nullable NestedSet<Package> packages) {
this.transitivePackagesForRunfileRepoMappingManifest = packages;
return this;
}

/** Determines and returns a map from attribute name to list of configured targets. */
private ImmutableSortedKeyListMultimap<String, ConfiguredTargetAndData> createTargetMap() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public static Optional<RuleConfiguredTargetValue> createDirectlyIncompatibleTarg
ConfigConditions configConditions,
Environment env,
@Nullable PlatformInfo platformInfo,
NestedSetBuilder<Package> transitivePackagesForPackageRootResolution)
NestedSetBuilder<Package> transitivePackages)
throws InterruptedException {
Target target = targetAndConfiguration.getTarget();
Rule rule = target.getAssociatedRule();
Expand Down Expand Up @@ -157,7 +157,7 @@ public static Optional<RuleConfiguredTargetValue> createDirectlyIncompatibleTarg
IncompatiblePlatformProvider.incompatibleDueToConstraints(
platformInfo.label(), invalidConstraintValues),
rule.getRuleClass(),
transitivePackagesForPackageRootResolution));
transitivePackages));
}

/**
Expand All @@ -181,7 +181,7 @@ public static Optional<RuleConfiguredTargetValue> createIndirectlyIncompatibleTa
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> depValueMap,
ConfigConditions configConditions,
@Nullable PlatformInfo platformInfo,
NestedSetBuilder<Package> transitivePackagesForPackageRootResolution) {
NestedSetBuilder<Package> transitivePackages) {
Target target = targetAndConfiguration.getTarget();
Rule rule = target.getAssociatedRule();

Expand Down Expand Up @@ -209,7 +209,7 @@ public static Optional<RuleConfiguredTargetValue> createIndirectlyIncompatibleTa
configConditions,
IncompatiblePlatformProvider.incompatibleDueToTargets(platformLabel, incompatibleDeps),
rule.getRuleClass(),
transitivePackagesForPackageRootResolution));
transitivePackages));
}

/** Creates an incompatible target. */
Expand All @@ -219,7 +219,7 @@ private static RuleConfiguredTargetValue createIncompatibleRuleConfiguredTarget(
ConfigConditions configConditions,
IncompatiblePlatformProvider incompatiblePlatformProvider,
String ruleClassString,
NestedSetBuilder<Package> transitivePackagesForPackageRootResolution) {
NestedSetBuilder<Package> transitivePackages) {
// Create dummy instances of the necessary data for a configured target. None of this data will
// actually be used because actions associated with incompatible targets must not be evaluated.
NestedSet<Artifact> filesToBuild = NestedSetBuilder.emptySet(Order.STABLE_ORDER);
Expand Down Expand Up @@ -248,10 +248,7 @@ private static RuleConfiguredTargetValue createIncompatibleRuleConfiguredTarget(
configConditions.asProviders(),
ruleClassString);
return new RuleConfiguredTargetValue(
configuredTarget,
transitivePackagesForPackageRootResolution == null
? null
: transitivePackagesForPackageRootResolution.build());
configuredTarget, transitivePackages == null ? null : transitivePackages.build());
}

/**
Expand Down
Loading

0 comments on commit 720dc5f

Please sign in to comment.