Skip to content

Commit

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

Rollback of rollback: breakages fixed by unknown commit

*** Original change description ***

Automated rollback of commit bcdd55d.

*** 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: 344242019
  • Loading branch information
googlewalt authored and copybara-github committed Nov 25, 2020
1 parent e3b7e17 commit c6bf178
Show file tree
Hide file tree
Showing 10 changed files with 29 additions and 336 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -237,21 +237,6 @@ 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,7 +100,6 @@ 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 @@ -138,10 +137,6 @@ 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 @@ -181,34 +176,8 @@ Builder addDepCcDirectProviders(Iterable<CcInfo> 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();

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) {
/** Add the providers for the build dependencies. */
Builder addDeps(List<ConfiguredTargetAndData> deps) {
ImmutableList.Builder<ObjcProvider> propagatedObjcDeps = ImmutableList.builder();
ImmutableList.Builder<CcInfo> cppDeps = ImmutableList.builder();
ImmutableList.Builder<CcInfo> directCppDeps = ImmutableList.builder();
Expand All @@ -235,28 +204,11 @@ private Builder addDepsPostMigration(List<ConfiguredTargetAndData> deps) {
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.STARLARK_CONSTRUCTOR);
}
this.runtimeDepObjcProviders =
Iterables.concat(this.runtimeDepObjcProviders, propagatedDeps.build());
return this;
}

private Builder addRuntimeDepsPostMigration(
List<? extends TransitiveInfoCollection> runtimeDeps) {
/**
* Adds providers for runtime frameworks included in the final app bundle but not linked with at
* build time.
*/
Builder addRuntimeDeps(List<? extends TransitiveInfoCollection> runtimeDeps) {
ImmutableList.Builder<ObjcProvider> propagatedObjcDeps = ImmutableList.builder();
ImmutableList.Builder<CcInfo> cppDeps = ImmutableList.builder();

Expand All @@ -270,18 +222,6 @@ private Builder addRuntimeDepsPostMigration(
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 @@ -356,7 +296,7 @@ Builder setLinkedBinary(Artifact linkedBinary) {
ObjcCommon build() {

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

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(false).build();
public static final ObjcCompilationContext EMPTY = builder().build();

private final ImmutableList<String> defines;

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

public static Builder builder(boolean compileInfoMigration) {
return new Builder(compileInfoMigration);
/** Create and return an initial empty Builder for ObjcCompilationContext. */
public static Builder builder() {
return new Builder();
}

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,9 +142,7 @@ static class Builder {
private final List<PathFragment> strictDependencyIncludes = new ArrayList<>();
private final List<CcCompilationContext> depCcCompilationContexts = new ArrayList<>();

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

public Builder addDefines(Iterable<String> defines) {
Iterables.addAll(this.defines, defines);
Expand Down Expand Up @@ -183,9 +181,6 @@ 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,7 +68,6 @@ 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 @@ -101,7 +100,6 @@ 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 @@ -262,9 +260,4 @@ 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,20 +96,9 @@ 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.
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();
}
CcInfo protobufCcInfo =
ruleContext.getPrerequisite(ObjcRuleClasses.PROTO_LIB_ATTR, CcInfo.PROVIDER);
CcCompilationContext protobufCcCompilationContext = protobufCcInfo.getCcCompilationContext();
aspectObjcProtoProvider.addProtobufHeaders(
protobufCcCompilationContext.getDeclaredIncludeSrcs());
aspectObjcProtoProvider.addProtobufHeaderSearchPaths(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ private ConfiguredTarget getConfiguredTargetInAppleBinaryTransition(String label
return getConfiguredTarget(label, childConfig);
}

private void testJ2ObjCInformationExportedFromJ2ObjcLibrary() throws Exception {
@Test
public void testJ2ObjCInformationExportedFromJ2ObjcLibrary() throws Exception {
ConfiguredTarget j2objcLibraryTarget = getConfiguredTarget(
"//java/com/google/dummy/test:transpile");
ObjcProvider provider = j2objcLibraryTarget.get(ObjcProvider.STARLARK_CONSTRUCTOR);
Expand All @@ -99,23 +100,6 @@ private 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,24 +509,9 @@ public void testArchivesPrecompiledObjectFiles() throws Exception {
}

@Test
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);
public void testCompileWithFrameworkImportsIncludesFlags() throws Exception {
useConfiguration("--crosstool_top=" + MockObjcSupport.DEFAULT_OSX_CROSSTOOL);
addBinWithTransitiveDepOnFrameworkImport();
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,7 +69,8 @@ public void testObjcProtoAspectPropagatesProvider() throws Exception {
assertThat(objcProtoProvider).isNotNull();
}

private void testObjcProtoAspectPropagatesProtobufProvider() throws Exception {
@Test
public void testObjcProtoAspectPropagatesProtobufProvider() throws Exception {
MockObjcSupport.setupObjcProtoLibrary(scratch);
scratch.file("x/data_filter.pbascii");
scratch.file(
Expand Down Expand Up @@ -102,19 +103,6 @@ private 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,42 +470,15 @@ protected void assertRequiresDarwin(Action action) {
assertHasRequirement(action, ExecutionRequirements.REQUIRES_DARWIN);
}

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

}

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 {
private ConfiguredTarget addLibWithDepOnFrameworkImport() throws Exception {
scratch.file(
"fx/defs.bzl",
"def _custom_static_framework_import_impl(ctx):",
Expand Down
Loading

0 comments on commit c6bf178

Please sign in to comment.