Skip to content

Commit

Permalink
Add --incompatible_objc_compile_info_migration flag
Browse files Browse the repository at this point in the history 
The new logic allows native rules to switch to using compile info from
CcInfo instead of ObjcProvider.

RELNOTES: --incompatible_objc_compile_info_migration determines
whether native rules can assume compile info has been migrated to
CcInfo. See #10854.
PiperOrigin-RevId: 298439046
  • Loading branch information
@googlewalt @copybara-github
googlewalt authored and copybara-github committed Mar 2, 2020
1 parent 0e1100f commit be1d118
Show file tree
Hide file tree
Showing 8 changed files with 321 additions and 39 deletions.
15 changes: 15 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommandLineOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,4 +265,19 @@ public class ObjcCommandLineOptions extends FragmentOptions {
+ "configuration."
)
public Label appleSdk;

@Option(
name = "incompatible_objc_compile_info_migration",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.CHANGES_INPUTS},
metadataTags = {
OptionMetadataTag.INCOMPATIBLE_CHANGE,
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES,
},
help =
"If true, native rules can assume compile info has been migrated to CcInfo. See "
+ "https://github.com/bazelbuild/bazel/issues/10854 for details and migration "
+ "instructions")
public boolean incompatibleObjcCompileInfoMigration;
}
118 changes: 99 additions & 19 deletions src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
import com.google.common.collect.UnmodifiableIterator;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
Expand Down Expand Up @@ -99,6 +100,7 @@ public enum Purpose {
}

