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

[6.3.0] rules_go & rules_python are failing in Downstream CI with Bazel@HEAD #18447

Merged
Merged
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 @@ -24,7 +24,6 @@
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleFactory.InvalidRuleException;
Expand Down Expand Up @@ -53,14 +52,14 @@ public static ModuleExtensionEvalStarlarkThreadContext from(StarlarkThread threa
}

@AutoValue
abstract static class PackageAndLocation {
abstract Package getPackage();
abstract static class RepoSpecAndLocation {
abstract RepoSpec getRepoSpec();

abstract Location getLocation();

static PackageAndLocation create(Package pkg, Location location) {
return new AutoValue_ModuleExtensionEvalStarlarkThreadContext_PackageAndLocation(
pkg, location);
static RepoSpecAndLocation create(RepoSpec repoSpec, Location location) {
return new AutoValue_ModuleExtensionEvalStarlarkThreadContext_RepoSpecAndLocation(
repoSpec, location);
}
}

Expand All @@ -69,7 +68,7 @@ static PackageAndLocation create(Package pkg, Location location) {
private final RepositoryMapping repoMapping;
private final BlazeDirectories directories;
private final ExtendedEventHandler eventHandler;
private final Map<String, PackageAndLocation> generatedRepos = new HashMap<>();
private final Map<String, RepoSpecAndLocation> generatedRepos = new HashMap<>();

public ModuleExtensionEvalStarlarkThreadContext(
String repoPrefix,
Expand All @@ -84,11 +83,6 @@ public ModuleExtensionEvalStarlarkThreadContext(
this.eventHandler = eventHandler;
}

/** The string that should be prefixed to the names of all repos generated by this extension. */
public String getRepoPrefix() {
return repoPrefix;
}

public void createRepo(StarlarkThread thread, Dict<String, Object> kwargs, RuleClass ruleClass)
throws InterruptedException, EvalException {
Object nameValue = kwargs.getOrDefault("name", Starlark.NONE);
Expand All @@ -98,7 +92,7 @@ public void createRepo(StarlarkThread thread, Dict<String, Object> kwargs, RuleC
}
String name = (String) nameValue;
RepositoryName.validateUserProvidedRepoName(name);
PackageAndLocation conflict = generatedRepos.get(name);
RepoSpecAndLocation conflict = generatedRepos.get(name);
if (conflict != null) {
throw Starlark.errorf(
"A repo named %s is already generated by this module extension at %s",
Expand All @@ -116,8 +110,17 @@ public void createRepo(StarlarkThread thread, Dict<String, Object> kwargs, RuleC
"RepositoryRuleFunction.createRule",
ruleClass,
Maps.transformEntries(kwargs, (k, v) -> k.equals("name") ? prefixedName : v));
generatedRepos.put(
name, PackageAndLocation.create(rule.getPackage(), thread.getCallerLocation()));

Map<String, Object> attributes = Maps.transformEntries(kwargs, (k, v) -> rule.getAttr(k));
String bzlFile = ruleClass.getRuleDefinitionEnvironmentLabel().getUnambiguousCanonicalForm();
RepoSpec repoSpec =
RepoSpec.builder()
.setBzlFile(bzlFile)
.setRuleClassName(ruleClass.getName())
.setAttributes(AttributeValues.create(attributes))
.build();

generatedRepos.put(name, RepoSpecAndLocation.create(repoSpec, thread.getCallerLocation()));
} catch (InvalidRuleException | NoSuchPackageException e) {
throw Starlark.errorf("%s", e.getMessage());
}
Expand All @@ -128,8 +131,8 @@ public void createRepo(StarlarkThread thread, Dict<String, Object> kwargs, RuleC
* specified by the extension) of the repo, and the value is the package containing (only) the
* repo rule.
*/
public ImmutableMap<String, Package> getGeneratedRepos() {
public ImmutableMap<String, RepoSpec> getGeneratedRepoSpecs() {
return ImmutableMap.copyOf(
Maps.transformValues(generatedRepos, PackageAndLocation::getPackage));
Maps.transformValues(generatedRepos, RepoSpecAndLocation::getRepoSpec));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
ModuleExtensionMetadata metadata = (ModuleExtensionMetadata) returnValue;
metadata.evaluate(
usagesValue.getExtensionUsages().values(),
threadContext.getGeneratedRepos().keySet(),
threadContext.getGeneratedRepoSpecs().keySet(),
env.getListener());
}
} catch (NeedsSkyframeRestartException e) {
Expand Down Expand Up @@ -228,7 +228,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
// Check that all imported repos have been actually generated
for (ModuleExtensionUsage usage : usagesValue.getExtensionUsages().values()) {
for (Entry<String, String> repoImport : usage.getImports().entrySet()) {
if (!threadContext.getGeneratedRepos().containsKey(repoImport.getValue())) {
if (!threadContext.getGeneratedRepoSpecs().containsKey(repoImport.getValue())) {
throw new SingleExtensionEvalFunctionException(
ExternalDepsException.withMessage(
Code.BAD_MODULE,
Expand All @@ -240,15 +240,15 @@ public SkyValue compute(SkyKey skyKey, Environment env)
repoImport.getKey(),
usage.getLocation(),
SpellChecker.didYouMean(
repoImport.getValue(), threadContext.getGeneratedRepos().keySet())),
repoImport.getValue(), threadContext.getGeneratedRepoSpecs().keySet())),
Transience.PERSISTENT);
}
}
}

return SingleExtensionEvalValue.create(
threadContext.getGeneratedRepos(),
threadContext.getGeneratedRepos().keySet().stream()
threadContext.getGeneratedRepoSpecs(),
threadContext.getGeneratedRepoSpecs().keySet().stream()
.collect(
toImmutableBiMap(
e ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@
public abstract class SingleExtensionEvalValue implements SkyValue {
/**
* Returns the repos generated by the extension. The key is the "internal name" (as specified by
* the extension) of the repo, and the value is the package containing (only) the repo rule.
* the extension) of the repo, and the value is the the repo specs that defins the repository .
*/
public abstract ImmutableMap<String, Package> getGeneratedRepos();
public abstract ImmutableMap<String, RepoSpec> getGeneratedRepoSpecs();

/**
* Returns the mapping from the canonical repo names of the repos generated by this extension to
Expand All @@ -46,9 +46,10 @@ public abstract class SingleExtensionEvalValue implements SkyValue {

@AutoCodec.Instantiator
public static SingleExtensionEvalValue create(
ImmutableMap<String, Package> generatedRepos,
ImmutableMap<String, RepoSpec> generatedRepoSpecs,
ImmutableBiMap<RepositoryName, String> canonicalRepoNameToInternalNames) {
return new AutoValue_SingleExtensionEvalValue(generatedRepos, canonicalRepoNameToInternalNames);
return new AutoValue_SingleExtensionEvalValue(
generatedRepoSpecs, canonicalRepoNameToInternalNames);
}

public static Key key(ModuleExtensionId id) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
if (internalRepo == null) {
return BzlmodRepoRuleValue.REPO_RULE_NOT_FOUND_VALUE;
}
Package pkg = extensionEval.getGeneratedRepos().get(internalRepo);
RepoSpec extRepoSpec = extensionEval.getGeneratedRepoSpecs().get(internalRepo);
Package pkg = createRuleFromSpec(extRepoSpec, starlarkSemantics, env).getRule().getPackage();
Preconditions.checkNotNull(pkg);

return new BzlmodRepoRuleValue(pkg, repositoryName.getName());
Expand Down Expand Up @@ -238,16 +239,16 @@ private ImmutableMap<String, Module> loadBzlModules(

// Load the .bzl module.
try {
// TODO(b/22193153, wyv): Determine whether .bzl load visibility should apply at all to this
// type of .bzl load. As it stands, this call checks that bzlFile is visible to package @//.
// No need to check visibility for an extension repospec that is always public
return PackageFunction.loadBzlModules(
env,
PackageIdentifier.EMPTY_PACKAGE_ID,
"Bzlmod system",
programLoads,
keys,
starlarkSemantics,
null);
null,
/* checkVisibility= */ false);
} catch (NoSuchPackageException e) {
throw new BzlmodRepoRuleFunctionException(e, Transience.PERSISTENT);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,7 @@ private static FileValue getBuildFileValue(Environment env, RootedPath buildFile
* construction to the caller, so that loadPrelude can become just a call to the factored-out
* code.
*/
// TODO(18422): Cleanup/refactor this method's signature.
@Nullable
static ImmutableMap<String, Module> loadBzlModules(
Environment env,
Expand All @@ -644,7 +645,8 @@ static ImmutableMap<String, Module> loadBzlModules(
List<Pair<String, Location>> programLoads,
List<BzlLoadValue.Key> keys,
StarlarkSemantics semantics,
@Nullable BzlLoadFunction bzlLoadFunctionForInlining)
@Nullable BzlLoadFunction bzlLoadFunctionForInlining,
boolean checkVisibility)
throws NoSuchPackageException, InterruptedException {
List<BzlLoadValue> bzlLoads;
try {
Expand All @@ -657,14 +659,17 @@ static ImmutableMap<String, Module> loadBzlModules(
}
// Validate that the current BUILD/WORKSPACE file satisfies each loaded dependency's
// load visibility.
BzlLoadFunction.checkLoadVisibilities(
packageId,
requestingFileDescription,
bzlLoads,
keys,
programLoads,
/*demoteErrorsToWarnings=*/ !semantics.getBool(BuildLanguageOptions.CHECK_BZL_VISIBILITY),
env.getListener());
if (checkVisibility) {
BzlLoadFunction.checkLoadVisibilities(
packageId,
requestingFileDescription,
bzlLoads,
keys,
programLoads,
/* demoteErrorsToWarnings= */ !semantics.getBool(
BuildLanguageOptions.CHECK_BZL_VISIBILITY),
env.getListener());
}
} catch (BzlLoadFailedException e) {
Throwable rootCause = Throwables.getRootCause(e);
throw PackageFunctionException.builder()
Expand Down Expand Up @@ -1325,7 +1330,8 @@ private LoadedPackage loadPackage(
programLoads,
keys.build(),
starlarkBuiltinsValue.starlarkSemantics,
bzlLoadFunctionForInlining);
bzlLoadFunctionForInlining,
/* checkVisibility= */ true);
} catch (NoSuchPackageException e) {
throw new PackageFunctionException(e, Transience.PERSISTENT);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
programLoads,
keys.build(),
starlarkSemantics,
bzlLoadFunctionForInlining);
bzlLoadFunctionForInlining,
/* checkVisibility= */ true);
} catch (NoSuchPackageException e) {
throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT);
}
Expand Down