Skip to content

Commit

Permalink
Invert attributes to get implementation deps behavior in cc_library
Browse files Browse the repository at this point in the history
Instead of having a new attribute called implementation_deps, we instead have
an attribute called interface_deps whose dependencies will behave as the
old regular deps where everything is propagated downstream. Dependencies in
deps will behave as implementation_deps.

This is done to more closely match the semantics of other language rules that
have an exports attribute for public deps.

This behavior will kick in in Bazel when the flag --experimental_cc_interface_deps
is True. Meaning that dependencies in cc_library.deps will be treated as
implementation deps.

RELNOTES:experimental cc_library.implementation_deps inverted to interface_deps
PiperOrigin-RevId: 430034906
  • Loading branch information
oquenchil authored and copybara-github committed Feb 21, 2022
1 parent c8fb023 commit 5640944
Show file tree
Hide file tree
Showing 12 changed files with 171 additions and 73 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -50,17 +50,17 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
<!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
.add(attr("alwayslink", BOOLEAN))
.override(attr("linkstatic", BOOLEAN).value(false))
/*<!-- #BLAZE_RULE(cc_library).ATTRIBUTE(implementation_deps) -->
The list of other libraries that the library target depends on. Unlike with
<code>deps</code>, the headers and include paths of these libraries (and all their
transitive deps) are only used for compilation of this library, and not libraries that
depend on it. Libraries specified with <code>implementation_deps</code> are still linked in
binary targets that depend on this library.
<p>For now usage is limited to cc_libraries and guarded by the flag
<code>--experimental_cc_implementation_deps</code>.</p>
/*<!-- #BLAZE_RULE(cc_library).ATTRIBUTE(interface_deps) -->
When <code>--experimental_cc_interface_deps</code> is True, the targets listed in deps will
behave as implementation deps. Unlike with regular deps, the headers and include paths of
implementation deps (and all their transitive deps) are only used for compilation of this
library, and not libraries that depend on it. Libraries depended on as implementation deps
are still linked in binary targets that depend on this library. The dependencies listed in
interface_deps will continue having the same behavior as the old deps where the headers and
include paths are propagated downstream.
<!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
.add(
attr("implementation_deps", LABEL_LIST)
attr("interface_deps", LABEL_LIST)
.allowedFileTypes(FileTypeSet.NO_FILE)
.mandatoryProviders(CcInfo.PROVIDER.id()))
.advertiseStarlarkProvider(CcInfo.PROVIDER.id())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,4 +166,15 @@ public void validateLayeringCheckFeatures(
public boolean createEmptyArchive() {
return false;
}

@Override
public boolean shouldUseInterfaceDepsBehavior(RuleContext ruleContext) {
boolean experimentalCcInterfaceDeps =
ruleContext.getFragment(CppConfiguration.class).experimentalCcInterfaceDeps();
if (!experimentalCcInterfaceDeps
&& ruleContext.attributes().isAttributeValueExplicitlySpecified("interface_deps")) {
ruleContext.attributeError("interface_deps", "requires --experimental_cc_interface_deps");
}
return experimentalCcInterfaceDeps;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ public CcCompilationContext getCcCompilationContext() {
private String stripIncludePrefix = null;
private String includePrefix = null;

// This context is built out of deps and implementation_deps.
// This context is built out of interface deps and implementation deps.
private CcCompilationContext ccCompilationContext;

private final RuleErrorConsumer ruleErrorConsumer;
Expand Down Expand Up @@ -1241,7 +1241,7 @@ private List<CppModuleMap> collectModuleMaps() {
ImmutableList.Builder<CppModuleMap> builder = ImmutableList.<CppModuleMap>builder();
// TODO(bazel-team): Here we use the implementationDeps to build the dependents of this rule's
// module map. This is technically incorrect for the following reasons:
// - Clang will not issue a layering_check warning if headers from implementation_deps are
// - Clang will not issue a layering_check warning if headers from implementation deps are
// included from headers of this library.
// - If we were to ever build with modules, Clang might store this dependency inside the .pcm
// It should be evaluated whether this is ok. If this turned into a problem at some
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.FailAction;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
Expand Down Expand Up @@ -122,12 +121,7 @@ public static void init(
return;
}

CppConfiguration cppConfiguration = ruleContext.getFragment(CppConfiguration.class);
if (!cppConfiguration.experimentalCcImplementationDeps()
&& ruleContext.attributes().isAttributeValueExplicitlySpecified("implementation_deps")) {
ruleContext.attributeError(
"implementation_deps", "requires --experimental_cc_implementation_deps");
}
boolean shouldUseInterfaceDepsBehavior = semantics.shouldUseInterfaceDepsBehavior(ruleContext);

final CcCommon common = new CcCommon(ruleContext);
common.reportInvalidOptions(ruleContext);
Expand Down Expand Up @@ -157,7 +151,24 @@ public static void init(
addEmptyRequiredProviders(targetBuilder);
return;
}
Iterable<CcInfo> ccInfosFromDeps = AnalysisUtils.getProviders(deps, CcInfo.PROVIDER);

ImmutableList.Builder<CcCompilationContext> interfaceDeps = ImmutableList.builder();
ImmutableList.Builder<CcCompilationContext> implementationDeps = ImmutableList.builder();

if (shouldUseInterfaceDepsBehavior) {
interfaceDeps.addAll(
CppHelper.getCompilationContextsFromDeps(
ImmutableList.copyOf(ruleContext.getPrerequisites("interface_deps"))));
implementationDeps.addAll(
CppHelper.getCompilationContextsFromDeps(
ImmutableList.copyOf(ruleContext.getPrerequisites("deps"))));
} else {
interfaceDeps.addAll(
CppHelper.getCompilationContextsFromDeps(
ImmutableList.copyOf(ruleContext.getPrerequisites("deps"))));
}
interfaceDeps.add(CcCompilationHelper.getStlCcCompilationContext(ruleContext));

CcCompilationHelper compilationHelper =
new CcCompilationHelper(
ruleContext,
Expand All @@ -176,15 +187,8 @@ public static void init(
.addPrivateHeaders(common.getPrivateHeaders())
.addPublicHeaders(common.getHeaders())
.setCodeCoverageEnabled(CcCompilationHelper.isCodeCoverageEnabled(ruleContext))
.addCcCompilationContexts(
Streams.stream(ccInfosFromDeps)
.map(CcInfo::getCcCompilationContext)
.collect(ImmutableList.toImmutableList()))
.addCcCompilationContexts(
ImmutableList.of(CcCompilationHelper.getStlCcCompilationContext(ruleContext)))
.addImplementationDepsCcCompilationContexts(
CppHelper.getCompilationContextsFromDeps(
ImmutableList.copyOf(ruleContext.getPrerequisites("implementation_deps"))))
.addCcCompilationContexts(interfaceDeps.build())
.addImplementationDepsCcCompilationContexts(implementationDeps.build())
.setHeadersCheckingMode(semantics.determineHeadersCheckingMode(ruleContext));

CcLinkingHelper linkingHelper =
Expand All @@ -205,7 +209,7 @@ public static void init(
.fromCommon(ruleContext, common)
.addCcLinkingContexts(
CppHelper.getLinkingContextsFromDeps(
ImmutableList.copyOf(ruleContext.getPrerequisites("implementation_deps"))))
ImmutableList.copyOf(ruleContext.getPrerequisites("interface_deps"))))
.setGrepIncludes(CppHelper.getGrepIncludes(ruleContext))
.setTestOrTestOnlyTarget(ruleContext.isTestOnlyTarget())
.addLinkopts(common.getLinkopts())
Expand Down Expand Up @@ -255,6 +259,8 @@ public static void init(
linkingHelper.setShouldCreateDynamicLibrary(createDynamicLibrary);
linkingHelper.setLinkerOutputArtifact(soImplArtifact);

CppConfiguration cppConfiguration = ruleContext.getFragment(CppConfiguration.class);

// If the reason we're not creating a dynamic library is that the toolchain
// doesn't support it, then register an action which complains when triggered,
// which only happens when some rule explicitly depends on the dynamic library.
Expand Down Expand Up @@ -485,7 +491,8 @@ public static void init(
.setCcLinkingContext(ccLinkingContext)
.setCcDebugInfoContext(
CppHelper.mergeCcDebugInfoContexts(
compilationInfo.getCcCompilationOutputs(), ccInfosFromDeps))
compilationInfo.getCcCompilationOutputs(),
AnalysisUtils.getProviders(deps, CcInfo.PROVIDER)))
.setCcNativeLibraryInfo(ccNativeLibraryInfo)
.build())
.addOutputGroups(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -859,18 +859,19 @@ public boolean objcShouldGenerateDotdFiles() {
return cppOptions.objcGenerateDotdFiles;
}

// TODO(plf): Change in separate CL where Starlark cc_library.implementation_deps is renamed.
@StarlarkMethod(
name = "experimental_cc_implementation_deps",
documented = false,
useStarlarkThread = true)
public boolean experimentalCcImplementationDepsForStarlark(StarlarkThread thread)
public boolean experimentalCcInterfaceDepsForStarlark(StarlarkThread thread)
throws EvalException {
CcModule.checkPrivateStarlarkificationAllowlist(thread);
return experimentalCcImplementationDeps();
return experimentalCcInterfaceDeps();
}

public boolean experimentalCcImplementationDeps() {
return cppOptions.experimentalCcImplementationDeps;
public boolean experimentalCcInterfaceDeps() {
return cppOptions.experimentalCcInterfaceDeps;
}

public boolean getExperimentalCppCompileResourcesEstimation() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1047,15 +1047,15 @@ public Label getPropellerOptimizeLabel() {
public boolean objcGenerateDotdFiles;

@Option(
name = "experimental_cc_implementation_deps",
name = "experimental_cc_interface_deps",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {
OptionEffectTag.LOADING_AND_ANALYSIS,
},
metadataTags = {OptionMetadataTag.EXPERIMENTAL},
help = "If enabled, cc_library targets can use attribute `implementation_deps`.")
public boolean experimentalCcImplementationDeps;
help = "If enabled, cc_library targets can use attribute `interface_deps`.")
public boolean experimentalCcInterfaceDeps;

@Option(
name = "experimental_link_static_libraries_once",
Expand Down Expand Up @@ -1175,7 +1175,7 @@ public FragmentOptions getHost() {
host.experimentalLinkStaticLibrariesOnce = experimentalLinkStaticLibrariesOnce;
host.experimentalEnableTargetExportCheck = experimentalEnableTargetExportCheck;
host.experimentalCcSharedLibraryDebug = experimentalCcSharedLibraryDebug;
host.experimentalCcImplementationDeps = experimentalCcImplementationDeps;
host.experimentalCcInterfaceDeps = experimentalCcInterfaceDeps;

host.coptList = coptListBuilder.addAll(hostCoptList).build();
host.cxxoptList = cxxoptListBuilder.addAll(hostCxxoptList).build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public static Label ccToolchainTypeAttribute(RuleDefinitionEnvironment env) {
FileTypeSet.of(
CPP_SOURCE, C_SOURCE, CPP_HEADER, ASSEMBLER_WITH_C_PREPROCESSOR, ASSEMBLER))
.withSourceAttributes("srcs", "hdrs")
.withDependencyAttributes("implementation_deps", "deps", "data");
.withDependencyAttributes("interface_deps", "deps", "data");

/** Implicit outputs for cc_binary rules. */
public static final SafeImplicitOutputsFunction CC_BINARY_STRIPPED =
Expand Down Expand Up @@ -464,6 +464,9 @@ public static Label ccToolchainTypeAttribute(RuleDefinitionEnvironment env) {
*/
public static final String LEGACY_IS_CC_TEST_FEATURE_NAME = "legacy_is_cc_test";

/** Tag used to opt in into interface_deps behavior. */
public static final String INTERFACE_DEPS_TAG = "__INTERFACE_DEPS__";

/** Ancestor for all rules that do include scanning. */
public static final class CcIncludeScanningRule implements RuleDefinition {
private final boolean addGrepIncludes;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,6 @@ void validateLayeringCheckFeatures(
ImmutableSet<String> unsupportedFeatures);

boolean createEmptyArchive();

boolean shouldUseInterfaceDepsBehavior(RuleContext ruleContext);
}
Original file line number Diff line number Diff line change
Expand Up @@ -126,4 +126,9 @@ public void validateLayeringCheckFeatures(
public boolean createEmptyArchive() {
return false;
}

@Override
public boolean shouldUseInterfaceDepsBehavior(RuleContext ruleContext) {
return false;
}
}
Loading

0 comments on commit 5640944

Please sign in to comment.