Refactors CompilationSupport for objc to use existing API

oquenchil · katre · commit ba74df07ced9 · 2021-08-13T11:35:34.000-04:00
This CL makes CompilationSupport use CcLinkingHelper instead of CppLinkActionBuilder. The former is the Java class used by the existing Starlark linking API. This is in preparation for a future (unknown when) re-write of objc rules to Starlark. This is a rollforward after fixing an issue with implicit outputs of proto_libraries which had the j2objc_aspect applied on it. These artifacts were created with the genfiles dir while the C++ sandwich was always creating them in the bin dir. RELNOTES:none PiperOrigin-RevId: 353879792 (cherry picked from commit 31b689b)
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingContext.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingContext.java
@@ -526,7 +526,7 @@ public Builder addUserLinkFlags(List<LinkOptions> userLinkFlags) {
       return this;
     }
 
-    Builder addLinkstamps(List<Linkstamp> linkstamps) {
+    public Builder addLinkstamps(List<Linkstamp> linkstamps) {
       hasDirectLinkerInput = true;
       linkerInputBuilder.addLinkstamps(linkstamps);
       return this;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingHelper.java
@@ -21,6 +21,7 @@
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.ActionRegistry;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.RuleContext;
@@ -87,7 +88,9 @@ public CcLinkingOutputs getCcLinkingOutputs() {
   private final BuildConfiguration configuration;
   private final CppConfiguration cppConfiguration;
 
-  private final List<Artifact> nonCodeLinkerInputs = new ArrayList<>();
+  private final NestedSetBuilder<Artifact> additionalLinkerInputsBuilder =
+      NestedSetBuilder.stableOrder();
+  private final List<Artifact> linkerOutputs = new ArrayList<>();
   private final List<String> linkopts = new ArrayList<>();
   private final List<CcLinkingContext> ccLinkingContexts = new ArrayList<>();
   private final NestedSetBuilder<Artifact> linkstamps = NestedSetBuilder.stableOrder();
@@ -96,6 +99,7 @@ public CcLinkingOutputs getCcLinkingOutputs() {
   @Nullable private Artifact linkerOutputArtifact;
   private LinkTargetType staticLinkType = LinkTargetType.STATIC_LIBRARY;
   private LinkTargetType dynamicLinkType = LinkTargetType.NODEPS_DYNAMIC_LIBRARY;
+  private NestedSet<Artifact> additionalLinkerInputs;
   private boolean neverlink;
 
   private boolean emitInterfaceSharedLibraries;
@@ -194,19 +198,27 @@ public CcLinkingHelper addAdditionalLinkstampDefines(List<String> additionalLink
     return this;
   }
 
-  /** Adds the corresponding non-code files as linker inputs. */
+  /**
+   * Adds the corresponding non-code files as linker inputs.
+   *
+   * <p>TODO(bazel-team): There is no practical difference in non-code inputs and additional linker
+   * inputs in CppLinkActionBuilder. So these should be merged. Even before that happens, it's
+   * totally fine for nonCodeLinkerInputs to contains precompiled libraries.
+   */
   public CcLinkingHelper addNonCodeLinkerInputs(List<Artifact> nonCodeLinkerInputs) {
-    for (Artifact nonCodeLinkerInput : nonCodeLinkerInputs) {
-      String basename = nonCodeLinkerInput.getFilename();
-      Preconditions.checkArgument(!Link.OBJECT_FILETYPES.matches(basename));
-      Preconditions.checkArgument(!Link.ARCHIVE_LIBRARY_FILETYPES.matches(basename));
-      Preconditions.checkArgument(!Link.SHARED_LIBRARY_FILETYPES.matches(basename));
-      this.nonCodeLinkerInputs.add(nonCodeLinkerInput);
-    }
-    if (fdoContext.getPropellerOptimizeInputFile() != null
-        && fdoContext.getPropellerOptimizeInputFile().getLdArtifact() != null) {
-      this.nonCodeLinkerInputs.add(fdoContext.getPropellerOptimizeInputFile().getLdArtifact());
-    }
+    this.additionalLinkerInputsBuilder.addAll(nonCodeLinkerInputs);
+    return this;
+  }
+
+  public CcLinkingHelper addTransitiveAdditionalLinkerInputs(
+      NestedSet<Artifact> additionalLinkerInputs) {
+    this.additionalLinkerInputsBuilder.addTransitive(additionalLinkerInputs);
+    return this;
+  }
+
+  /** TODO(bazel-team): Add to Starlark API */
+  public CcLinkingHelper addLinkerOutputs(List<Artifact> linkerOutputs) {
+    this.linkerOutputs.addAll(linkerOutputs);
     return this;
   }
 
@@ -361,6 +373,9 @@ public CcLinkingOutputs link(CcCompilationOutputs ccOutputs)
       throws RuleErrorException, InterruptedException {
     Preconditions.checkNotNull(ccOutputs);
 
+    Preconditions.checkState(additionalLinkerInputs == null);
+    additionalLinkerInputs = additionalLinkerInputsBuilder.build();
+
     // Create link actions (only if there are object files or if explicitly requested).
     //
     // On some systems, the linker gives an error message if there are no input files. Even with
@@ -401,7 +416,8 @@ public CcLinkingContext buildCcLinkingContextFromLibrariesToLink(
                           CcLinkingContext.LinkOptions.of(
                               ImmutableList.copyOf(linkopts), symbolGenerator)))
               .addLibraries(librariesToLink)
-              .addNonCodeInputs(nonCodeLinkerInputs)
+              // additionalLinkerInputsBuilder not expected to be a big list for now.
+              .addNonCodeInputs(additionalLinkerInputsBuilder.build().toList())
               .addLinkstamps(linkstampBuilder.build())
               .build();
     }
@@ -629,7 +645,6 @@ private CppLinkAction registerActionForStaticLibrary(
     CppLinkAction action =
         newLinkActionBuilder(linkedArtifact, linkTargetTypeUsedForNaming)
             .addObjectFiles(ccOutputs.getObjectFiles(usePic))
-            .addNonCodeInputs(nonCodeLinkerInputs)
             .addLtoCompilationContext(ccOutputs.getLtoCompilationContext())
             .setUsePicForLtoBackendActions(usePic)
             .setLinkingMode(LinkingMode.STATIC)
@@ -694,7 +709,6 @@ private boolean createDynamicLinkAction(
             .addActionInputs(linkActionInputs)
             .addLinkopts(linkopts)
             .addLinkopts(sonameLinkopts)
-            .addNonCodeInputs(nonCodeLinkerInputs)
             .addVariablesExtensions(variablesExtensions);
 
     dynamicLinkActionBuilder.addObjectFiles(ccOutputs.getObjectFiles(usePic));
@@ -829,28 +843,43 @@ private boolean createDynamicLinkAction(
 
   private CppLinkActionBuilder newLinkActionBuilder(
       Artifact outputArtifact, LinkTargetType linkType) {
-    return new CppLinkActionBuilder(
-            ruleErrorConsumer,
-            actionConstructionContext,
-            label,
-            outputArtifact,
-            configuration,
-            ccToolchain,
-            fdoContext,
-            featureConfiguration,
-            semantics)
-        .setGrepIncludes(grepIncludes)
-        .setIsStampingEnabled(isStampingEnabled)
-        .setTestOrTestOnlyTarget(isTestOrTestOnlyTarget)
-        .setLinkType(linkType)
-        .setLinkerFiles(
-            (cppConfiguration.useSpecificToolFiles()
-                    && linkType.linkerOrArchiver() == LinkerOrArchiver.ARCHIVER)
-                ? ccToolchain.getArFiles()
-                : ccToolchain.getLinkerFiles())
-        .setLinkArtifactFactory(linkArtifactFactory)
-        .setUseTestOnlyFlags(useTestOnlyFlags)
-        .addExecutionInfo(executionInfo);
+    if (!additionalLinkerInputsBuilder.isEmpty()) {
+      if (fdoContext.getPropellerOptimizeInputFile() != null
+          && fdoContext.getPropellerOptimizeInputFile().getLdArtifact() != null) {
+        this.additionalLinkerInputsBuilder.add(
+            fdoContext.getPropellerOptimizeInputFile().getLdArtifact());
+      }
+    }
+    CppLinkActionBuilder builder =
+        new CppLinkActionBuilder(
+                ruleErrorConsumer,
+                actionConstructionContext,
+                label,
+                outputArtifact,
+                configuration,
+                ccToolchain,
+                fdoContext,
+                featureConfiguration,
+                semantics)
+            .setGrepIncludes(grepIncludes)
+            .setMnemonic(
+                featureConfiguration.isEnabled(CppRuleClasses.LANG_OBJC) ? "ObjcLink" : null)
+            .setIsStampingEnabled(isStampingEnabled)
+            .setTestOrTestOnlyTarget(isTestOrTestOnlyTarget)
+            .setLinkType(linkType)
+            .setLinkerFiles(
+                (cppConfiguration.useSpecificToolFiles()
+                        && linkType.linkerOrArchiver() == LinkerOrArchiver.ARCHIVER)
+                    ? ccToolchain.getArFiles()
+                    : ccToolchain.getLinkerFiles())
+            .setLinkArtifactFactory(linkArtifactFactory)
+            .setUseTestOnlyFlags(useTestOnlyFlags)
+            .addTransitiveActionInputs(additionalLinkerInputs)
+            .addExecutionInfo(executionInfo);
+    for (Artifact output : linkerOutputs) {
+      builder.addActionOutput(output);
+    }
+    return builder;
   }
 
   /**
@@ -876,12 +905,28 @@ private Artifact getLinkedArtifact(LinkTargetType linkTargetType) throws RuleErr
     linkedName =
         CppHelper.getArtifactNameForCategory(
             ruleErrorConsumer, ccToolchain, linkTargetType.getLinkerOutput(), linkedName);
+
+    ArtifactRoot artifactRoot = configuration.getBinDirectory(label.getRepository());
+    if (linkTargetType.equals(LinkTargetType.OBJC_FULLY_LINKED_ARCHIVE)) {
+      // TODO(blaze-team): This unfortunate editing of the name is here bedcause Objective-C rules
+      // were creating this type of archive without the lib prefix, unlike what the objective-c
+      // toolchain says with getArtifactNameForCategory.
+      // This can be fixed either when implicit outputs are removed from objc_library by keeping the
+      // lib prefix, or by editing the toolchain not to add it.
+      Preconditions.checkState(linkedName.startsWith("lib"));
+      linkedName = linkedName.substring(3);
+      artifactRoot =
+          ((RuleContext) actionConstructionContext).getRule().hasBinaryOutput()
+              ? configuration.getBinDir()
+              : configuration.getGenfilesDir();
+    }
       PathFragment artifactFragment =
           PathFragment.create(label.getName()).getParentDirectory().getRelative(linkedName);
 
     return CppHelper.getLinkedArtifact(
         label,
         actionConstructionContext,
+        artifactRoot,
         configuration,
         linkTargetType,
         linkedArtifactNameSuffix,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
@@ -31,6 +31,7 @@
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
+import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.actions.FailAction;
 import com.google.devtools.build.lib.actions.MiddlemanFactory;
 import com.google.devtools.build.lib.actions.ParamFileInfo;
@@ -473,19 +474,24 @@ public static Artifact getLinkedArtifact(
     }
 
     return getLinkedArtifact(
-        ruleContext.getLabel(), ruleContext, config, linkType, linkedArtifactNameSuffix, name);
+        ruleContext.getLabel(),
+        ruleContext,
+        ruleContext.getBinDirectory(),
+        config,
+        linkType,
+        linkedArtifactNameSuffix,
+        name);
   }
 
   public static Artifact getLinkedArtifact(
       Label label,
       ActionConstructionContext actionConstructionContext,
+      ArtifactRoot artifactRoot,
       BuildConfiguration config,
       LinkTargetType linkType,
       String linkedArtifactNameSuffix,
       PathFragment name) {
-    Artifact result =
-        actionConstructionContext.getPackageRelativeArtifact(
-            name, config.getBinDirectory(label.getRepository()));
+    Artifact result = actionConstructionContext.getPackageRelativeArtifact(name, artifactRoot);
 
     // If the linked artifact is not the linux default, then a FailAction is generated for said
     // linux default to satisfy the requirements of any implicit outputs.
@@ -507,7 +513,7 @@ public static Artifact getLinkedArtifact(
     return result;
   }
 
-  public static Artifact getLinuxLinkedArtifact(
+  private static Artifact getLinuxLinkedArtifact(
       Label label,
       ActionConstructionContext actionConstructionContext,
       BuildConfiguration config,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
@@ -1315,6 +1315,7 @@ public CppLinkActionBuilder addObjectFiles(Iterable<Artifact> inputs) {
    * Adds non-code files to the set of inputs. They will not be passed to the linker command line
    * unless that is explicitly modified, too.
    */
+  // TOOD: Remove and just use method for addLinkerInputs
   public CppLinkActionBuilder addNonCodeInputs(Iterable<Artifact> inputs) {
     for (Artifact input : inputs) {
       addNonCodeInput(input);
@@ -1328,11 +1329,6 @@ public CppLinkActionBuilder addNonCodeInputs(Iterable<Artifact> inputs) {
    * line unless that is explicitly modified, too.
    */
   public CppLinkActionBuilder addNonCodeInput(Artifact input) {
-    String basename = input.getFilename();
-    Preconditions.checkArgument(!Link.ARCHIVE_LIBRARY_FILETYPES.matches(basename), basename);
-    Preconditions.checkArgument(!Link.SHARED_LIBRARY_FILETYPES.matches(basename), basename);
-    Preconditions.checkArgument(!Link.OBJECT_FILETYPES.matches(basename), basename);
-
     this.nonCodeInputs.add(input);
     return this;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java

Original file line number	Diff line number	Diff line change
`@@ -526,7 +526,7 @@ public Builder addUserLinkFlags(List<LinkOptions> userLinkFlags) {`
`526`	`526`	`return this;`
`527`	`527`	`}`
`528`	`528`
`529`		`- Builder addLinkstamps(List<Linkstamp> linkstamps) {`
	`529`	`+ public Builder addLinkstamps(List<Linkstamp> linkstamps) {`
`530`	`530`	`hasDirectLinkerInput = true;`
`531`	`531`	`linkerInputBuilder.addLinkstamps(linkstamps);`
`532`	`532`	`return this;`