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 bazelbuild/bazel#10854.
    PiperOrigin-RevId: 298439046
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent 9c716b0 commit f563633
Show file tree
Hide file tree
Showing 8 changed files with 70 additions and 100 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,13 @@ public class ObjcCommandLineOptions extends FragmentOptions {
public boolean generateLinkmap;

@Option(
name = "objccopt",
allowMultiple = true,
defaultValue = "null",
documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
effectTags = {OptionEffectTag.ACTION_COMMAND_LINES},
help = "Additional options to pass to Objective C compilation.")
name = "objccopt",
allowMultiple = true,
defaultValue = "",
documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
effectTags = {OptionEffectTag.ACTION_COMMAND_LINES},
help = "Additional options to pass to Objective C compilation."
)
public List<String> copts;

@Option(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ static class Builder {
throws InterruptedException {
this.purpose = purpose;
this.context = Preconditions.checkNotNull(context);
this.semantics = context.getAnalysisEnvironment().getStarlarkSemantics();
this.semantics = context.getAnalysisEnvironment().getSkylarkSemantics();
this.buildConfiguration = Preconditions.checkNotNull(buildConfiguration);

ObjcConfiguration objcConfiguration = buildConfiguration.getFragment(ObjcConfiguration.class);
Expand Down Expand Up @@ -190,8 +190,8 @@ private Builder addDepsPreMigration(List<ConfiguredTargetAndData> 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);
if (depCT.get(ObjcProvider.SKYLARK_CONSTRUCTOR) != null) {
addAnyProviders(propagatedObjcDeps, depCT, ObjcProvider.SKYLARK_CONSTRUCTOR);
} else {
addAnyProviders(cppDeps, depCT, CcInfo.PROVIDER);
if (isCcLibrary(dep)) {
Expand All @@ -216,8 +216,8 @@ private Builder addDepsPostMigration(List<ConfiguredTargetAndData> deps) {

for (ConfiguredTargetAndData dep : deps) {
ConfiguredTarget depCT = dep.getConfiguredTarget();
if (depCT.get(ObjcProvider.STARLARK_CONSTRUCTOR) != null) {
addAnyProviders(propagatedObjcDeps, depCT, ObjcProvider.STARLARK_CONSTRUCTOR);
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);
Expand Down Expand Up @@ -248,7 +248,7 @@ private Builder addRuntimeDepsPreMigration(
ImmutableList.Builder<ObjcProvider> propagatedDeps = ImmutableList.builder();

for (TransitiveInfoCollection dep : runtimeDeps) {
addAnyProviders(propagatedDeps, dep, ObjcProvider.STARLARK_CONSTRUCTOR);
addAnyProviders(propagatedDeps, dep, ObjcProvider.SKYLARK_CONSTRUCTOR);
}
this.runtimeDepObjcProviders =
Iterables.concat(this.runtimeDepObjcProviders, propagatedDeps.build());
Expand All @@ -261,7 +261,7 @@ private Builder addRuntimeDepsPostMigration(
ImmutableList.Builder<CcInfo> cppDeps = ImmutableList.builder();

for (TransitiveInfoCollection dep : runtimeDeps) {
addAnyProviders(propagatedObjcDeps, dep, ObjcProvider.STARLARK_CONSTRUCTOR);
addAnyProviders(propagatedObjcDeps, dep, ObjcProvider.SKYLARK_CONSTRUCTOR);
addAnyProviders(cppDeps, dep, CcInfo.PROVIDER);
}
this.runtimeDepObjcProviders =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,8 @@ public CcCompilationContext createCcCompilationContext() {
.addDeclaredIncludeSrcs(getPublicHeaders())
.addDeclaredIncludeSrcs(getPrivateHeaders())
.addDeclaredIncludeSrcs(getPublicTextualHeaders())
.addModularPublicHdrs(ImmutableList.copyOf(getPublicHeaders()))
.addModularPrivateHdrs(ImmutableList.copyOf(getPrivateHeaders()))
.addModularHdrs(ImmutableList.copyOf(getPublicHeaders()))
.addModularHdrs(ImmutableList.copyOf(getPrivateHeaders()))
.addTextualHdrs(ImmutableList.copyOf(getPublicTextualHeaders()))
.addIncludeDirs(getIncludes())
.addSystemIncludeDirs(getSystemIncludes())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,19 @@
public class ObjcConfiguration extends BuildConfiguration.Fragment
implements ObjcConfigurationApi<PlatformType> {
@VisibleForTesting
static final ImmutableList<String> DBG_COPTS =
ImmutableList.of("-O0", "-DDEBUG=1", "-fstack-protector", "-fstack-protector-all", "-g");

@VisibleForTesting
static final ImmutableList<String> GLIBCXX_DBG_COPTS =
ImmutableList.of(
"-D_GLIBCXX_DEBUG", "-D_GLIBCXX_DEBUG_PEDANTIC", "-D_GLIBCPP_CONCEPT_CHECKS");

@VisibleForTesting
static final ImmutableList<String> OPT_COPTS =
ImmutableList.of(
"-Os", "-DNDEBUG=1", "-Wno-unused-variable", "-Winit-self", "-Wno-extra");

private final DottedVersion iosSimulatorVersion;
private final String iosSimulatorDevice;
private final DottedVersion watchosSimulatorVersion;
Expand Down Expand Up @@ -171,14 +180,17 @@ public ImmutableList<String> getCoptsForCompilationMode() {
switch (compilationMode) {
case DBG:
if (this.debugWithGlibcxx) {
return GLIBCXX_DBG_COPTS;
return ImmutableList.<String>builder()
.addAll(DBG_COPTS)
.addAll(GLIBCXX_DBG_COPTS)
.build();
} else {
return ImmutableList.of();
return DBG_COPTS;
}
case FASTBUILD:
return fastbuildOptions;
case OPT:
return ImmutableList.of();
return OPT_COPTS;
default:
throw new AssertionError();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@
import com.google.devtools.build.lib.analysis.ConfiguredAspectFactory;
import com.google.devtools.build.lib.analysis.PrerequisiteArtifacts;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.TransitionMode;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.packages.AspectDefinition;
import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.StarlarkNativeAspect;
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;
Expand All @@ -39,7 +39,7 @@
* Aspect that gathers the proto dependencies of the attached rule target, and propagates the proto
* values of its dependencies through the ObjcProtoProvider.
*/
public class ObjcProtoAspect extends StarlarkNativeAspect implements ConfiguredAspectFactory {
public class ObjcProtoAspect extends SkylarkNativeAspect implements ConfiguredAspectFactory {
public static final String NAME = "ObjcProtoAspect";

@Override
Expand All @@ -56,14 +56,14 @@ public ConfiguredAspect create(
AspectParameters parameters,
String toolsRepository)
throws InterruptedException, ActionConflictException {
ConfiguredAspect.Builder aspectBuilder = new ConfiguredAspect.Builder(ruleContext);
ConfiguredAspect.Builder aspectBuilder = new ConfiguredAspect.Builder(
this, parameters, ruleContext);

ObjcProtoProvider.Builder aspectObjcProtoProvider = new ObjcProtoProvider.Builder();

if (ruleContext.attributes().has("deps", BuildType.LABEL_LIST)) {
Iterable<ObjcProtoProvider> depObjcProtoProviders =
ruleContext.getPrerequisites(
"deps", TransitionMode.TARGET, ObjcProtoProvider.STARLARK_CONSTRUCTOR);
ruleContext.getPrerequisites("deps", Mode.TARGET, ObjcProtoProvider.SKYLARK_CONSTRUCTOR);
aspectObjcProtoProvider.addTransitive(depObjcProtoProviders);
}

Expand All @@ -76,15 +76,15 @@ public ConfiguredAspect create(

// Gather up all the dependency protos depended by this target.
List<ProtoInfo> protoInfos =
ruleContext.getPrerequisites("deps", TransitionMode.TARGET, ProtoInfo.PROVIDER);
ruleContext.getPrerequisites("deps", Mode.TARGET, ProtoInfo.PROVIDER);

for (ProtoInfo protoInfo : protoInfos) {
aspectObjcProtoProvider.addProtoFiles(protoInfo.getTransitiveProtoSources());
}

NestedSet<Artifact> portableProtoFilters =
PrerequisiteArtifacts.nestedSet(
ruleContext, ProtoAttributes.PORTABLE_PROTO_FILTERS_ATTR, TransitionMode.HOST);
ruleContext, ProtoAttributes.PORTABLE_PROTO_FILTERS_ATTR, Mode.HOST);

// If this target does not provide filters but specifies direct proto_library dependencies,
// generate a filter file only for those proto files.
Expand All @@ -106,14 +106,12 @@ public ConfiguredAspect create(
if (objcConfiguration.compileInfoMigration()) {
CcInfo protobufCcInfo =
ruleContext.getPrerequisite(
ObjcRuleClasses.PROTO_LIB_ATTR, TransitionMode.TARGET, CcInfo.PROVIDER);
ObjcRuleClasses.PROTO_LIB_ATTR, Mode.TARGET, CcInfo.PROVIDER);
protobufCcCompilationContext = protobufCcInfo.getCcCompilationContext();
} else {
ObjcProvider protobufObjcProvider =
ruleContext.getPrerequisite(
ObjcRuleClasses.PROTO_LIB_ATTR,
TransitionMode.TARGET,
ObjcProvider.STARLARK_CONSTRUCTOR);
ObjcRuleClasses.PROTO_LIB_ATTR, Mode.TARGET, ObjcProvider.SKYLARK_CONSTRUCTOR);
protobufCcCompilationContext = protobufObjcProvider.getCcCompilationContext();
}
aspectObjcProtoProvider.addProtobufHeaders(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import static com.google.devtools.build.lib.rules.objc.ObjcProvider.WEAK_SDK_FRAMEWORK;
import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.NON_ARC_SRCS_TYPE;
import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.SRCS_TYPE;
import static org.junit.Assert.assertThrows;
import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;

import com.google.common.base.Joiner;
import com.google.common.base.Optional;
Expand All @@ -49,8 +49,6 @@
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.util.MockObjcSupport;
import com.google.devtools.build.lib.rules.apple.AppleToolchain;
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.cpp.CppCompileAction;
import com.google.devtools.build.lib.rules.cpp.CppModuleMap;
import com.google.devtools.build.lib.rules.cpp.CppModuleMapAction;
Expand Down Expand Up @@ -1024,11 +1022,11 @@ public void testProvidesObjcLibraryAndHeaders() throws Exception {
private static Iterable<String> getArifactPaths(
ConfiguredTarget target, ObjcProvider.Key<Artifact> artifactKey) {
return Artifact.toRootRelativePaths(
target.get(ObjcProvider.STARLARK_CONSTRUCTOR).get(artifactKey));
target.get(ObjcProvider.SKYLARK_CONSTRUCTOR).get(artifactKey));
}

private static Iterable<String> getArifactPathsOfHeaders(ConfiguredTarget target) {
return Artifact.toRootRelativePaths(target.get(ObjcProvider.STARLARK_CONSTRUCTOR).header());
return Artifact.toRootRelativePaths(target.get(ObjcProvider.SKYLARK_CONSTRUCTOR).header());
}

@Test
Expand Down Expand Up @@ -1903,37 +1901,27 @@ public void testToolchainRuntimeLibrariesSolibDir() throws Exception {
@Test
public void testDirectFields() throws Exception {
useConfiguration("--crosstool_top=" + MockObjcSupport.DEFAULT_OSX_CROSSTOOL);

scratch.file(
"x/BUILD",
"objc_library(",
" name = 'foo',",
" srcs = ['foo.m', 'foo_impl.h'],",
" hdrs = ['foo.h'],",
" textual_hdrs = ['foo.inc'],",
" name = 'foo',",
" srcs = ['foo.m'],",
" hdrs = ['foo.h'],",
")",
"objc_library(",
" name = 'bar',",
" srcs = ['bar.m', 'bar_impl.h'],",
" srcs = ['bar.m'],",
" hdrs = ['bar.h'],",
" textual_hdrs = ['bar.inc'],",
" deps = [':foo'],",
")");

ObjcProvider dependerProvider = providerForTarget("//x:bar");
assertThat(baseArtifactNames(dependerProvider.getDirect(ObjcProvider.HEADER)))
.containsExactly("bar.h", "bar.inc");
.containsExactly("bar.h");
assertThat(baseArtifactNames(dependerProvider.getDirect(ObjcProvider.SOURCE)))
.containsExactly("bar.m");
assertThat(Artifact.toRootRelativePaths(dependerProvider.getDirect(ObjcProvider.MODULE_MAP)))
.containsExactly("x/bar.modulemaps/module.modulemap");

ConfiguredTarget target = getConfiguredTarget("//x:bar");
CcCompilationContext ccCompilationContext =
target.get(CcInfo.PROVIDER).getCcCompilationContext();
assertThat(baseArtifactNames(ccCompilationContext.getDirectHdrs()))
.containsExactly("bar.h", "bar_impl.h");
assertThat(baseArtifactNames(ccCompilationContext.getTextualHdrs())).containsExactly("bar.inc");
}

@Test
Expand Down Expand Up @@ -1969,22 +1957,4 @@ private void setupTestObjcLibraryLoadedThroughMacro(boolean loadMacro) throws Ex
getAnalysisMock().ccSupport().getMacroLoadStatement(loadMacro, "objc_library"),
"objc_library(name='a', srcs=['a.cc'])");
}

@Test
public void testGenerateDsymFlagPropagatesToObjcLibraryFeature() throws Exception {
useConfiguration("--apple_generate_dsym");
createLibraryTargetWriter("//objc/lib").setList("srcs", "a.m").write();
CommandAction compileAction = compileAction("//objc/lib", "a.o");
assertThat(compileAction.getArguments()).contains("-DDUMMY_GENERATE_DSYM_FILE");
}

@Test
public void testGenerateDsymFlagPropagatesToCcLibraryFeature() throws Exception {
useConfiguration("--apple_generate_dsym");
ScratchAttributeWriter.fromLabelString(this, "cc_library", "//cc/lib")
.setList("srcs", "a.cc")
.write();
CommandAction compileAction = compileAction("//cc/lib", "a.o");
assertThat(compileAction.getArguments()).contains("-DDUMMY_GENERATE_DSYM_FILE");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,20 @@ private static String compilationModeFlag(CompilationMode mode) {
throw new AssertionError();
}

private static List<String> compilationModeCopts(CompilationMode mode) {
switch (mode) {
case DBG:
return ImmutableList.<String>builder()
.addAll(ObjcConfiguration.DBG_COPTS)
.build();
case OPT:
return ObjcConfiguration.OPT_COPTS;
case FASTBUILD:
return FASTBUILD_COPTS;
}
throw new AssertionError();
}

@Override
protected void useConfiguration(String... args) throws Exception {
ImmutableList<String> extraArgs = MockObjcSupport.requiredObjcCrosstoolFlags();
Expand Down Expand Up @@ -300,8 +314,7 @@ protected List<BuildConfiguration> getSplitConfigurations(
throws InterruptedException, OptionsParsingException, InvalidConfigurationException {
ImmutableList.Builder<BuildConfiguration> splitConfigs = ImmutableList.builder();

for (BuildOptions splitOptions :
splitTransition.split(configuration.getOptions(), eventCollector).values()) {
for (BuildOptions splitOptions : splitTransition.split(configuration.getOptions()).values()) {
splitConfigs.add(getSkyframeExecutor().getConfigurationForTesting(
reporter, configuration.fragmentClasses(), splitOptions));
}
Expand Down Expand Up @@ -958,8 +971,9 @@ protected void checkErrorsWrongFileTypeForSrcsWhenCompiling(RuleType ruleType)

protected void checkClangCoptsForCompilationMode(RuleType ruleType, CompilationMode mode,
CodeCoverageMode codeCoverageMode) throws Exception {
ImmutableList.Builder<String> allExpectedCoptsBuilder =
ImmutableList.<String>builder().addAll(CompilationSupport.DEFAULT_COMPILER_FLAGS);
ImmutableList.Builder<String> allExpectedCoptsBuilder = ImmutableList.<String>builder()
.addAll(CompilationSupport.DEFAULT_COMPILER_FLAGS)
.addAll(compilationModeCopts(mode));

switch (codeCoverageMode) {
case NONE:
Expand Down Expand Up @@ -993,8 +1007,9 @@ protected void checkClangCoptsForCompilationMode(RuleType ruleType, CompilationM
}

protected void checkClangCoptsForDebugModeWithoutGlib(RuleType ruleType) throws Exception {
ImmutableList.Builder<String> allExpectedCoptsBuilder =
ImmutableList.<String>builder().addAll(CompilationSupport.DEFAULT_COMPILER_FLAGS);
ImmutableList.Builder<String> allExpectedCoptsBuilder = ImmutableList.<String>builder()
.addAll(CompilationSupport.DEFAULT_COMPILER_FLAGS)
.addAll(ObjcConfiguration.DBG_COPTS);

useConfiguration(
"--apple_platform_type=ios", "--compilation_mode=dbg", "--objc_debug_with_GLIBCXX=false");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1225,32 +1225,6 @@ public void testSkylarkCanCreateObjcProviderWithStrictDepsPostMigration() throws
assertThat(skylarkProviderDirectDepender.include()).isEmpty();
}

@Test
public void testSkylarkCanCreateObjcProviderWithStrictDepsDirectly() throws Exception {
ConfiguredTarget skylarkTarget =
createObjcProviderSkylarkTarget(
" strict_includes = depset(['path'])",
" created_provider = apple_common.new_objc_provider\\",
"(strict_include=strict_includes)",
" return [created_provider]");

ObjcProvider skylarkProvider = skylarkTarget.get(ObjcProvider.SKYLARK_CONSTRUCTOR);
assertThat(skylarkProvider.getStrictDependencyIncludes())
.containsExactly(PathFragment.create("path"));

scratch.file(
"examples/objc_skylark2/BUILD",
"objc_library(",
" name = 'direct_dep',",
" deps = ['//examples/objc_skylark:my_target']",
")");

ObjcProvider skylarkProviderDirectDepender =
getConfiguredTarget("//examples/objc_skylark2:direct_dep")
.get(ObjcProvider.SKYLARK_CONSTRUCTOR);
assertThat(skylarkProviderDirectDepender.getStrictDependencyIncludes()).isEmpty();
}

@Test
public void testSkylarkStrictDepsDoesNotSupportDefine() throws Exception {
AssertionError e =
Expand Down

0 comments on commit f563633

Please sign in to comment.