Skip to content

Commit

Permalink
Fix Label() behavior when called with '@repo' parts
Browse files Browse the repository at this point in the history
The Label() constructor reads its repo mapping from BazelStarlarkContext, which is constructed when the thread is created. This is wrong because the repo mapping it needs is actually the one of the repo where the .bzl file lives. So we do that by having BazelModuleContext store the repo mapping (similar to how it stores the label right now) and return that. Callers who want the repo mapping (not just Label()) now get it from the topmost stack frame rather than from the BazelStarlarkContext, which no longer carries a repo mapping.

This coincidentally also fixes Label() behavior when called from a Bzlmod environment (either extension impl functions, or repo rules; see #13316), because we simply don't have a BazelStarlarkContext in those cases.

Fixes #12963.

PiperOrigin-RevId: 401732211
  • Loading branch information
Wyverald authored and copybara-github committed Oct 8, 2021
1 parent 7cf0c34 commit 463e8c8
Show file tree
Hide file tree
Showing 17 changed files with 116 additions and 69 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
import com.google.devtools.build.lib.analysis.starlark.StarlarkModules;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.graph.Digraph;
import com.google.devtools.build.lib.graph.Node;
import com.google.devtools.build.lib.packages.BazelStarlarkContext;
Expand Down Expand Up @@ -860,13 +859,11 @@ public ImmutableMap<String, Object> getEnvironment() {
}

@Override
public void setStarlarkThreadContext(
StarlarkThread thread, Label fileLabel, RepositoryMapping repoMapping) {
public void setStarlarkThreadContext(StarlarkThread thread, Label fileLabel) {
new BazelStarlarkContext(
BazelStarlarkContext.Phase.LOADING,
toolsRepository,
configurationFragmentMap,
repoMapping,
/*convertedLabelsInPackage=*/ new HashMap<>(),
new SymbolGenerator<>(fileLabel),
/*analysisRuleLabel=*/ null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1127,7 +1127,6 @@ private StarlarkThread createStarlarkThread(Mutability mutability) {
BazelStarlarkContext.Phase.ANALYSIS,
ruleClassProvider.getToolsRepository(),
/*fragmentNameToClass=*/ null,
getTarget().getPackage().getRepositoryMapping(),
/*convertedLabelsInPackage=*/ new HashMap<>(),
getSymbolGenerator(),
getLabel(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,10 @@ public static StarlarkDefinedConfigTransition newRegularTransition(
StarlarkSemantics semantics,
Label parentLabel,
Location location,
BazelStarlarkContext starlarkContext)
RepositoryMapping repoMapping)
throws EvalException {
return new RegularTransition(
impl, inputs, outputs, semantics, parentLabel, location, starlarkContext.getRepoMapping());
impl, inputs, outputs, semantics, parentLabel, location, repoMapping);
}

public static StarlarkDefinedConfigTransition newAnalysisTestTransition(
Expand Down Expand Up @@ -350,7 +350,6 @@ public ImmutableMap<String, Map<String, Object>> evaluate(
Phase.ANALYSIS,
/*toolsRepository=*/ null,
/*fragmentNameToClass=*/ null,
repoMapping,
/*convertedLabelsInPackage=*/ new HashMap<>(),
dummySymbolGenerator,
parentLabel,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import com.google.devtools.build.lib.starlarkbuildapi.StarlarkAttrModuleApi;
import com.google.devtools.build.lib.util.FileType;
import com.google.devtools.build.lib.util.FileTypeSet;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -137,13 +138,12 @@ private static Attribute.Builder<?> createAttribute(
} else if (defaultValue instanceof StarlarkLateBoundDefault) {
builder.value((StarlarkLateBoundDefault) defaultValue); // unchecked cast
} else {
BazelStarlarkContext bazelStarlarkContext = BazelStarlarkContext.from(thread);
BazelModuleContext moduleContext =
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread));
builder.defaultValue(
defaultValue,
new BuildType.LabelConversionContext(
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)).label(),
bazelStarlarkContext.getRepoMapping(),
bazelStarlarkContext.getConvertedLabelsInPackage()),
moduleContext.label(), moduleContext.repoMapping(), new HashMap<>()),
DEFAULT_ARG);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -557,10 +557,12 @@ private static ImmutableList<Label> parseLabels(
Iterable<String> inputs, StarlarkThread thread, String adjective) throws EvalException {
ImmutableList.Builder<Label> parsedLabels = new ImmutableList.Builder<>();
BazelStarlarkContext bazelStarlarkContext = BazelStarlarkContext.from(thread);
BazelModuleContext moduleContext =
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread));
LabelConversionContext context =
new LabelConversionContext(
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)).label(),
bazelStarlarkContext.getRepoMapping(),
moduleContext.label(),
moduleContext.repoMapping(),
bazelStarlarkContext.getConvertedLabelsInPackage());
for (String input : inputs) {
try {
Expand Down Expand Up @@ -958,8 +960,6 @@ public boolean isImmutable() {

@Override
public Label label(String labelString, StarlarkThread thread) throws EvalException {
BazelStarlarkContext context = BazelStarlarkContext.from(thread);

// This function is surprisingly complex.
//
// - The logic to find the "current repo" is rather magical, using dynamic scope:
Expand All @@ -984,16 +984,14 @@ public Label label(String labelString, StarlarkThread thread) throws EvalExcepti
// and cache without needing four allocations (parseAbsoluteLabel,
// getRelativeWithRemapping, getUnambiguousCanonicalForm, parseAbsoluteLabel
// in labelCache)

// This is the label of the innermost BUILD/.bzl file on the current call stack.
Label parentLabel =
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)).label();

BazelModuleContext moduleContext =
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread));
try {
LabelValidator.parseAbsoluteLabel(labelString);
labelString =
parentLabel
.getRelativeWithRemapping(labelString, context.getRepoMapping())
moduleContext
.label()
.getRelativeWithRemapping(labelString, moduleContext.repoMapping())
.getUnambiguousCanonicalForm();
return labelCache.get(labelString);
} catch (LabelValidator.BadLabelException | LabelSyntaxException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,12 @@ private Label transformKey(StarlarkThread starlarkThread, Object key) throws Eva
} else if (key instanceof String) {
try {
BazelStarlarkContext bazelStarlarkContext = BazelStarlarkContext.from(starlarkThread);
BazelModuleContext moduleContext =
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(starlarkThread));
LabelConversionContext context =
new LabelConversionContext(
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(starlarkThread))
.label(),
bazelStarlarkContext.getRepoMapping(),
moduleContext.label(),
moduleContext.repoMapping(),
bazelStarlarkContext.getConvertedLabelsInPackage());
return context.convert((String) key);
} catch (LabelSyntaxException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,6 @@ public RepositoryDirectoryValue.Builder fetch(
BazelStarlarkContext.Phase.LOADING, // ("fetch")
/*toolsRepository=*/ null,
/*fragmentNameToClass=*/ null,
rule.getPackage().getRepositoryMapping(),
/*convertedLabelsInPackage=*/ new HashMap<>(),
new SymbolGenerator<>(key),
/*analysisRuleLabel=*/ null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -548,17 +548,19 @@ public Label getLocalTargetLabel(String targetName) throws LabelSyntaxException
useStarlarkThread = true)
public Label getRelative(String relName, StarlarkThread thread) throws LabelSyntaxException {
HasRepoMapping hrm = thread.getThreadLocal(HasRepoMapping.class);
return getRelativeWithRemapping(relName, hrm.getRepoMapping());
return getRelativeWithRemapping(relName, hrm.getRepoMappingForCurrentBzlFile(thread));
}

/**
* An interface for retrieving a repository mapping.
* An interface for retrieving a repository mapping that's applicable for the repo containing the
* current .bzl file (more precisely, the .bzl file where the function at the innermost Starlark
* stack frame lives).
*
* <p>This has only a single implementation, {@code BazelStarlarkContext}, but we can't mention
* that type here because logically it belongs in Bazel, above this package.
*/
public interface HasRepoMapping {
RepositoryMapping getRepoMapping();
RepositoryMapping getRepoMappingForCurrentBzlFile(StarlarkThread thread);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import net.starlark.java.eval.Module;

/**
Expand All @@ -28,6 +29,9 @@ public abstract class BazelModuleContext {
/** Label associated with the Starlark {@link net.starlark.java.eval.Module}. */
public abstract Label label();

/** The repository mapping applicable to the repo where the .bzl file is located in. */
public abstract RepositoryMapping repoMapping();

/** Returns the name of the module's .bzl file, as provided to the parser. */
public abstract String filename();

Expand Down Expand Up @@ -64,9 +68,11 @@ public static BazelModuleContext of(Module m) {

public static BazelModuleContext create(
Label label,
RepositoryMapping repoMapping,
String filename,
ImmutableMap<String, Module> loads,
byte[] bzlTransitiveDigest) {
return new AutoValue_BazelModuleContext(label, filename, loads, bzlTransitiveDigest);
return new AutoValue_BazelModuleContext(
label, repoMapping, filename, loads, bzlTransitiveDigest);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.Optional;
import javax.annotation.Nullable;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Module;
import net.starlark.java.eval.Starlark;
import net.starlark.java.eval.StarlarkThread;

Expand Down Expand Up @@ -63,7 +64,6 @@ public void storeInThread(StarlarkThread thread) {
@Nullable private final String toolsRepository;
// Only necessary for loading phase threads to construct configuration_field.
@Nullable private final ImmutableMap<String, Class<?>> fragmentNameToClass;
private final RepositoryMapping repoMapping;
private final HashMap<String, Label> convertedLabelsInPackage;
private final SymbolGenerator<?> symbolGenerator;
@Nullable private final Label analysisRuleLabel;
Expand All @@ -77,7 +77,6 @@ public void storeInThread(StarlarkThread thread) {
* @param fragmentNameToClass a map from configuration fragment name to configuration fragment
* class, such as "apple" to AppleConfiguration.class for loading phase threads, null for
* other threads.
* @param repoMapping a map from RepositoryName to RepositoryName to be used for external
* @param convertedLabelsInPackage a mutable map from String to Label, used during package loading
* of a single package.
* @param symbolGenerator a {@link SymbolGenerator} to be used when creating objects to be
Expand All @@ -90,22 +89,19 @@ public void storeInThread(StarlarkThread thread) {
// separate structs, exactly one of which is populated (plus the common fields). And eliminate
// StarlarkUtils.Phase.
// TODO(adonovan): move PackageFactory.PackageContext in here, for loading-phase threads.
// TODO(adonovan): add a PackageIdentifier here, for use by the Starlark Label function.
// TODO(adonovan): is there any reason not to put the entire RuleContext in this thread, for
// analysis threads?
public BazelStarlarkContext(
Phase phase,
@Nullable String toolsRepository,
@Nullable ImmutableMap<String, Class<?>> fragmentNameToClass,
RepositoryMapping repoMapping,
HashMap<String, Label> convertedLabelsInPackage,
SymbolGenerator<?> symbolGenerator,
@Nullable Label analysisRuleLabel,
@Nullable Label networkAllowlistForTests) {
this.phase = Preconditions.checkNotNull(phase);
this.toolsRepository = toolsRepository;
this.fragmentNameToClass = fragmentNameToClass;
this.repoMapping = repoMapping;
this.convertedLabelsInPackage = convertedLabelsInPackage;
this.symbolGenerator = Preconditions.checkNotNull(symbolGenerator);
this.analysisRuleLabel = analysisRuleLabel;
Expand All @@ -130,8 +126,11 @@ public ImmutableMap<String, Class<?>> getFragmentNameToClass() {
* in the BUILD files and the values are new repository names chosen by the main repository.
*/
@Override
public RepositoryMapping getRepoMapping() {
return repoMapping;
public RepositoryMapping getRepoMappingForCurrentBzlFile(StarlarkThread thread) {
// TODO(b/200024947): Find a better place for this. We don't want Label to have to depend on
// StarlarkModuleContext, but having the logic in BazelStarlarkContext is purely a historical
// misstep.
return BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)).repoMapping();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,6 @@ private void executeBuildFileImpl(
BazelStarlarkContext.Phase.LOADING,
ruleClassProvider.getToolsRepository(),
/*fragmentNameToClass=*/ null,
pkgBuilder.getRepositoryMapping(),
pkgBuilder.getConvertedLabelsInPackage(),
new SymbolGenerator<>(pkgBuilder.getPackageIdentifier()),
/*analysisRuleLabel=*/ null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.packages.RuleClass.Builder.ThirdPartyLicenseExistencePolicy;
import com.google.devtools.build.lib.vfs.Root;
import java.util.Map;
Expand Down Expand Up @@ -65,9 +64,8 @@ public interface RuleClassProvider extends RuleDefinitionEnvironment {
*
* @param thread StarlarkThread in which to store the context.
* @param label the label of the .bzl file
* @param repoMapping map of RepositoryNames to be remapped
*/
void setStarlarkThreadContext(StarlarkThread thread, Label label, RepositoryMapping repoMapping);
void setStarlarkThreadContext(StarlarkThread thread, Label label);

/**
* Returns all the predeclared top-level symbols (for .bzl files) that belong to native rule sets,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSortedSet;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.NullEventHandler;
Expand Down Expand Up @@ -138,7 +137,6 @@ public void execute(
BazelStarlarkContext.Phase.WORKSPACE,
/*toolsRepository=*/ null,
/*fragmentNameToClass=*/ null,
/*repoMapping=*/ RepositoryMapping.ALWAYS_FALLBACK,
/*convertedLabelsInPackage=*/ new HashMap<>(),
new SymbolGenerator<>(workspaceFileKey),
/*analysisRuleLabel=*/ null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@
import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.Settings;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.packages.BazelModuleContext;
import com.google.devtools.build.lib.packages.BazelStarlarkContext;
import com.google.devtools.build.lib.starlarkbuildapi.config.ConfigGlobalLibraryApi;
import com.google.devtools.build.lib.starlarkbuildapi.config.ConfigurationTransitionApi;
import java.util.HashSet;
Expand Down Expand Up @@ -57,12 +55,17 @@ public ConfigurationTransitionApi transition(
List<String> outputsList = Sequence.cast(outputs, String.class, "outputs");
validateBuildSettingKeys(inputsList, Settings.INPUTS);
validateBuildSettingKeys(outputsList, Settings.OUTPUTS);
Label parentLabel =
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)).label();
BazelModuleContext moduleContext =
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread));
Location location = thread.getCallerLocation();
BazelStarlarkContext context = BazelStarlarkContext.from(thread);
return StarlarkDefinedConfigTransition.newRegularTransition(
implementation, inputsList, outputsList, semantics, parentLabel, location, context);
implementation,
inputsList,
outputsList,
semantics,
moduleContext.label(),
location,
moduleContext.repoMapping());
}

@Override
Expand All @@ -73,12 +76,11 @@ public ConfigurationTransitionApi analysisTestTransition(
Map<String, Object> changedSettingsMap =
Dict.cast(changedSettings, String.class, Object.class, "changed_settings dict");
validateBuildSettingKeys(changedSettingsMap.keySet(), Settings.OUTPUTS);
RepositoryMapping repoMapping = BazelStarlarkContext.from(thread).getRepoMapping();
Label parentLabel =
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)).label();
BazelModuleContext moduleContext =
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread));
Location location = thread.getCallerLocation();
return StarlarkDefinedConfigTransition.newAnalysisTestTransition(
changedSettingsMap, repoMapping, parentLabel, location);
changedSettingsMap, moduleContext.repoMapping(), moduleContext.label(), location);
}

private void validateBuildSettingKeys(Iterable<String> optionKeys, Settings keyErrorDescriptor)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -788,18 +788,16 @@ private BzlLoadValue computeInternalWithCompiledBzl(
Module module = Module.withPredeclared(builtins.starlarkSemantics, predeclared);
module.setClientData(
BazelModuleContext.create(
key.getLabel(), prog.getFilename(), ImmutableMap.copyOf(loadMap), transitiveDigest));
key.getLabel(),
repoMapping,
prog.getFilename(),
ImmutableMap.copyOf(loadMap),
transitiveDigest));

