Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix objc transition for cc_binary -- simplifying change #19236

Closed
wants to merge 7 commits into from
Closed

Fix objc transition for cc_binary -- simplifying change #19236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
921d24d
Fix objc transition for cc_binary
cpsauer Aug 12, 2023
89fdd87
Add missing --cpu settings to ObjcRuleTestCase.java
cpsauer Aug 12, 2023
2902272
Add missing --cpu settings to ObjcLibraryTest.java
cpsauer Aug 12, 2023
2f85186
Don't set iOS cpu for macOS test in ObjcLibraryTest.java
cpsauer Aug 12, 2023
3154e23
Actually don't set iOS cpu for macOS test in ObjcLibraryTest.java
cpsauer Aug 12, 2023
a6eccce
Update ObjcLibraryTest.java to reflect expectation that generated sta…
cpsauer Aug 12, 2023
61c29a7
Remove hardcoded but unspecified configuration paths in BazelJ2ObjcLi…
cpsauer Aug 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 5 additions & 68 deletions src/main/starlark/builtins_bzl/common/objc/transitions.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,75 +17,12 @@
transition = _builtins.toplevel.transition

def _cpu_string(platform_type, settings):
arch = _determine_single_architecture(platform_type, settings)
if platform_type == MACOS:
return "darwin_{}".format(arch)

return "{}_{}".format(platform_type, arch)

def _determine_single_architecture(platform_type, settings):
apple_split_cpu = settings["//command_line_option:apple_split_cpu"]
if apple_split_cpu != None and len(apple_split_cpu) > 0:
return apple_split_cpu
if platform_type == IOS:
ios_cpus = settings["//command_line_option:ios_multi_cpus"]
if len(ios_cpus) > 0:
return ios_cpus[0]
cpu_value = settings["//command_line_option:cpu"]
if cpu_value.startswith(IOS_CPU_PREFIX):
return cpu_value[len(IOS_CPU_PREFIX):]
if cpu_value == "darwin_arm64":
return "sim_arm64"
return DEFAULT_IOS_CPU
if platform_type == VISIONOS:
cpus = settings["//command_line_option:visionos_cpus"]
if len(cpus) > 0:
return cpus[0]
cpu_value = settings["//command_line_option:cpu"]
if cpu_value == "darwin_arm64":
return "sim_arm64"
return DEFAULT_VISIONOS_CPU
if platform_type == WATCHOS:
watchos_cpus = settings["//command_line_option:watchos_cpus"]
if len(watchos_cpus) == 0:
return DEFAULT_WATCHOS_CPU
return watchos_cpus[0]
if platform_type == TVOS:
tvos_cpus = settings["//command_line_option:tvos_cpus"]
if len(tvos_cpus) == 0:
return DEFAULT_TVOS_CPU
return tvos_cpus[0]
if platform_type == MACOS:
macos_cpus = settings["//command_line_option:macos_cpus"]
if macos_cpus:
return macos_cpus[0]
cpu_value = settings["//command_line_option:cpu"]
if cpu_value.startswith(DARWIN_CPU_PREFIX):
return cpu_value[len(DARWIN_CPU_PREFIX):]
return DEFAULT_MACOS_CPU
if platform_type == CATALYST:
catalyst_cpus = settings["//command_line_option:catalyst_cpus"]
if len(catalyst_cpus) == 0:
return DEFAULT_CATALYST_CPU
return catalyst_cpus[0]

fail("ERROR: Unhandled platform type {}".format(platform_type))

