Skip to content

Commit

Permalink
[6.3.0] rules_go & rules_python are failing in Downstream CI with Baz…
Browse files Browse the repository at this point in the history 
…el@HEAD (#18447)

* Store repospec instead of package for module extension

PiperOrigin-RevId: 529679264
Change-Id: Ib9d410f5de1bac171c3c6f642b8584986181e44b

* Stop checking .bzl files visibility in RepoRuleFunction
Fixes #18346

At this point we are creating a rule from an extension repospec and that is always public

PiperOrigin-RevId: 532558852
Change-Id: I245de0a23f92ba6283be4bf3be6af4863b7a8c68

---------

Co-authored-by: salma-samy <[email protected]>
  • Loading branch information
@iancha1992 @SalmaSamy
iancha1992 and SalmaSamy authored May 19, 2023
1 parent e023bd3 commit 71cfbba
Show file tree
Hide file tree
Showing 6 changed files with 53 additions and 41 deletions.
37 changes: 20 additions & 17 deletions .../com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java
View file
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));
}
}
10 changes: 5 additions & 5 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java
View file
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
9 changes: 5 additions & 4 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalValue.java
View file
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
9 changes: 5 additions & 4 deletions src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java
View file
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
26 changes: 16 additions & 10 deletions src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
View file
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
3 changes: 2 additions & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
View file
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

0 comments on commit 71cfbba

Please sign in to comment.