From 76c8c3a35776ecd6c568cf4ef1b6f2d2f52c0280 Mon Sep 17 00:00:00 2001 From: ilist Date: Wed, 2 Feb 2022 08:44:04 -0800 Subject: [PATCH] Remove native proto_library implementation. PiperOrigin-RevId: 425894387 --- .../build/lib/actions/CommandLineItem.java | 9 - .../nestedset/NestedSetFingerprintCache.java | 13 - .../lib/rules/proto/BazelProtoLibrary.java | 60 --- .../rules/proto/BazelProtoLibraryRule.java | 25 +- .../build/lib/rules/proto/ProtoCommon.java | 395 ------------------ .../proto/ProtoCompileActionBuilder.java | 298 +------------ .../NestedSetFingerprintCacheTest.java | 7 - 7 files changed, 4 insertions(+), 803 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibrary.java diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java index 5580b1cd026ca8..7b983799f54bfe 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java @@ -57,15 +57,6 @@ abstract class ParametrizedMapFn implements MapFn { public abstract int maxInstancesAllowed(); } - /** - * Use this map function when your map function needs to capture per-rule information. - * - *

Use of this class prevents sharing sub-computations over shared NestedSets, since the map - * function is per-target. This will make your action key computations become O(N^2). Please avoid - * if possible. - */ - interface CapturingMapFn extends MapFn {} - /** Expands the object into the command line as a string. */ String expandToCommandLine(); diff --git a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCache.java b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCache.java index 35aa803cfdcbc3..48f238a2332912 100644 --- a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCache.java +++ b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCache.java @@ -19,7 +19,6 @@ import com.google.common.collect.Multiset; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.CommandLineItem; -import com.google.devtools.build.lib.actions.CommandLineItem.MapFn; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.DigestHashFunction; import java.util.HashSet; @@ -45,10 +44,6 @@ public void addNestedSetToFingerprint(Fingerprint fingerprint, NestedSet public void addNestedSetToFingerprint( CommandLineItem.MapFn mapFn, Fingerprint fingerprint, NestedSet nestedSet) throws CommandLineExpansionException, InterruptedException { - if (mapFn instanceof CommandLineItem.CapturingMapFn) { - addNestedSetToFingerprintSlow(mapFn, fingerprint, nestedSet); - return; - } // Only top-level nested sets can be empty, so we can bail here if (nestedSet.isEmpty()) { fingerprint.addInt(EMPTY_SET_DIGEST); @@ -60,14 +55,6 @@ public void addNestedSetToFingerprint( addToFingerprint(mapFn, fingerprint, digestMap, children); } - private void addNestedSetToFingerprintSlow( - MapFn mapFn, Fingerprint fingerprint, NestedSet nestedSet) - throws CommandLineExpansionException, InterruptedException { - for (T object : nestedSet.toList()) { - addToFingerprint(mapFn, fingerprint, object); - } - } - public void clear() { mapFnToDigestMap = createMap(); seenMapFns.clear(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibrary.java deleted file mode 100644 index 63e33fefaf9b16..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibrary.java +++ /dev/null @@ -1,60 +0,0 @@ -// Copyright 2015 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.rules.proto; - -import static com.google.devtools.build.lib.collect.nestedset.Order.STABLE_ORDER; - -import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; -import com.google.devtools.build.lib.analysis.ConfiguredTarget; -import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder; -import com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory; -import com.google.devtools.build.lib.analysis.RuleContext; -import com.google.devtools.build.lib.analysis.Runfiles; -import com.google.devtools.build.lib.analysis.RunfilesProvider; -import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; -import com.google.devtools.build.lib.rules.proto.ProtoCompileActionBuilder.Services; - -/** The implementation of the proto_library rule. */ -public class BazelProtoLibrary implements RuleConfiguredTargetFactory { - - @Override - public ConfiguredTarget create(RuleContext ruleContext) - throws ActionConflictException, RuleErrorException, InterruptedException { - ProtoCommon.checkSourceFilesAreInSamePackage(ruleContext); - ProtoInfo protoInfo = - ProtoCommon.createProtoInfo( - ruleContext, - ruleContext.getFragment(ProtoConfiguration.class).generatedProtosInVirtualImports()); - if (ruleContext.hasErrors()) { - return null; - } - - ProtoCompileActionBuilder.writeDescriptorSet(ruleContext, protoInfo, Services.ALLOW); - - Runfiles dataRunfiles = - ProtoCommon.createDataRunfilesProvider(protoInfo.getTransitiveProtoSources(), ruleContext) - .addArtifact(protoInfo.getDirectDescriptorSet()) - .build(); - - RuleConfiguredTargetBuilder builder = - new RuleConfiguredTargetBuilder(ruleContext) - .setFilesToBuild( - NestedSetBuilder.create(STABLE_ORDER, protoInfo.getDirectDescriptorSet())) - .addProvider(RunfilesProvider.withData(Runfiles.EMPTY, dataRunfiles)) - .addNativeDeclaredProvider(protoInfo); - - return builder.build(); - } -} diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryRule.java b/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryRule.java index 00a7d0788d9a2a..68258531599453 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryRule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryRule.java @@ -15,46 +15,27 @@ package com.google.devtools.build.lib.rules.proto; import static com.google.devtools.build.lib.packages.Attribute.attr; -import static com.google.devtools.build.lib.packages.BuildType.LABEL; import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST; import static com.google.devtools.build.lib.packages.Type.STRING; import com.google.devtools.build.lib.analysis.BaseRuleClasses; import com.google.devtools.build.lib.analysis.RuleDefinition; import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment; -import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory; -import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.util.FileType; /** * Rule definition for the proto_library rule. + * + *

This rule is implemented in Starlark. This class remains only for doc-gen purposes. */ public final class BazelProtoLibraryRule implements RuleDefinition { - - private static final Label DEFAULT_PROTO_COMPILER = - Label.parseAbsoluteUnchecked("@com_google_protobuf//:protoc"); - private static final Attribute.LabelLateBoundDefault PROTO_COMPILER = - Attribute.LabelLateBoundDefault.fromTargetConfiguration( - ProtoConfiguration.class, - DEFAULT_PROTO_COMPILER, - (rule, attributes, protoConfig) -> - protoConfig.protoCompiler() != null - ? protoConfig.protoCompiler() - : DEFAULT_PROTO_COMPILER); - @Override public RuleClass build(RuleClass.Builder builder, final RuleDefinitionEnvironment env) { return builder .requiresConfigurationFragments(ProtoConfiguration.class) .setOutputToGenfiles() - .add( - attr(":proto_compiler", LABEL) - .cfg(ExecutionTransitionFactory.create()) - .exec() - .value(PROTO_COMPILER)) /* The list of other proto_library rules that the target depends upon. A proto_library may only depend on other @@ -114,7 +95,7 @@ public Metadata getMetadata() { return RuleDefinition.Metadata.builder() .name("proto_library") .ancestors(BaseRuleClasses.NativeActionCreatingRule.class) - .factoryClass(BazelProtoLibrary.class) + .factoryClass(BaseRuleClasses.EmptyRuleConfiguredTargetFactory.class) .build(); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java index 3825da8888e8fc..4758eb4c2a92d1 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java @@ -14,29 +14,18 @@ package com.google.devtools.build.lib.rules.proto; -import static com.google.devtools.build.lib.collect.nestedset.Order.STABLE_ORDER; -import static com.google.devtools.build.lib.packages.Type.STRING; - import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.Interner; -import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.analysis.RuleContext; -import com.google.devtools.build.lib.analysis.Runfiles; -import com.google.devtools.build.lib.analysis.actions.SymlinkAction; import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.StrictDepsMode; import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.collect.nestedset.NestedSet; -import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.packages.BazelModuleContext; -import com.google.devtools.build.lib.packages.BuildType; -import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.PathFragment; -import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; import net.starlark.java.eval.Module; import net.starlark.java.eval.Starlark; @@ -70,353 +59,6 @@ static PathFragment memoryEfficientProtoSourceRoot(PathFragment protoSourceRoot) return PROTO_SOURCE_ROOT_INTERNER.intern(protoSourceRoot); } - /** - * Gets the direct sources of a proto library. If protoSources is not empty, the value is just - * protoSources. Otherwise, it's the combined sources of all direct dependencies of the given - * RuleContext. - * - * @param sources the direct proto sources. - * @param deps the proto dependencies. - * @return the direct sources of a proto library. - */ - private static NestedSet computeStrictImportableProtosForDependents( - ImmutableList sources, ImmutableList deps) { - - if (sources.isEmpty()) { - /* a proxy/alias library, return the sources of the direct deps */ - NestedSetBuilder builder = NestedSetBuilder.stableOrder(); - for (ProtoInfo provider : deps) { - builder.addTransitive(provider.getStrictImportableProtoSourcesForDependents()); - } - return builder.build(); - } else { - return NestedSetBuilder.wrap( - STABLE_ORDER, Iterables.transform(sources, ProtoSource::getSourceFile)); - } - } - - private static NestedSet computeExportedProtos( - ImmutableList directSources, ImmutableList deps) { - if (!directSources.isEmpty()) { - return NestedSetBuilder.wrap(STABLE_ORDER, directSources); - } - - /* a proxy/alias library, return the sources of the direct deps */ - NestedSetBuilder builder = NestedSetBuilder.stableOrder(); - for (ProtoInfo provider : deps) { - builder.addTransitive(provider.getExportedSources()); - } - return builder.build(); - } - - private static NestedSet computeTransitiveProtoSources( - ImmutableList protoDeps, Library library) { - NestedSetBuilder result = NestedSetBuilder.naiveLinkOrder(); - result.addAll(library.getSources()); - for (ProtoInfo dep : protoDeps) { - result.addTransitive(dep.getTransitiveSources()); - } - return result.build(); - } - - /** - * Collects all .proto files in this lib and its transitive dependencies. - * - *

Each import is a Artifact/Label pair. - */ - private static NestedSet computeTransitiveProtoSourceArtifacts( - ImmutableList sources, ImmutableList deps) { - NestedSetBuilder result = NestedSetBuilder.naiveLinkOrder(); - result.addAll(Iterables.transform(sources, ProtoSource::getSourceFile)); - for (ProtoInfo dep : deps) { - result.addTransitive(dep.getTransitiveProtoSources()); - } - return result.build(); - } - - static NestedSet computeDependenciesDescriptorSets(ImmutableList deps) { - return computeTransitiveDescriptorSets(null, deps); - } - - private static NestedSet computeTransitiveDescriptorSets( - @Nullable Artifact directDescriptorSet, ImmutableList deps) { - NestedSetBuilder result = NestedSetBuilder.stableOrder(); - if (directDescriptorSet != null) { - result.add(directDescriptorSet); - } - for (ProtoInfo dep : deps) { - result.addTransitive(dep.getTransitiveDescriptorSets()); - } - return result.build(); - } - - /** - * Returns all proto source roots in this lib ({@code currentProtoSourceRoot}) and in its - * transitive dependencies. - * - *

Assumes {@code currentProtoSourceRoot} is the same as the package name. - */ - private static NestedSet computeTransitiveProtoSourceRoots( - ImmutableList protoDeps, String currentProtoSourceRoot) { - NestedSetBuilder protoPath = NestedSetBuilder.stableOrder(); - - protoPath.add(currentProtoSourceRoot); - for (ProtoInfo provider : protoDeps) { - protoPath.addTransitive(provider.getTransitiveProtoSourceRoots()); - } - - return protoPath.build(); - } - - /** Basically a {@link Pair}. */ - private static final class Library { - private final ImmutableList sources; - private final PathFragment sourceRoot; - - Library(ImmutableList sources, PathFragment sourceRoot) { - this.sources = sources; - this.sourceRoot = sourceRoot; - } - - public ImmutableList getSources() { - return sources; - } - - public PathFragment getSourceRoot() { - return sourceRoot; - } - } - - /** - * Returns the {@link Library} representing this proto_library rule. - * - *

Assumes that strip_import_prefix and import_prefix are unset and - * that there are no generated .proto files that need to be compiled. - */ - @Nullable - public static Library createLibraryWithoutVirtualSourceRoot( - PathFragment protoSourceRoot, ImmutableList directSources) { - ImmutableList.Builder sources = ImmutableList.builder(); - for (Artifact protoSource : directSources) { - sources.add( - new ProtoSource( - /* sourceFile */ protoSource, - /* sourceRoot */ memoryEfficientProtoSourceRoot( - protoSourceRoot.getRelative(protoSource.getRoot().getExecPath())))); - } - return new Library(sources.build(), memoryEfficientProtoSourceRoot(protoSourceRoot)); - } - - private static PathFragment getPathFragmentAttribute( - RuleContext ruleContext, String attributeName) { - if (!ruleContext.attributes().has(attributeName)) { - return null; - } - - if (!ruleContext.attributes().isAttributeValueExplicitlySpecified(attributeName)) { - return null; - } - - String asString = ruleContext.attributes().get(attributeName, STRING); - if (!PathFragment.isNormalized(asString)) { - ruleContext.attributeError( - attributeName, "should be normalized (without uplevel references or '.' path segments)"); - return null; - } - - return PathFragment.create(asString); - } - - /** - * Returns the {@link Library} representing this proto_library rule if import prefix - * munging is done. Otherwise, returns null. - */ - private static Library createLibraryWithVirtualSourceRootMaybe( - RuleContext ruleContext, - ImmutableList protoSources, - boolean generatedProtosInVirtualImports) { - PathFragment importPrefixAttribute = getPathFragmentAttribute(ruleContext, "import_prefix"); - PathFragment stripImportPrefixAttribute = - getPathFragmentAttribute(ruleContext, "strip_import_prefix"); - boolean hasGeneratedSources = false; - - if (generatedProtosInVirtualImports) { - for (Artifact protoSource : protoSources) { - if (!protoSource.isSourceArtifact()) { - hasGeneratedSources = true; - break; - } - } - } - - if (importPrefixAttribute == null - && stripImportPrefixAttribute == null - && !hasGeneratedSources) { - // Simple case, no magic required. - return null; - } - - PathFragment stripImportPrefix; - if (stripImportPrefixAttribute == null) { - stripImportPrefix = PathFragment.EMPTY_FRAGMENT; - } else if (stripImportPrefixAttribute.isAbsolute()) { - stripImportPrefix = stripImportPrefixAttribute.toRelative(); - } else { - stripImportPrefix = - ruleContext.getLabel().getPackageFragment().getRelative(stripImportPrefixAttribute); - } - - PathFragment importPrefix = - importPrefixAttribute != null ? importPrefixAttribute : PathFragment.EMPTY_FRAGMENT; - if (importPrefix.isAbsolute()) { - ruleContext.attributeError("import_prefix", "should be a relative path"); - return null; - } - - PathFragment sourceRootPath = ruleContext.getUniqueDirectory("_virtual_imports"); - PathFragment sourceRoot = - memoryEfficientProtoSourceRoot( - ruleContext.getBinOrGenfilesDirectory().getExecPath().getRelative(sourceRootPath)); - - ImmutableList.Builder sources = ImmutableList.builder(); - for (Artifact realProtoSource : protoSources) { - if (!realProtoSource.getRepositoryRelativePath().startsWith(stripImportPrefix)) { - ruleContext.ruleError( - String.format( - ".proto file '%s' is not under the specified strip prefix '%s'", - realProtoSource.getExecPathString(), stripImportPrefix.getPathString())); - continue; - } - Artifact virtualProtoSource = - createVirtualProtoSource( - ruleContext, realProtoSource, sourceRootPath, importPrefix, stripImportPrefix); - sources.add( - new ProtoSource( - /* sourceFile */ virtualProtoSource, - /* originalSourceFile */ realProtoSource, - /* sourceRoot */ sourceRoot)); - } - return new Library(sources.build(), sourceRoot); - } - - private static Artifact createVirtualProtoSource( - RuleContext ruleContext, - Artifact realProtoSource, - PathFragment sourceRootPath, - PathFragment importPrefix, - PathFragment stripImportPrefix) { - PathFragment importPath = - importPrefix.getRelative( - realProtoSource.getRepositoryRelativePath().relativeTo(stripImportPrefix)); - - Artifact virtualProtoSource = - ruleContext.getDerivedArtifact( - sourceRootPath.getRelative(importPath), ruleContext.getBinOrGenfilesDirectory()); - - ruleContext.registerAction( - SymlinkAction.toArtifact( - ruleContext.getActionOwner(), - realProtoSource, - virtualProtoSource, - "Symlinking virtual .proto sources for " + ruleContext.getLabel())); - - return virtualProtoSource; - } - - /** - * Check that .proto files in sources are from the same package. This is done to avoid clashes - * with the generated sources. - */ - public static void checkSourceFilesAreInSamePackage(RuleContext ruleContext) { - // TODO(bazel-team): this does not work with filegroups that contain files - // that are not in the package - for (Label source : ruleContext.attributes().get("srcs", BuildType.LABEL_LIST)) { - if (!isConfiguredTargetInSamePackage(ruleContext, source)) { - ruleContext.attributeError( - "srcs", - "Proto source with label '" + source + "' must be in same package as consuming rule."); - } - } - } - - private static boolean isConfiguredTargetInSamePackage(RuleContext ruleContext, Label source) { - return ruleContext.getLabel().getPackageIdentifier().equals(source.getPackageIdentifier()); - } - - /** - * Creates the {@link ProtoInfo} for the {@code proto_library} rule associated with {@code - * ruleContext}. - */ - public static ProtoInfo createProtoInfo( - RuleContext ruleContext, boolean generatedProtosInVirtualImports) { - ImmutableList originalDirectProtoSources = - ruleContext.getPrerequisiteArtifacts("srcs").list(); - ImmutableList deps = - ImmutableList.copyOf(ruleContext.getPrerequisites("deps", ProtoInfo.PROVIDER)); - ImmutableList exports = - ImmutableList.copyOf(ruleContext.getPrerequisites("exports", ProtoInfo.PROVIDER)); - - Library library = - createLibraryWithVirtualSourceRootMaybe( - ruleContext, originalDirectProtoSources, generatedProtosInVirtualImports); - if (ruleContext.hasErrors()) { - return null; - } - - if (library == null) { - PathFragment contextProtoSourceRoot = - ruleContext - .getLabel() - .getRepository() - .getExecPath(ruleContext.getConfiguration().isSiblingRepositoryLayout()); - library = - createLibraryWithoutVirtualSourceRoot(contextProtoSourceRoot, originalDirectProtoSources); - } - - ImmutableList directSources = library.getSources(); - PathFragment directProtoSourceRoot = library.getSourceRoot(); - NestedSet transitiveSources = computeTransitiveProtoSources(deps, library); - NestedSet transitiveProtoSources = - computeTransitiveProtoSourceArtifacts(directSources, deps); - NestedSet transitiveProtoSourceRoots = - computeTransitiveProtoSourceRoots(deps, directProtoSourceRoot.getSafePathString()); - NestedSet strictImportableProtosForDependents = - computeStrictImportableProtosForDependents(directSources, deps); - Artifact directDescriptorSet = - ruleContext.getGenfilesArtifact( - ruleContext.getLabel().getName() + "-descriptor-set.proto.bin"); - NestedSet transitiveDescriptorSets = - computeTransitiveDescriptorSets(directDescriptorSet, deps); - - // Layering checks. - NestedSet exportedSources = computeExportedProtos(directSources, deps); - NestedSet strictImportableSources = - computeStrictImportableProtos(directSources, deps); - NestedSet publicImportSources = computePublicImportProtos(exports); - - return new ProtoInfo( - directSources, - directProtoSourceRoot, - transitiveSources, - transitiveProtoSources, - transitiveProtoSourceRoots, - strictImportableProtosForDependents, - directDescriptorSet, - transitiveDescriptorSets, - exportedSources, - strictImportableSources, - publicImportSources); - } - - public static Runfiles.Builder createDataRunfilesProvider( - final NestedSet transitiveProtoSources, RuleContext ruleContext) { - // We assume that the proto sources will not have conflicting artifacts - // with the same root relative path - return new Runfiles.Builder( - ruleContext.getWorkspaceName(), ruleContext.getConfiguration().legacyExternalRunfiles()) - .addTransitiveArtifactsWrappedInStableOrder(transitiveProtoSources); - } - // ================================================================= // Protocol compiler invocation stuff. @@ -475,43 +117,6 @@ public static ImmutableList getGeneratedOutputs( return getGeneratedOutputs(ruleContext, protoSources, extension, false); } - public static ImmutableList getGeneratedTreeArtifactOutputs( - RuleContext ruleContext, ImmutableList protoSources, PathFragment directory) { - ImmutableList.Builder outputsBuilder = new ImmutableList.Builder<>(); - if (!protoSources.isEmpty()) { - ArtifactRoot genfiles = - ruleContext - .getConfiguration() - .getGenfilesDirectory(ruleContext.getRule().getRepository()); - outputsBuilder.add(ruleContext.getTreeArtifact(directory, genfiles)); - } - return outputsBuilder.build(); - } - - private static NestedSet computeStrictImportableProtos( - ImmutableList directSources, ImmutableList deps) { - NestedSetBuilder builder = NestedSetBuilder.stableOrder(); - if (!directSources.isEmpty()) { - builder.addAll(directSources); - for (ProtoInfo provider : deps) { - builder.addTransitive(provider.getExportedSources()); - } - } - return builder.build(); - } - - /** - * Returns the .proto files that are the direct srcs of the exported dependencies of this rule. - */ - private static NestedSet computePublicImportProtos( - ImmutableList exports) { - NestedSetBuilder result = NestedSetBuilder.stableOrder(); - for (ProtoInfo export : exports) { - result.addTransitive(export.getExportedSources()); - } - return result.build(); - } - /** * Decides whether this proto_library should check for strict proto deps. * diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java index 4202e3aac5695c..41e22eba5ba3c1 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java @@ -15,42 +15,24 @@ package com.google.devtools.build.lib.rules.proto; import static com.google.common.collect.Iterables.isEmpty; -import static com.google.devtools.build.lib.rules.proto.ProtoCommon.areDepsStrict; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; -import com.google.common.base.Supplier; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.devtools.build.lib.actions.AbstractAction; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.CommandLineItem; -import com.google.devtools.build.lib.actions.CommandLineItem.CapturingMapFn; -import com.google.devtools.build.lib.actions.ParamFileInfo; -import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.actions.ResourceSetOrBuilder; import com.google.devtools.build.lib.analysis.FilesToRunProvider; import com.google.devtools.build.lib.analysis.RuleContext; -import com.google.devtools.build.lib.analysis.actions.CustomCommandLine; -import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg; -import com.google.devtools.build.lib.analysis.actions.FileWriteAction; -import com.google.devtools.build.lib.analysis.actions.SpawnAction; -import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.Depset; import com.google.devtools.build.lib.collect.nestedset.Depset.ElementType; -import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException; -import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; -import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; import com.google.devtools.build.lib.util.OS; -import com.google.devtools.build.lib.util.OnDemandString; import java.util.HashSet; import java.util.List; -import java.util.function.Consumer; -import javax.annotation.Nullable; import net.starlark.java.eval.Dict; import net.starlark.java.eval.StarlarkCallable; import net.starlark.java.eval.StarlarkFloat; @@ -141,25 +123,6 @@ public ProtoCompileActionBuilder( this.mnemonic = DEFAULT_MNEMONIC; } - /** Static class to avoid keeping a reference to this builder after build() is called. */ - @AutoCodec.VisibleForSerialization - @AutoCodec - static class OnDemandLangPluginFlag extends OnDemandString { - private final String langPrefix; - private final Supplier langPluginParameter; - - @AutoCodec.VisibleForSerialization - OnDemandLangPluginFlag(String langPrefix, Supplier langPluginParameter) { - this.langPrefix = langPrefix; - this.langPluginParameter = langPluginParameter; - } - - @Override - public String toString() { - return String.format("--%s_out=%s", langPrefix, langPluginParameter.get()); - } - } - /** Builds a ResourceSet based on the number of inputs. */ public static class ProtoCompileResourceSetBuilder implements ResourceSetOrBuilder { @Override @@ -219,8 +182,7 @@ public void maybeRegister(RuleContext ruleContext) /* mnemonic */ mnemonic, /* strict_imports */ checkStrictImportPublic, /* additional_inputs */ inputs == null - ? Depset.of( - ElementType.EMPTY, NestedSetBuilder.emptySet(Order.STABLE_ORDER)) + ? Depset.of(ElementType.EMPTY, NestedSetBuilder.emptySet(Order.STABLE_ORDER)) : Depset.of(Artifact.TYPE, NestedSetBuilder.wrap(Order.STABLE_ORDER, inputs)), /* resource_set */ new StarlarkCallable() { @@ -244,67 +206,6 @@ public Object call(StarlarkThread thread, Tuple args, Dict kwarg ImmutableMap.of()); } - public static void writeDescriptorSet( - RuleContext ruleContext, ProtoInfo protoInfo, Services allowServices) { - Artifact output = protoInfo.getDirectDescriptorSet(); - ImmutableList protoDeps = - ImmutableList.copyOf(ruleContext.getPrerequisites("deps", ProtoInfo.PROVIDER)); - NestedSet dependenciesDescriptorSets = - ProtoCommon.computeDependenciesDescriptorSets(protoDeps); - - ProtoToolchainInfo protoToolchain = ProtoToolchainInfo.fromRuleContext(ruleContext); - if (protoToolchain == null || protoInfo.getDirectProtoSources().isEmpty()) { - ruleContext.registerAction( - FileWriteAction.createEmptyWithInputs( - ruleContext.getActionOwner(), dependenciesDescriptorSets, output)); - return; - } - - SpawnAction.Builder actions = - createActions( - ruleContext, - protoToolchain, - ImmutableList.of( - createDescriptorSetToolchain( - ruleContext.getFragment(ProtoConfiguration.class), output.getExecPathString())), - protoInfo, - ruleContext.getLabel(), - ImmutableList.of(output), - "Descriptor Set", - Exports.DO_NOT_USE, - allowServices); - if (actions == null) { - return; - } - - actions.setMnemonic("GenProtoDescriptorSet"); - actions.addTransitiveInputs(dependenciesDescriptorSets); - ruleContext.registerAction(actions.build(ruleContext)); - } - - private static ToolchainInvocation createDescriptorSetToolchain( - ProtoConfiguration configuration, CharSequence outReplacement) { - ImmutableList.Builder protocOpts = ImmutableList.builder(); - if (configuration.experimentalProtoDescriptorSetsIncludeSourceInfo()) { - protocOpts.add("--include_source_info"); - } - - return new ToolchainInvocation( - "dontcare", - ProtoLangToolchainProvider.create( - // Note: adding --include_imports here was requested multiple times, but it'll cause the - // output size to become quadratic, so don't. - // A rule that concatenates the artifacts from ctx.deps.proto.transitive_descriptor_sets - // provides similar results. - "--descriptor_set_out=%s", - /* pluginFormatFlag = */ null, - /* pluginExecutable= */ null, - /* runtime= */ null, - /* providedProtoSources= */ ImmutableList.of()), - outReplacement, - protocOpts.build()); - } - /** Whether to use exports in the proto compile action. */ public enum Exports { USE, @@ -317,11 +218,6 @@ public enum Services { DISALLOW, } - /** Whether to enable strict dependency checking. */ - public enum Deps { - STRICT, - NON_STRICT, - } /** * Registers actions to generate code from .proto files. * @@ -400,203 +296,11 @@ public static void registerActions( arePublicImportsStrict(ruleContext) ? (useExports == Exports.USE) : false)); } - @Nullable - private static SpawnAction.Builder createActions( - RuleContext ruleContext, - ProtoToolchainInfo protoToolchain, - List toolchainInvocations, - ProtoInfo protoInfo, - Label ruleLabel, - Iterable outputs, - String flavorName, - Exports useExports, - Services allowServices) { - if (isEmpty(outputs)) { - return null; - } - - SpawnAction.Builder result = - new SpawnAction.Builder().addTransitiveInputs(protoInfo.getTransitiveProtoSources()); - - for (ToolchainInvocation invocation : toolchainInvocations) { - ProtoLangToolchainProvider toolchain = invocation.toolchain; - if (toolchain.pluginExecutable() != null) { - result.addTool(toolchain.pluginExecutable()); - } - } - - result - .addOutputs(outputs) - .setResources(AbstractAction.DEFAULT_RESOURCE_SET) - .useDefaultShellEnvironment() - .setExecutable(protoToolchain.getCompiler()) - .addCommandLine( - createCommandLineFromToolchains( - toolchainInvocations, - protoInfo, - ruleLabel, - areDepsStrict(ruleContext) ? Deps.STRICT : Deps.NON_STRICT, - arePublicImportsStrict(ruleContext) ? useExports : Exports.DO_NOT_USE, - allowServices, - protoToolchain.getCompilerOptions()), - ParamFileInfo.builder(ParameterFileType.UNQUOTED).build()) - .setProgressMessage("Generating %s proto_library %s", flavorName, ruleContext.getLabel()); - - return result; - } public static boolean arePublicImportsStrict(RuleContext ruleContext) { return ruleContext.getFragment(ProtoConfiguration.class).strictPublicImports(); } - /** - * Constructs command-line arguments to execute proto-compiler. - * - *
    - *
  • Each toolchain contributes a command-line, formatted from its commandLine() method. - *
  • $(OUT) is replaced with the outReplacement field of ToolchainInvocation. - *
  • If a toolchain's {@code plugin()} is non-null, we point at it by emitting - * --plugin=protoc-gen-PLUGIN_=. - *
- * - * Note {@code toolchainInvocations} is ordered, and affects the order in which plugins are - * called. As some plugins rely on output from other plugins, their order matters. - * - * @param toolchainInvocations See {@link #createCommandLineFromToolchains}. - * @param ruleLabel Name of the proto_library for which we're compiling. This string is used to - * populate an error message format that's passed to proto-compiler. - * @param allowServices If false, the compilation will break if any .proto file has - */ - private static CustomCommandLine createCommandLineFromToolchains( - List toolchainInvocations, - ProtoInfo protoInfo, - Label ruleLabel, - Deps strictDeps, - Exports useExports, - Services allowServices, - ImmutableList protocOpts) { - CustomCommandLine.Builder cmdLine = CustomCommandLine.builder(); - - cmdLine.addAll( - VectorArg.of(protoInfo.getTransitiveProtoSourceRoots()) - .mapped(EXPAND_TRANSITIVE_PROTO_PATH_FLAGS)); - - // A set to check if there are multiple invocations with the same name. - HashSet invocationNames = new HashSet<>(); - - for (ToolchainInvocation invocation : toolchainInvocations) { - if (!invocationNames.add(invocation.name)) { - throw new IllegalStateException( - "Invocation name " - + invocation.name - + " appears more than once. " - + "This could lead to incorrect proto-compiler behavior"); - } - - ProtoLangToolchainProvider toolchain = invocation.toolchain; - - final String formatString = toolchain.outReplacementFormatFlag(); - final CharSequence outReplacement = invocation.outReplacement; - cmdLine.addLazyString( - new OnDemandString() { - @Override - public String toString() { - return String.format(formatString, outReplacement); - } - }); - - if (toolchain.pluginExecutable() != null) { - cmdLine.addFormatted( - "--plugin=protoc-gen-PLUGIN_%s=%s", - invocation.name, toolchain.pluginExecutable().getExecutable().getExecPath()); - } - - cmdLine.addAll(invocation.protocOpts); - } - - cmdLine.addAll(protocOpts); - - // Add include maps - addIncludeMapArguments( - cmdLine, - strictDeps == Deps.STRICT ? protoInfo.getStrictImportableSources() : null, - protoInfo.getTransitiveSources()); - - if (strictDeps == Deps.STRICT) { - cmdLine.addFormatted(STRICT_DEPS_FLAG_TEMPLATE, ruleLabel); - } - - if (useExports == Exports.USE) { - if (protoInfo.getPublicImportSources().isEmpty()) { - // This line is necessary to trigger the check. - cmdLine.add("--allowed_public_imports="); - } else { - cmdLine.addAll( - "--allowed_public_imports", - VectorArg.join(":") - .each(protoInfo.getPublicImportSources()) - .mapped(EXPAND_TO_IMPORT_PATHS)); - } - } - - for (Artifact src : protoInfo.getDirectProtoSources()) { - cmdLine.addPath(src.getExecPath()); - } - - if (allowServices == Services.DISALLOW) { - cmdLine.add("--disallow_services"); - } - - return cmdLine.build(); - } - - @VisibleForTesting - static void addIncludeMapArguments( - CustomCommandLine.Builder commandLine, - @Nullable NestedSet strictImportableProtoSources, - NestedSet transitiveSources) { - // For each import, include both the import as well as the import relativized against its - // protoSourceRoot. This ensures that protos can reference either the full path or the short - // path when including other protos. - commandLine.addAll(VectorArg.of(transitiveSources).mapped(new ExpandImportArgsFn())); - if (strictImportableProtoSources != null) { - if (!strictImportableProtoSources.isEmpty()) { - commandLine.addAll( - "--direct_dependencies", - VectorArg.join(":").each(strictImportableProtoSources).mapped(EXPAND_TO_IMPORT_PATHS)); - - } else { - // The proto compiler requires an empty list to turn on strict deps checking - commandLine.add("--direct_dependencies="); - } - } - } - - @SerializationConstant @AutoCodec.VisibleForSerialization - static final CommandLineItem.MapFn EXPAND_TO_IMPORT_PATHS = - (src, args) -> args.accept(src.getImportPath().getSafePathString()); - - @SerializationConstant @AutoCodec.VisibleForSerialization - static final CommandLineItem.MapFn EXPAND_TRANSITIVE_PROTO_PATH_FLAGS = - (flag, args) -> { - if (!flag.equals(".")) { - args.accept("--proto_path=" + flag); - } - }; - - private static final class ExpandImportArgsFn implements CapturingMapFn { - /** - * Generates up to two import flags for each artifact: one for full path (only relative to the - * repository root) and one for the path relative to the proto source root (if one exists - * corresponding to the artifact). - */ - @Override - public void expandToCommandLine(ProtoSource proto, Consumer args) { - String importPath = proto.getImportPath().getSafePathString(); - args.accept("-I" + importPath + "=" + proto.getSourceFile().getExecPathString()); - } - } - /** * Describes a toolchain and the value to replace for a $(OUT) that might appear in its * commandLine() (e.g., "bazel-out/foo.srcjar"). diff --git a/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCacheTest.java b/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCacheTest.java index 87f5275c8c4f97..216b3c326be023 100644 --- a/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCacheTest.java @@ -21,7 +21,6 @@ import com.google.common.collect.Multiset; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.CommandLineItem; -import com.google.devtools.build.lib.actions.CommandLineItem.CapturingMapFn; import com.google.devtools.build.lib.actions.CommandLineItem.MapFn; import com.google.devtools.build.lib.util.Fingerprint; import java.util.function.Consumer; @@ -142,12 +141,6 @@ public void testMultipleInstancesOfMapFnThrows() throws Exception { (s, args) -> args.accept(s + "_mapped"), new Fingerprint(), nestedSet); } - // Make sure a CapturingMapFn doesn't get denied - for (int i = 0; i < 2; ++i) { - cache.addNestedSetToFingerprint( - (CapturingMapFn) (s, args) -> args.accept(s + 1), new Fingerprint(), nestedSet); - } - // Make sure a ParametrizedMapFn doesn't get denied until it exceeds its instance count cache.addNestedSetToFingerprint(new IntParametrizedMapFn(1), new Fingerprint(), nestedSet); cache.addNestedSetToFingerprint(new IntParametrizedMapFn(2), new Fingerprint(), nestedSet);