Skip to content

Commit

Permalink
Refactors CompilationSupport for objc to use existing API
Browse files Browse the repository at this point in the history 
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)
  • Loading branch information
@oquenchil @katre
oquenchil authored and katre committed Aug 13, 2021
1 parent f4e1036 commit ba74df0
Show file tree
Hide file tree
Showing 5 changed files with 196 additions and 110 deletions.
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
121 changes: 83 additions & 38 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -694,7 +709,6 @@ private boolean createDynamicLinkAction(
.addActionInputs(linkActionInputs)
.addLinkopts(linkopts)
.addLinkopts(sonameLinkopts)
.addNonCodeInputs(nonCodeLinkerInputs)
.addVariablesExtensions(variablesExtensions);

dynamicLinkActionBuilder.addObjectFiles(ccOutputs.getObjectFiles(usePic));
Expand Down Expand Up @@ -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;
}

/**
Expand All @@ -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,
Expand Down
16 changes: 11 additions & 5 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand All @@ -507,7 +513,7 @@ public static Artifact getLinkedArtifact(
return result;
}

public static Artifact getLinuxLinkedArtifact(
private static Artifact getLinuxLinkedArtifact(
Label label,
ActionConstructionContext actionConstructionContext,
BuildConfiguration config,
Expand Down
6 changes: 1 addition & 5 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
}
Expand Down
Loading

0 comments on commit ba74df0

Please sign in to comment.