From 969c102de5c48b2238d514afbf2b876e988b9e35 Mon Sep 17 00:00:00 2001 From: salma-samy Date: Fri, 5 May 2023 04:14:58 -0700 Subject: [PATCH 1/2] Store repospec instead of package for module extension PiperOrigin-RevId: 529679264 Change-Id: Ib9d410f5de1bac171c3c6f642b8584986181e44b --- ...uleExtensionEvalStarlarkThreadContext.java | 37 ++++++++++--------- .../bzlmod/SingleExtensionEvalFunction.java | 10 ++--- .../bzlmod/SingleExtensionEvalValue.java | 9 +++-- .../lib/skyframe/BzlmodRepoRuleFunction.java | 3 +- 4 files changed, 32 insertions(+), 27 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java index 457a474f4c4d99..9ffa35c394abb2 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java @@ -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; @@ -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); } } @@ -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 generatedRepos = new HashMap<>(); + private final Map generatedRepos = new HashMap<>(); public ModuleExtensionEvalStarlarkThreadContext( String repoPrefix, @@ -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 kwargs, RuleClass ruleClass) throws InterruptedException, EvalException { Object nameValue = kwargs.getOrDefault("name", Starlark.NONE); @@ -98,7 +92,7 @@ public void createRepo(StarlarkThread thread, Dict 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", @@ -116,8 +110,17 @@ public void createRepo(StarlarkThread thread, Dict 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 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()); } @@ -128,8 +131,8 @@ public void createRepo(StarlarkThread thread, Dict kwargs, RuleC * specified by the extension) of the repo, and the value is the package containing (only) the * repo rule. */ - public ImmutableMap getGeneratedRepos() { + public ImmutableMap getGeneratedRepoSpecs() { return ImmutableMap.copyOf( - Maps.transformValues(generatedRepos, PackageAndLocation::getPackage)); + Maps.transformValues(generatedRepos, RepoSpecAndLocation::getRepoSpec)); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java index 38360edb1fd724..b76131c9761da0 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java @@ -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) { @@ -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 repoImport : usage.getImports().entrySet()) { - if (!threadContext.getGeneratedRepos().containsKey(repoImport.getValue())) { + if (!threadContext.getGeneratedRepoSpecs().containsKey(repoImport.getValue())) { throw new SingleExtensionEvalFunctionException( ExternalDepsException.withMessage( Code.BAD_MODULE, @@ -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 -> diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalValue.java index d46ad803fd6ed6..d8f6f631420f70 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalValue.java @@ -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 getGeneratedRepos(); + public abstract ImmutableMap getGeneratedRepoSpecs(); /** * Returns the mapping from the canonical repo names of the repos generated by this extension to @@ -46,9 +46,10 @@ public abstract class SingleExtensionEvalValue implements SkyValue { @AutoCodec.Instantiator public static SingleExtensionEvalValue create( - ImmutableMap generatedRepos, + ImmutableMap generatedRepoSpecs, ImmutableBiMap canonicalRepoNameToInternalNames) { - return new AutoValue_SingleExtensionEvalValue(generatedRepos, canonicalRepoNameToInternalNames); + return new AutoValue_SingleExtensionEvalValue( + generatedRepoSpecs, canonicalRepoNameToInternalNames); } public static Key key(ModuleExtensionId id) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java index a7c5c48e0357b4..7d51708fca58e6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java @@ -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()); From c630a7c5303d1472ef9605d84cef33debeca00a5 Mon Sep 17 00:00:00 2001 From: salma-samy Date: Tue, 16 May 2023 13:32:46 -0700 Subject: [PATCH 2/2] Stop checking .bzl files visibility in RepoRuleFunction Fixes https://github.com/bazelbuild/bazel/issues/18346 At this point we are creating a rule from an extension repospec and that is always public PiperOrigin-RevId: 532558852 Change-Id: I245de0a23f92ba6283be4bf3be6af4863b7a8c68 --- .../lib/skyframe/BzlmodRepoRuleFunction.java | 6 ++--- .../build/lib/skyframe/PackageFunction.java | 26 ++++++++++++------- .../lib/skyframe/WorkspaceFileFunction.java | 3 ++- 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java index 7d51708fca58e6..dfaf1c83d19a33 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java @@ -239,8 +239,7 @@ private ImmutableMap 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, @@ -248,7 +247,8 @@ private ImmutableMap loadBzlModules( programLoads, keys, starlarkSemantics, - null); + null, + /* checkVisibility= */ false); } catch (NoSuchPackageException e) { throw new BzlmodRepoRuleFunctionException(e, Transience.PERSISTENT); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java index 4e8f3732eb466a..3977631986e5fb 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java @@ -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 loadBzlModules( Environment env, @@ -644,7 +645,8 @@ static ImmutableMap loadBzlModules( List> programLoads, List keys, StarlarkSemantics semantics, - @Nullable BzlLoadFunction bzlLoadFunctionForInlining) + @Nullable BzlLoadFunction bzlLoadFunctionForInlining, + boolean checkVisibility) throws NoSuchPackageException, InterruptedException { List bzlLoads; try { @@ -657,14 +659,17 @@ static ImmutableMap 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() @@ -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); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java index 768586cb067543..83b7c0b994fe2f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java @@ -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); }