// executeBzlFile may post events to the Environment's handler, but events do not matter when
// caching BzlLoadValues. Note that executing the code mutates the module.
executeBzlFile(
prog,
key.getLabel(),
module,
loadMap,
builtins.starlarkSemantics,
env.getListener(),
repoMapping);
prog, key.getLabel(), module, loadMap, builtins.starlarkSemantics, env.getListener());
return new BzlLoadValue(module, transitiveDigest);
}

Expand Down Expand Up @@ -1073,17 +1071,14 @@ private void executeBzlFile(
Module module,
Map<String, Module> loadedModules,
StarlarkSemantics starlarkSemantics,
ExtendedEventHandler skyframeEventHandler,
RepositoryMapping repositoryMapping)
ExtendedEventHandler skyframeEventHandler)
throws BzlLoadFailedException, InterruptedException {
try (Mutability mu = Mutability.create("loading", label)) {
StarlarkThread thread = new StarlarkThread(mu, starlarkSemantics);
thread.setLoader(loadedModules::get);
StoredEventHandler starlarkEventHandler = new StoredEventHandler();
thread.setPrintHandler(Event.makeDebugPrintHandler(starlarkEventHandler));
packageFactory
.getRuleClassProvider()
.setStarlarkThreadContext(thread, label, repositoryMapping);
packageFactory.getRuleClassProvider().setStarlarkThreadContext(thread, label);
execAndExport(prog, label, starlarkEventHandler, module, thread);

Event.replayEventsOn(skyframeEventHandler, starlarkEventHandler.getEvents());
Expand Down
Loading

0 comments on commit 463e8c8

Please sign in to comment.