Skip to content

Commit

Permalink
Migrate XcodeConfigInfo to Starlark.
Browse files Browse the repository at this point in the history 
Unlike the other providers, this one is very public API, used by any rule that needs to access the user's desired minimum OS version or the Xcode version used in the build. While the provider is initialized by passing all of the min OS and SDK version numbers, it only defines _functions_ to read them back out. To preserve this API during the Starlark migration, we have to make the new provider likewise return functions that capture the values that were passed in at the time of construction.

Once it's moved to apple_support, we can explore refactoring this to make it a more "normal" provider, and supply other APIs to interpret the values inside it.

PiperOrigin-RevId: 625718058
Change-Id: Ibe40774203d193e0aa4e06b2c39b2c5767a7d7f6
  • Loading branch information
@allevato @copybara-github
allevato authored and copybara-github committed Apr 17, 2024
1 parent 6b62f77 commit cf5951e
Show file tree
Hide file tree
Showing 11 changed files with 358 additions and 665 deletions.
3 changes: 0 additions & 3 deletions src/main/java/com/google/devtools/build/lib/bazel/rules/ObjcRules.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import com.google.devtools.build.lib.rules.objc.AppleToolchain;
import com.google.devtools.build.lib.rules.objc.J2ObjcConfiguration;
import com.google.devtools.build.lib.rules.objc.ObjcConfiguration;
import com.google.devtools.build.lib.rules.objc.XcodeConfigInfo;
import com.google.devtools.build.lib.starlarkbuildapi.objc.AppleBootstrap;

/** Rules for Objective-C support in Bazel. */
Expand Down Expand Up @@ -55,8 +54,6 @@ public void init(ConfiguredRuleClassProvider.Builder builder) {
builder.addRuleDefinition(new EmptyRule("xcode_config_alias") {});
builder.addRuleDefinition(new EmptyRule("xcode_version") {});

builder.addStarlarkBuiltinsInternal("XcodeConfigInfo", XcodeConfigInfo.PROVIDER);

builder.addStarlarkBuiltinsInternal("apple_common", new AppleStarlarkCommon());
builder.addStarlarkBootstrap(new AppleBootstrap());
}
Expand Down
5 changes: 4 additions & 1 deletion src/main/java/com/google/devtools/build/lib/rules/objc/AppleToolchain.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ public class AppleToolchain implements AppleToolchainApi<AppleConfiguration> {
@VisibleForTesting
public static final String SYSTEM_FRAMEWORK_PATH = "/System/Library/Frameworks";

/** XcodeConfig attribute name for Apple rules that take an xcode_config parameter */
public static final String XCODE_CONFIG_ATTR_NAME = ":xcode_config";

/** Returns the platform directory inside of Xcode for a platform name. */
public static String platformDir(String platformName) {
return developerDir() + "/Platforms/" + platformName + ".platform";
Expand Down Expand Up @@ -130,7 +133,7 @@ public RequiresXcodeConfigRule(RepositoryName toolsRepository) {
public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
return builder
.add(
attr(XcodeConfigInfo.XCODE_CONFIG_ATTR_NAME, LABEL)
attr(XCODE_CONFIG_ATTR_NAME, LABEL)
.allowedRuleClasses("xcode_config")
.checkConstraints()
.value(getXcodeConfigLabel(toolsRepository)))
Expand Down
21 changes: 0 additions & 21 deletions src/main/java/com/google/devtools/build/lib/rules/objc/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@ java_library(
srcs = glob(["*.java"]),
deps = [
"//src/main/java/com/google/devtools/build/docgen/annot",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
Expand All @@ -29,7 +27,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
"//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
"//src/main/java/com/google/devtools/build/lib/analysis/platform",
"//src/main/java/com/google/devtools/build/lib/analysis/starlark/annotations",
Expand All @@ -38,7 +35,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/rules:alias",
"//src/main/java/com/google/devtools/build/lib/rules/apple",
"//src/main/java/com/google/devtools/build/lib/rules/cpp",
"//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
Expand All @@ -58,20 +54,3 @@ java_library(
"//third_party:jsr305",
],
)

java_library(
name = "xcode_config_info",
srcs = ["XcodeConfigInfo.java"],
deps = [
"//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/rules/apple",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple",
"//src/main/java/net/starlark/java/annot",
"//src/main/java/net/starlark/java/eval",
"//third_party:guava",
"//third_party:jsr305",
],
)
Loading

4 comments on commit cf5951e

@brentleyjones
Copy link
Contributor

Choose a reason for hiding this comment

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

@allevato Before this commit the apple_common.XcodeVersionConfig constructor looked like this:

apple_common.XcodeVersionConfig(
    iosSdkVersion = ...,
)

and now it's this:

apple_common.XcodeVersionConfig(
    ios_sdk_version = ...,
)

Just letting you know this is a breaking change: https://buildkite.com/bazel/apple-support-darwin/builds/2520#018f87f2-6ddb-46c5-93f4-1a4552db2f47/259-296

@allevato
Copy link
Member Author

Choose a reason for hiding this comment

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

That change is intentional; the old camelCaseNames didn't meet Starlark naming conventions and no pre-existing external Starlark code was expected to use the old incorrect API. The apple_support PRs that migrate the Xcode rules to external Starlark are only meant to be used with Bazel versions that follow this commit, though I suppose you could use a version check of some sort to figure out which form of the API you need if you want to use it with something earlier.

@brentleyjones
Copy link
Contributor

@brentleyjones brentleyjones commented on cf5951e May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the old camelCaseNames didn't meet Starlark naming conventions and no pre-existing external Starlark code was expected to use the old incorrect API

But it was public API, right? That was my main call out, in case people were using the API.

@allevato
Copy link
Member Author

@allevato allevato commented on cf5951e May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Technically yes, it's a breaking change—but there will be a lot of breaking changes as the remaining Apple/Xcode logic is eventually removed from Bazel core. The only reason this API was kept at all instead of being migrated completely to apple_support is because of the dependency in the ObjcBinarySymbolStrip action; if it weren't for that, it would have been deleted entirely, which also would have been a breaking change.

In practice, I don't expect it to break anything because the only reason to use the API would be to write one's own custom xcode_config rule. I don't have the full history behind why the API was exposed to Starlark in the first place, because (at least internally) it doesn't appear to have ever been used in Starlark after that.

Please sign in to comment.