Refactors CompilationSupport for objc to use existing API

oquenchil · copybara-github · commit a8f6152e9f62 · 2021-01-19T09:28:10.000-08: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.

PiperOrigin-RevId: 352585170
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
@@ -566,7 +566,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
@@ -87,7 +87,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 +98,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 +197,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 +372,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 +415,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 +644,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 +708,6 @@ private boolean createDynamicLinkAction(
             .addActionInputs(linkActionInputs)
             .addLinkopts(linkopts)
             .addLinkopts(sonameLinkopts)
-            .addNonCodeInputs(nonCodeLinkerInputs)
             .addVariablesExtensions(variablesExtensions);
 
     dynamicLinkActionBuilder.addObjectFiles(ccOutputs.getObjectFiles(usePic));
@@ -829,28 +842,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,6 +904,15 @@ private Artifact getLinkedArtifact(LinkTargetType linkTargetType) throws RuleErr
     linkedName =
         CppHelper.getArtifactNameForCategory(
             ruleErrorConsumer, ccToolchain, linkTargetType.getLinkerOutput(), linkedName);
+    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);
+    }
       PathFragment artifactFragment =
           PathFragment.create(label.getName()).getParentDirectory().getRelative(linkedName);
 
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
`@@ -566,7 +566,7 @@ public Builder addUserLinkFlags(List<LinkOptions> userLinkFlags) {`
`566`	`566`	`return this;`
`567`	`567`	`}`
`568`	`568`
`569`		`- Builder addLinkstamps(List<Linkstamp> linkstamps) {`
	`569`	`+ public Builder addLinkstamps(List<Linkstamp> linkstamps) {`
`570`	`570`	`hasDirectLinkerInput = true;`
`571`	`571`	`linkerInputBuilder.addLinkstamps(linkstamps);`
`572`	`572`	`return this;`