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

[DRAFT] plumb transitive packages to RuleContext #16278

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
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