IOS = "ios"
VISIONOS = "visionos"
WATCHOS = "watchos"
TVOS = "tvos"
MACOS = "macos"
CATALYST = "catalyst"
IOS_CPU_PREFIX = "ios_"
VISIONOS_CPU_PREFIX = "visionos_"
DARWIN_CPU_PREFIX = "darwin_"
DEFAULT_IOS_CPU = "x86_64"
DEFAULT_VISIONOS_CPU = "x86_64"
DEFAULT_WATCHOS_CPU = "i386"
DEFAULT_TVOS_CPU = "x86_64"
DEFAULT_MACOS_CPU = "x86_64"
DEFAULT_CATALYST_CPU = "x86_64"
if apple_split_cpu:
if platform_type == "macos":
return "darwin_{}".format(apple_split_cpu)
return "{}_{}".format(platform_type, apple_split_cpu)
return settings["//command_line_option:cpu"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

erm wouldn't removing all the other platforms here mean you couldn't build an objc library directly for iOS anymore? I feel like this change would likely be about the same as removing the transition all together?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Probably best to look at #19235 (comment) first, just to make sure we're on the same page. Thanks for thinking about it with me!)

To clarify: Not removing all the others; instead unifying a separate condition. (Note that despite how the diff preview decides to align, I've moved this code down from above, not replaced all the platform-specific cases with that macOS if statement. It's all the platform-specific, non-fat-binary-because-no-rules_apple-binary fallback cases that are unified into the last line, looking at CPU. I realize I still need to make the case to you that that's the right, consistent analysis, hence attempting to order the other comment first.)

To build an objc_library directly for iOS (rather than the default, Mac) one would now just specify --cpu (as elsewhere). Previously, (I think?) one couldn't build an objc_library directly for iOS with public APIs, since --apple_platform_type is undocumented (and explicitly labeled as not for direct use), and you'd have to specify it to hit the iOS case when building objc_library directly.


def _output_dictionary(settings, cpu, platform_type, platforms):
return {
Expand Down
19 changes: 7 additions & 12 deletions src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,7 @@ public void testJ2ObjcProtoClassMappingFilesExportedInJavaLibrary() throws Excep
getArtifacts(j2ObjcMappingFileInfo, "class_mapping_files");

assertThat(classMappingFilesList.get(0).getExecPathString())
.containsMatch(
"/applebin_macos-darwin_x86_64-fastbuild-ST-[^/]*/bin/java/com/google/dummy/test/proto/test.clsmap.properties");
.containsMatch("/bin/java/com/google/dummy/test/proto/test.clsmap.properties");
}

@Test
Expand All @@ -317,16 +316,15 @@ public void testJavaProtoLibraryWithProtoLibrary() throws Exception {
ImmutableList<Artifact> classMappingFilesList =
getArtifacts(j2ObjcMappingFileInfo, "class_mapping_files");
assertThat(classMappingFilesList.get(0).getExecPathString())
.containsMatch(
"/applebin_macos-darwin_x86_64-fastbuild-ST-[^/]*/bin/x/test.clsmap.properties");
.containsMatch("/bin/x/test.clsmap.properties");

ObjcProvider objcProvider = target.get(ObjcProvider.STARLARK_CONSTRUCTOR);
CcCompilationContext ccCompilationContext =
target.get(CcInfo.PROVIDER).getCcCompilationContext();
assertThat(ccCompilationContext.getDeclaredIncludeSrcs().toList().toString())
.containsMatch("/applebin_macos-darwin_x86_64-fastbuild-ST-[^/]*/bin]x/test.j2objc.pb.h");
.containsMatch("/bin]x/test.j2objc.pb.h");
assertThat(objcProvider.get(ObjcProvider.SOURCE).toList().toString())
.containsMatch("/applebin_macos-darwin_x86_64-fastbuild-ST-[^/]*/bin]x/test.j2objc.pb.m,");
.containsMatch("/bin]x/test.j2objc.pb.m,");
}

@Test
Expand Down Expand Up @@ -367,19 +365,16 @@ public void testJavaProtoLibraryWithProtoLibrary_external() throws Exception {
getArtifacts(j2ObjcMappingFileInfo, "class_mapping_files");

assertThat(classMappingFilesList.get(0).getExecPathString())
.containsMatch(
"/applebin_macos-darwin_x86_64-fastbuild-ST-[^/]*/bin/external/bla/foo/test.clsmap.properties");
.containsMatch("/bin/external/bla/foo/test.clsmap.properties");

ObjcProvider objcProvider = target.get(ObjcProvider.STARLARK_CONSTRUCTOR);
CcCompilationContext ccCompilationContext =
target.get(CcInfo.PROVIDER).getCcCompilationContext();

assertThat(ccCompilationContext.getDeclaredIncludeSrcs().toList().toString())
.containsMatch(
"/applebin_macos-darwin_x86_64-fastbuild-ST-[^/]*/bin]external/bla/foo/test.j2objc.pb.h");
.containsMatch("/bin]external/bla/foo/test.j2objc.pb.h");
assertThat(objcProvider.get(ObjcProvider.SOURCE).toList().toString())
.containsMatch(
"/applebin_macos-darwin_x86_64-fastbuild-ST-[^/]*/bin]external/bla/foo/test.j2objc.pb.m");
.containsMatch("/bin]external/bla/foo/test.j2objc.pb.m");
assertThat(ccCompilationContext.getIncludeDirs())
.contains(
getConfiguration(target)
Expand Down
16 changes: 8 additions & 8 deletions src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -927,6 +927,7 @@ public void testClangCoptsForDebugModeWithoutGlib() throws Exception {
public void testClangCoptsForDebugModeWithoutHardcoding() throws Exception {
useConfiguration(
"--apple_platform_type=ios",
"--cpu=ios_x86_64",
"--compilation_mode=dbg",
"--incompatible_avoid_hardcoded_objc_compilation_flags");
scratch.file("x/a.m");
Expand All @@ -939,6 +940,7 @@ public void testClangCoptsForDebugModeWithoutHardcoding() throws Exception {
public void testClangCoptsForDebugModeWithoutGlibOrHardcoding() throws Exception {
useConfiguration(
"--apple_platform_type=ios",
"--cpu=ios_x86_64",
"--compilation_mode=dbg",
"--objc_debug_with_GLIBCXX=false",
"--incompatible_avoid_hardcoded_objc_compilation_flags");
Expand All @@ -958,6 +960,7 @@ public void testCompilationActionsForOptimized() throws Exception {
public void testClangCoptsForOptimizedWithoutHardcoding() throws Exception {
useConfiguration(
"--apple_platform_type=ios",
"--cpu=ios_x86_64",
"--compilation_mode=opt",
"--incompatible_avoid_hardcoded_objc_compilation_flags");
scratch.file("x/a.m");
Expand Down Expand Up @@ -993,7 +996,7 @@ public void testAllowVariousNonBlacklistedTypesInHeaders() throws Exception {

@Test
public void testAppleSdkVersionEnv() throws Exception {
useConfiguration("--apple_platform_type=ios");
useConfiguration("--apple_platform_type=ios", "--cpu=ios_x86_64");
createLibraryTargetWriter("//objc:lib")
.setAndCreateFiles("srcs", "a.m", "b.m", "private.h")
.setAndCreateFiles("hdrs", "c.h")
Expand All @@ -1005,7 +1008,7 @@ public void testAppleSdkVersionEnv() throws Exception {

@Test
public void testNonDefaultAppleSdkVersionEnv() throws Exception {
useConfiguration("--apple_platform_type=ios", "--ios_sdk_version=8.1");
useConfiguration("--apple_platform_type=ios", "--cpu=ios_x86_64", "--ios_sdk_version=8.1");

createLibraryTargetWriter("//objc:lib")
.setAndCreateFiles("srcs", "a.m", "b.m", "private.h")
Expand Down Expand Up @@ -1086,7 +1089,7 @@ public void testSdkIncludesUsedInCompileAction() throws Exception {

@Test
public void testCompilationActionsWithPch() throws Exception {
useConfiguration("--apple_platform_type=ios");
useConfiguration("--apple_platform_type=ios", "--cpu=ios_x86_64");
scratch.file("objc/foo.pch");
createLibraryTargetWriter("//objc:lib")
.setAndCreateFiles("srcs", "a.m", "b.m", "private.h")
Expand Down Expand Up @@ -1388,7 +1391,7 @@ public void testUsesDotdPruning() throws Exception {

@Test
public void testAppleSdkDefaultPlatformEnv() throws Exception {
useConfiguration("--apple_platform_type=ios");
useConfiguration("--apple_platform_type=ios", "--cpu=ios_x86_64");
createLibraryTargetWriter("//objc:lib")
.setAndCreateFiles("srcs", "a.m", "b.m", "private.h")
.setAndCreateFiles("hdrs", "c.h")
Expand Down Expand Up @@ -1561,11 +1564,8 @@ public void testSysrootArgSpecifiedWithGrteTopFlag() throws Exception {

@Test
public void testDefaultEnabledFeatureIsUsed() throws Exception {
// Although using --cpu=ios_x86_64, it transitions to darwin_x86_64, so the actual
// cc_toolchain in use will be the darwin_x86_64 one.
MockObjcSupport.setupCcToolchainConfig(
mockToolsConfig, MockObjcSupport.darwinX86_64().withFeatures("default_feature"));
useConfiguration("--cpu=ios_x86_64");
scratch.file("x/BUILD", "objc_library(", " name = 'objc',", " srcs = ['source.m'],", ")");
CommandAction compileAction = compileAction("//x:objc", "source.o");
assertThat(compileAction.getArguments()).contains("-dummy");
Expand Down Expand Up @@ -2071,7 +2071,7 @@ public void testPassesDependenciesStaticLibrariesInCcInfo() throws Exception {
.flatMap(List::stream)
.map(LibraryToLink::getStaticLibrary)
.collect(toImmutableList())))
.contains("/ x/libbaz.a");
.contains("bin x/libbaz.a");
}

@Test
Expand Down
11 changes: 9 additions & 2 deletions src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,19 +961,23 @@ protected void checkClangCoptsForCompilationMode(
switch (codeCoverageMode) {
case NONE:
useConfiguration(
"--apple_platform_type=ios", "--compilation_mode=" + compilationModeFlag(mode));
"--apple_platform_type=ios",
"--cpu=ios_x86_64",
"--compilation_mode=" + compilationModeFlag(mode));
break;
case GCOV:
allExpectedCoptsBuilder.addAll(CompilationSupport.CLANG_GCOV_COVERAGE_FLAGS);
useConfiguration(
"--apple_platform_type=ios",
"--cpu=ios_x86_64",
"--collect_code_coverage",
"--compilation_mode=" + compilationModeFlag(mode));
break;
case LLVMCOV:
allExpectedCoptsBuilder.addAll(CompilationSupport.CLANG_LLVM_COVERAGE_FLAGS);
useConfiguration(
"--apple_platform_type=ios",
"--cpu=ios_x86_64",
"--collect_code_coverage",
"--experimental_use_llvm_covmap",
"--compilation_mode=" + compilationModeFlag(mode));
Expand All @@ -995,7 +999,10 @@ protected void checkClangCoptsForDebugModeWithoutGlib(RuleType ruleType) throws
.addAll(ObjcConfiguration.DBG_COPTS);

useConfiguration(
"--apple_platform_type=ios", "--compilation_mode=dbg", "--objc_debug_with_GLIBCXX=false");
"--apple_platform_type=ios",
"--cpu=ios_x86_64",
"--compilation_mode=dbg",
"--objc_debug_with_GLIBCXX=false");
scratch.file("x/a.m");
ruleType.scratchTarget(scratch, "srcs", "['a.m']");

Expand Down