Skip to content

Commit

Permalink
Automated rollback of commit bcdd55d.
Browse files Browse the repository at this point in the history
*** Reason for rollback ***

Broke some tests

*** Original change description ***

Delete --incompatible_objc_compile_info_migration

RELNOTES: Flag --incompatible_objc_compile_info_migration is removed.  See #10854.
PiperOrigin-RevId: 343856408
  • Loading branch information
googlewalt authored and copybara-github committed Nov 23, 2020
1 parent ecbf463 commit 10e4472
Show file tree
Hide file tree
Showing 10 changed files with 336 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,21 @@ public class ObjcCommandLineOptions extends FragmentOptions {
)
public Label appleSdk;

@Option(
name = "incompatible_objc_compile_info_migration",
defaultValue = "true",
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;

@Option(
name = "incompatible_avoid_hardcoded_objc_compilation_flags",
defaultValue = "false",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,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 Down Expand Up @@ -137,6 +138,10 @@ static class Builder {
this.context = Preconditions.checkNotNull(context);
this.semantics = context.getAnalysisEnvironment().getStarlarkSemantics();
this.buildConfiguration = Preconditions.checkNotNull(buildConfiguration);

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

this.compileInfoMigration = objcConfiguration.compileInfoMigration();
}

public Builder setCompilationAttributes(CompilationAttributes baseCompilationAttributes) {
Expand Down Expand Up @@ -176,8 +181,34 @@ Builder addDepCcDirectProviders(Iterable<CcInfo> cppDeps) {
return this;
}

/** Add the providers for the build dependencies. */
Builder addDeps(List<ConfiguredTargetAndData> deps) {
private Builder addDepsPreMigration(List<ConfiguredTargetAndData> deps) {
ImmutableList.Builder<ObjcProvider> propagatedObjcDeps = ImmutableList.builder();
ImmutableList.Builder<CcInfo> cppDeps = ImmutableList.builder();
ImmutableList.Builder<CcLinkingContext> cppDepLinkParams = ImmutableList.builder();

for (ConfiguredTargetAndData dep : deps) {
ConfiguredTarget depCT = dep.getConfiguredTarget();
// It is redundant to process both ObjcProvider and CcInfo; doing so causes direct header
// field to include indirect headers from deps.
if (depCT.get(ObjcProvider.STARLARK_CONSTRUCTOR) != null) {
addAnyProviders(propagatedObjcDeps, depCT, ObjcProvider.STARLARK_CONSTRUCTOR);
} else {
addAnyProviders(cppDeps, depCT, CcInfo.PROVIDER);
if (isCcLibrary(dep)) {
cppDepLinkParams.add(depCT.get(CcInfo.PROVIDER).getCcLinkingContext());
}
}
}
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();
Expand All @@ -204,11 +235,28 @@ Builder addDeps(List<ConfiguredTargetAndData> deps) {
return this;
}

/**
* Adds providers for runtime frameworks included in the final app bundle but not linked with at
* build time.
*/
Builder addRuntimeDeps(List<? extends TransitiveInfoCollection> runtimeDeps) {
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.STARLARK_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();

Expand All @@ -222,6 +270,18 @@ Builder addRuntimeDeps(List<? extends TransitiveInfoCollection> runtimeDeps) {
return this;
}

/**
* Adds providers for runtime frameworks included in the final app bundle but not linked with
* at build time.
*/
Builder addRuntimeDeps(List<? extends TransitiveInfoCollection> runtimeDeps) {
if (compileInfoMigration) {
return addRuntimeDepsPostMigration(runtimeDeps);
} else {
return addRuntimeDepsPreMigration(runtimeDeps);
}
}

private <T extends Info> ImmutableList.Builder<T> addAnyProviders(
ImmutableList.Builder<T> listBuilder,
TransitiveInfoCollection collection,
Expand Down Expand Up @@ -296,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 Down
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,12 +126,12 @@ public CcCompilationContext createCcCompilationContext() {
return builder.build();
}

/** Create and return an initial empty Builder for ObjcCompilationContext. */
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 @@ -142,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 @@ -181,6 +183,9 @@ public Builder addQuoteIncludes(Iterable<PathFragment> includes) {

public Builder addDepObjcProviders(Iterable<ObjcProvider> objcProviders) {
for (ObjcProvider objcProvider : objcProviders) {
if (!compileInfoMigration) {
this.depCcCompilationContexts.add(objcProvider.getCcCompilationContext());
}
this.strictDependencyIncludes.addAll(objcProvider.getStrictDependencyIncludes());
}
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ public class ObjcConfiguration extends Fragment implements ObjcConfigurationApi<
private final boolean enableAppleBinaryNativeProtos;
private final HeaderDiscovery.DotdPruningMode dotdPruningPlan;
private final boolean shouldScanIncludes;
private final boolean compileInfoMigration;
private final boolean avoidHardcodedCompilationFlags;

public ObjcConfiguration(BuildOptions buildOptions) {
Expand Down Expand Up @@ -100,6 +101,7 @@ public ObjcConfiguration(BuildOptions buildOptions) {
? HeaderDiscovery.DotdPruningMode.USE
: HeaderDiscovery.DotdPruningMode.DO_NOT_USE;
this.shouldScanIncludes = objcOptions.scanIncludes;
this.compileInfoMigration = objcOptions.incompatibleObjcCompileInfoMigration;
this.avoidHardcodedCompilationFlags =
objcOptions.incompatibleAvoidHardcodedObjcCompilationFlags;
}
Expand Down Expand Up @@ -260,4 +262,9 @@ public HeaderDiscovery.DotdPruningMode getDotdPruningPlan() {
public boolean shouldScanIncludes() {
return shouldScanIncludes;
}

/** Whether native rules can assume compile info has been migrated to CcInfo. */
public boolean compileInfoMigration() {
return compileInfoMigration;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,20 @@ 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.
CcInfo protobufCcInfo =
ruleContext.getPrerequisite(ObjcRuleClasses.PROTO_LIB_ATTR, CcInfo.PROVIDER);
CcCompilationContext protobufCcCompilationContext = protobufCcInfo.getCcCompilationContext();
ObjcConfiguration objcConfiguration =
ruleContext.getConfiguration().getFragment(ObjcConfiguration.class);
CcCompilationContext protobufCcCompilationContext;
if (objcConfiguration.compileInfoMigration()) {
CcInfo protobufCcInfo =
ruleContext.getPrerequisite(ObjcRuleClasses.PROTO_LIB_ATTR, CcInfo.PROVIDER);
protobufCcCompilationContext = protobufCcInfo.getCcCompilationContext();
} else {
ObjcProvider protobufObjcProvider =
ruleContext.getPrerequisite(
ObjcRuleClasses.PROTO_LIB_ATTR,
ObjcProvider.STARLARK_CONSTRUCTOR);
protobufCcCompilationContext = protobufObjcProvider.getCcCompilationContext();
}
aspectObjcProtoProvider.addProtobufHeaders(
protobufCcCompilationContext.getDeclaredIncludeSrcs());
aspectObjcProtoProvider.addProtobufHeaderSearchPaths(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,7 @@ private ConfiguredTarget getConfiguredTargetInAppleBinaryTransition(String label
return getConfiguredTarget(label, childConfig);
}

@Test
public void testJ2ObjCInformationExportedFromJ2ObjcLibrary() throws Exception {
private void testJ2ObjCInformationExportedFromJ2ObjcLibrary() throws Exception {
ConfiguredTarget j2objcLibraryTarget = getConfiguredTarget(
"//java/com/google/dummy/test:transpile");
ObjcProvider provider = j2objcLibraryTarget.get(ObjcProvider.STARLARK_CONSTRUCTOR);
Expand All @@ -100,6 +99,23 @@ public void testJ2ObjCInformationExportedFromJ2ObjcLibrary() throws Exception {
.containsExactly(execPath + "java/com/google/dummy/test/_j2objc/test");
}

@Test
public void testJ2ObjCInformationExportedFromJ2ObjcLibraryPreMigration() throws Exception {
useConfiguration(
"--proto_toolchain_for_java=//tools/proto/toolchains:java",
"--incompatible_objc_compile_info_migration=false");
setBuildLanguageOptions("--incompatible_objc_provider_remove_compile_info=false");
testJ2ObjCInformationExportedFromJ2ObjcLibrary();
}

@Test
public void testJ2ObjCInformationExportedFromJ2ObjcLibraryPostMigration() throws Exception {
useConfiguration(
"--proto_toolchain_for_java=//tools/proto/toolchains:java",
"--incompatible_objc_compile_info_migration=true");
testJ2ObjCInformationExportedFromJ2ObjcLibrary();
}

@Test
public void testJ2ObjCInformationExportedWithGeneratedJavaSources() throws Exception {
scratch.file("java/com/google/test/in.txt");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -509,9 +509,24 @@ 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");
setBuildLanguageOptions("--incompatible_objc_provider_remove_compile_info=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
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,7 @@ public void testObjcProtoAspectPropagatesProvider() throws Exception {
assertThat(objcProtoProvider).isNotNull();
}

@Test
public void testObjcProtoAspectPropagatesProtobufProvider() throws Exception {
private void testObjcProtoAspectPropagatesProtobufProvider() throws Exception {
MockObjcSupport.setupObjcProtoLibrary(scratch);
scratch.file("x/data_filter.pbascii");
scratch.file(
Expand Down Expand Up @@ -103,6 +102,19 @@ public void testObjcProtoAspectPropagatesProtobufProvider() throws Exception {
.containsExactly(includePath, genIncludePath);
}

@Test
public void testObjcProtoAspectPropagatesProtobufProviderPreMigration() throws Exception {
useConfiguration("--incompatible_objc_compile_info_migration=false");
setBuildLanguageOptions("--incompatible_objc_provider_remove_compile_info=false");
testObjcProtoAspectPropagatesProtobufProvider();
}

@Test
public void testObjcProtoAspectPropagatesProtobufProviderPostMigration() throws Exception {
useConfiguration("--incompatible_objc_compile_info_migration=true");
testObjcProtoAspectPropagatesProtobufProvider();
}

@Test
public void testObjcProtoAspectDoesNotPropagateProviderWhenNoProtos() throws Exception {
scratch.file("x/BUILD",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -470,15 +470,42 @@ protected void assertRequiresDarwin(Action action) {
assertHasRequirement(action, ExecutionRequirements.REQUIRES_DARWIN);
}

protected ConfiguredTarget addBinWithTransitiveDepOnFrameworkImport() throws Exception {
ConfiguredTarget lib = addLibWithDepOnFrameworkImport();
protected ConfiguredTarget addBinWithTransitiveDepOnFrameworkImport(boolean compileInfoMigration)
throws Exception {
ConfiguredTarget lib =
compileInfoMigration
? addLibWithDepOnFrameworkImportPostMigration()
: addLibWithDepOnFrameworkImportPreMigration();
return createBinaryTargetWriter("//bin:bin")
.setList("deps", lib.getLabel().toString())
.write();

}

private ConfiguredTarget addLibWithDepOnFrameworkImport() throws Exception {
private ConfiguredTarget addLibWithDepOnFrameworkImportPreMigration() throws Exception {
scratch.file(
"fx/defs.bzl",
"def _custom_static_framework_import_impl(ctx):",
" return [apple_common.new_objc_provider(",
" framework_search_paths=depset(ctx.attr.framework_search_paths))]",
"custom_static_framework_import = rule(",
" _custom_static_framework_import_impl,",
" attrs={'framework_search_paths': attr.string_list()},",
")");
scratch.file(
"fx/BUILD",
"load(':defs.bzl', 'custom_static_framework_import')",
"custom_static_framework_import(",
" name = 'fx',",
" framework_search_paths = ['fx/fx1.framework', 'fx/fx2.framework'],",
")");
return createLibraryTargetWriter("//lib:lib")
.setAndCreateFiles("srcs", "a.m", "b.m", "private.h")
.setList("deps", "//fx:fx")
.write();
}

private ConfiguredTarget addLibWithDepOnFrameworkImportPostMigration() throws Exception {
scratch.file(
"fx/defs.bzl",
"def _custom_static_framework_import_impl(ctx):",
Expand Down
Loading

0 comments on commit 10e4472

Please sign in to comment.