static class Builder {
private final boolean compileInfoMigration;
private final Purpose purpose;
private final RuleContext context;
private final StarlarkSemantics semantics;
Expand All @@ -115,6 +117,7 @@ static class Builder {
private Optional<Artifact> linkedBinary = Optional.absent();
private Iterable<CcCompilationContext> depCcHeaderProviders = ImmutableList.of();
private Iterable<CcLinkingContext> depCcLinkProviders = ImmutableList.of();
private Iterable<CcCompilationContext> depCcDirectProviders = ImmutableList.of();

/**
* Builder for {@link ObjcCommon} obtaining both attribute data and configuration data from the
Expand All @@ -135,6 +138,10 @@ static class Builder {
this.context = Preconditions.checkNotNull(context);
this.semantics = context.getAnalysisEnvironment().getSkylarkSemantics();
this.buildConfiguration = Preconditions.checkNotNull(buildConfiguration);

ObjcConfiguration objcConfiguration = buildConfiguration.getFragment(ObjcConfiguration.class);

this.compileInfoMigration = objcConfiguration.compileInfoMigration();
}

public Builder setCompilationAttributes(CompilationAttributes baseCompilationAttributes) {
Expand All @@ -155,9 +162,27 @@ Builder setCompilationArtifacts(CompilationArtifacts compilationArtifacts) {
return this;
}

Builder addDeps(List<ConfiguredTargetAndData> deps) {
ImmutableList.Builder<ObjcProvider> propagatedObjcDeps =
ImmutableList.<ObjcProvider>builder();
private static ImmutableList<CcCompilationContext> getCcCompilationContexts(
Iterable<CcInfo> ccInfos) {
return Streams.stream(ccInfos)
.map(CcInfo::getCcCompilationContext)
.collect(ImmutableList.toImmutableList());
}

Builder addDepCcHeaderProviders(Iterable<CcInfo> cppDeps) {
this.depCcHeaderProviders =
Iterables.concat(this.depCcHeaderProviders, getCcCompilationContexts(cppDeps));
return this;
}

Builder addDepCcDirectProviders(Iterable<CcInfo> cppDeps) {
this.depCcDirectProviders =
Iterables.concat(this.depCcDirectProviders, getCcCompilationContexts(cppDeps));
return this;
}

private Builder addDepsPreMigration(List<ConfiguredTargetAndData> deps) {
ImmutableList.Builder<ObjcProvider> propagatedObjcDeps = ImmutableList.builder();
ImmutableList.Builder<CcInfo> cppDeps = ImmutableList.builder();
ImmutableList.Builder<CcLinkingContext> cppDepLinkParams = ImmutableList.builder();

Expand All @@ -174,15 +199,74 @@ Builder addDeps(List<ConfiguredTargetAndData> deps) {
}
}
}
ImmutableList.Builder<CcCompilationContext> ccCompilationContextBuilder =
ImmutableList.builder();
for (CcInfo ccInfo : cppDeps.build()) {
ccCompilationContextBuilder.add(ccInfo.getCcCompilationContext());
addDepObjcProviders(propagatedObjcDeps.build());
ImmutableList<CcInfo> ccInfos = cppDeps.build();
addDepCcHeaderProviders(ccInfos);
addDepCcDirectProviders(ccInfos);
this.depCcLinkProviders = Iterables.concat(this.depCcLinkProviders, cppDepLinkParams.build());

return this;
}

private Builder addDepsPostMigration(List<ConfiguredTargetAndData> deps) {
ImmutableList.Builder<ObjcProvider> propagatedObjcDeps = ImmutableList.builder();
ImmutableList.Builder<CcInfo> cppDeps = ImmutableList.builder();
ImmutableList.Builder<CcInfo> directCppDeps = ImmutableList.builder();
ImmutableList.Builder<CcLinkingContext> cppDepLinkParams = ImmutableList.builder();

for (ConfiguredTargetAndData dep : deps) {
ConfiguredTarget depCT = dep.getConfiguredTarget();
if (depCT.get(ObjcProvider.SKYLARK_CONSTRUCTOR) != null) {
addAnyProviders(propagatedObjcDeps, depCT, ObjcProvider.SKYLARK_CONSTRUCTOR);
} else {
// This is the way we inject cc_library attributes into direct fields.
addAnyProviders(directCppDeps, depCT, CcInfo.PROVIDER);
}
addAnyProviders(cppDeps, depCT, CcInfo.PROVIDER);
if (isCcLibrary(dep)) {
cppDepLinkParams.add(depCT.get(CcInfo.PROVIDER).getCcLinkingContext());
}
}
addDepObjcProviders(propagatedObjcDeps.build());
this.depCcHeaderProviders =
Iterables.concat(this.depCcHeaderProviders, ccCompilationContextBuilder.build());
addDepCcHeaderProviders(cppDeps.build());
addDepCcDirectProviders(directCppDeps.build());
this.depCcLinkProviders = Iterables.concat(this.depCcLinkProviders, cppDepLinkParams.build());

return this;
}

Builder addDeps(List<ConfiguredTargetAndData> deps) {
if (compileInfoMigration) {
return addDepsPostMigration(deps);
} else {
return addDepsPreMigration(deps);
}
}

private Builder addRuntimeDepsPreMigration(
List<? extends TransitiveInfoCollection> runtimeDeps) {
ImmutableList.Builder<ObjcProvider> propagatedDeps = ImmutableList.builder();

for (TransitiveInfoCollection dep : runtimeDeps) {
addAnyProviders(propagatedDeps, dep, ObjcProvider.SKYLARK_CONSTRUCTOR);
}
this.runtimeDepObjcProviders =
Iterables.concat(this.runtimeDepObjcProviders, propagatedDeps.build());
return this;
}

private Builder addRuntimeDepsPostMigration(
List<? extends TransitiveInfoCollection> runtimeDeps) {
ImmutableList.Builder<ObjcProvider> propagatedObjcDeps = ImmutableList.builder();
ImmutableList.Builder<CcInfo> cppDeps = ImmutableList.builder();

for (TransitiveInfoCollection dep : runtimeDeps) {
addAnyProviders(propagatedObjcDeps, dep, ObjcProvider.SKYLARK_CONSTRUCTOR);
addAnyProviders(cppDeps, dep, CcInfo.PROVIDER);
}
this.runtimeDepObjcProviders =
Iterables.concat(this.runtimeDepObjcProviders, propagatedObjcDeps.build());
addDepCcHeaderProviders(cppDeps.build());
return this;
}

Expand All @@ -191,15 +275,11 @@ Builder addDeps(List<ConfiguredTargetAndData> deps) {
* at build time.
*/
Builder addRuntimeDeps(List<? extends TransitiveInfoCollection> runtimeDeps) {
ImmutableList.Builder<ObjcProvider> propagatedDeps =
ImmutableList.<ObjcProvider>builder();

for (TransitiveInfoCollection dep : runtimeDeps) {
addAnyProviders(propagatedDeps, dep, ObjcProvider.SKYLARK_CONSTRUCTOR);
if (compileInfoMigration) {
return addRuntimeDepsPostMigration(runtimeDeps);
} else {
return addRuntimeDepsPreMigration(runtimeDeps);
}
this.runtimeDepObjcProviders = Iterables.concat(
this.runtimeDepObjcProviders, propagatedDeps.build());
return this;
}

private <T extends Info> ImmutableList.Builder<T> addAnyProviders(
Expand Down Expand Up @@ -276,7 +356,7 @@ Builder setLinkedBinary(Artifact linkedBinary) {
ObjcCommon build() {

ObjcCompilationContext.Builder objcCompilationContextBuilder =
ObjcCompilationContext.builder();
ObjcCompilationContext.builder(compileInfoMigration);

ObjcProvider.Builder objcProvider = new ObjcProvider.NativeBuilder(semantics);

Expand All @@ -292,7 +372,7 @@ ObjcCommon build() {
// CcCompilationHelper.getStlCcCompilationContext(), but probably shouldn't.
.addDepCcCompilationContexts(depCcHeaderProviders);

for (CcCompilationContext headerProvider : depCcHeaderProviders) {
for (CcCompilationContext headerProvider : depCcDirectProviders) {
objcProvider.addAllDirect(HEADER, headerProvider.getDeclaredIncludeSrcs().toList());
}

Expand Down
16 changes: 10 additions & 6 deletions src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompilationContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
*/
@Immutable
public final class ObjcCompilationContext {
public static final ObjcCompilationContext EMPTY = builder().build();
public static final ObjcCompilationContext EMPTY = builder(false).build();

private final ImmutableList<String> defines;

Expand Down Expand Up @@ -126,11 +126,12 @@ public CcCompilationContext createCcCompilationContext() {
return builder.build();
}

public static Builder builder() {
return new Builder();
public static Builder builder(boolean compileInfoMigration) {
return new Builder(compileInfoMigration);
}

static class Builder {
private final boolean compileInfoMigration;
private final List<String> defines = new ArrayList<>();
private final List<Artifact> publicHeaders = new ArrayList<>();
private final List<Artifact> publicTextualHeaders = new ArrayList<>();
Expand All @@ -141,7 +142,9 @@ static class Builder {
private final List<PathFragment> strictDependencyIncludes = new ArrayList<>();
private final List<CcCompilationContext> depCcCompilationContexts = new ArrayList<>();

Builder() {}
Builder(boolean compileInfoMigration) {
this.compileInfoMigration = compileInfoMigration;
}

public Builder addDefines(Iterable<String> defines) {
Iterables.addAll(this.defines, defines);
Expand Down Expand Up @@ -180,8 +183,9 @@ public Builder addQuoteIncludes(Iterable<PathFragment> includes) {

public Builder addDepObjcProviders(Iterable<ObjcProvider> objcProviders) {
for (ObjcProvider objcProvider : objcProviders) {
this.depCcCompilationContexts.add(objcProvider.getCcCompilationContext());

if (!compileInfoMigration) {
this.depCcCompilationContexts.add(objcProvider.getCcCompilationContext());
}
this.strictDependencyIncludes.addAll(objcProvider.getStrictDependencyIncludes());
}
return this;
Expand Down
7 changes: 7 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment
private final HeaderDiscovery.DotdPruningMode dotdPruningPlan;
private final boolean shouldScanIncludes;
private final Label appleSdk;
private final boolean compileInfoMigration;

ObjcConfiguration(ObjcCommandLineOptions objcOptions, CoreOptions options) {
this.iosSimulatorDevice = objcOptions.iosSimulatorDevice;
Expand Down Expand Up @@ -95,6 +96,7 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment
: HeaderDiscovery.DotdPruningMode.DO_NOT_USE;
this.shouldScanIncludes = objcOptions.scanIncludes;
this.appleSdk = objcOptions.appleSdk;
this.compileInfoMigration = objcOptions.incompatibleObjcCompileInfoMigration;
}

/**
Expand Down Expand Up @@ -258,4 +260,9 @@ public boolean shouldScanIncludes() {
public Label getAppleSdk() {
return appleSdk;
}

/** Whether native rules can assume compile info has been migrated to CcInfo. */
public boolean compileInfoMigration() {
return compileInfoMigration;
}
}
23 changes: 18 additions & 5 deletions src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoAspect.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.SkylarkNativeAspect;
import com.google.devtools.build.lib.rules.cpp.CcCompilationContext;
import com.google.devtools.build.lib.rules.cpp.CcInfo;
import com.google.devtools.build.lib.rules.proto.ProtoInfo;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand Down Expand Up @@ -98,14 +100,25 @@ public ConfiguredAspect create(

// Propagate protobuf's headers and search paths so the BinaryLinkingTargetFactory subclasses
// (i.e. objc_binary) don't have to depend on it.
ObjcProvider protobufObjcProvider =
ruleContext.getPrerequisite(
ObjcRuleClasses.PROTO_LIB_ATTR, Mode.TARGET, ObjcProvider.SKYLARK_CONSTRUCTOR);
ObjcConfiguration objcConfiguration =
ruleContext.getConfiguration().getFragment(ObjcConfiguration.class);
CcCompilationContext protobufCcCompilationContext;
if (objcConfiguration.compileInfoMigration()) {
CcInfo protobufCcInfo =
ruleContext.getPrerequisite(
ObjcRuleClasses.PROTO_LIB_ATTR, Mode.TARGET, CcInfo.PROVIDER);
protobufCcCompilationContext = protobufCcInfo.getCcCompilationContext();
} else {
ObjcProvider protobufObjcProvider =
ruleContext.getPrerequisite(
ObjcRuleClasses.PROTO_LIB_ATTR, Mode.TARGET, ObjcProvider.SKYLARK_CONSTRUCTOR);
protobufCcCompilationContext = protobufObjcProvider.getCcCompilationContext();
}
aspectObjcProtoProvider.addProtobufHeaders(
protobufObjcProvider.getCcCompilationContext().getDeclaredIncludeSrcs());
protobufCcCompilationContext.getDeclaredIncludeSrcs());
aspectObjcProtoProvider.addProtobufHeaderSearchPaths(
NestedSetBuilder.<PathFragment>linkOrder()
.addAll(protobufObjcProvider.getCcCompilationContext().getIncludeDirs())
.addAll(protobufCcCompilationContext.getIncludeDirs())
.build());
}

Expand Down
20 changes: 17 additions & 3 deletions src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -503,9 +503,23 @@ public void testArchivesPrecompiledObjectFiles() throws Exception {
}

@Test
public void testCompileWithFrameworkImportsIncludesFlags() throws Exception {
useConfiguration("--crosstool_top=" + MockObjcSupport.DEFAULT_OSX_CROSSTOOL);
addBinWithTransitiveDepOnFrameworkImport();
public void testCompileWithFrameworkImportsIncludesFlagsPreMigration() throws Exception {
useConfiguration(
"--crosstool_top=" + MockObjcSupport.DEFAULT_OSX_CROSSTOOL,
"--incompatible_objc_compile_info_migration=false");
addBinWithTransitiveDepOnFrameworkImport(false);
CommandAction compileAction = compileAction("//lib:lib", "a.o");

assertThat(compileAction.getArguments()).doesNotContain("-framework");
assertThat(Joiner.on("").join(compileAction.getArguments())).contains("-Ffx");
}

@Test
public void testCompileWithFrameworkImportsIncludesFlagsPostMigration() throws Exception {
useConfiguration(
"--crosstool_top=" + MockObjcSupport.DEFAULT_OSX_CROSSTOOL,
"--incompatible_objc_compile_info_migration=true");
addBinWithTransitiveDepOnFrameworkImport(true);
CommandAction compileAction = compileAction("//lib:lib", "a.o");

assertThat(compileAction.getArguments()).doesNotContain("-framework");
Expand Down
Loading

0 comments on commit be1d118

Please sign in to comment.