Change xcode_config rule semantics to fit current usage

- require_defined_versions is deprecated and a no-op. A version must match existing defined versions if any exist
- default label must be present in versions labels if any are defined
- default label may not exist if no versions are defined
- when --xcode_version is specified on the command line, it must match a defined version if any are defined. If none are defined, this flag is a no-op

RELNOTES: None.
PiperOrigin-RevId: 167616628

7 files changed, 60 insertions, 62 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfig.java b/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfig.java
index fed295e6fc..f433d20f84 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfig.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfig.java
@@ -18,6 +18,7 @@ import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.RedirectChaser;
@@ -36,7 +37,6 @@ import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
-import com.google.devtools.build.lib.syntax.Type;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -82,12 +82,8 @@ public class XcodeConfig implements RuleConfiguredTargetFactory {
     ImmutableList<XcodeVersionRuleData> versions = getAvailableVersions(env, xcodeConfigRule);
     XcodeVersionRuleData defaultVersion = getDefaultVersion(env, xcodeConfigRule);
 
-    boolean requireDefinedVersions = NonconfigurableAttributeMapper.of(xcodeConfigRule)
-        .get(XcodeConfigRule.REQUIRE_DEFINED_VERSIONS_ATTR_NAME, Type.BOOLEAN);
-
     try {
-      return resolveXcodeVersion(
-          requireDefinedVersions, appleOptions.xcodeVersion, versions, defaultVersion);
+      return resolveXcodeVersion(appleOptions.xcodeVersion, versions, defaultVersion);
     } catch (XcodeConfigException e) {
       throw new InvalidConfigurationException(e.getMessage());
     }
@@ -113,12 +109,9 @@ public class XcodeConfig implements RuleConfiguredTargetFactory {
     Iterable<XcodeVersionRuleData> availableVersions = ruleContext.getPrerequisites(
         XcodeConfigRule.VERSIONS_ATTR_NAME, RuleConfiguredTarget.Mode.TARGET,
         XcodeVersionRuleData.class);
-    boolean requireDefinedVersions = ruleContext.attributes().get(
-        XcodeConfigRule.REQUIRE_DEFINED_VERSIONS_ATTR_NAME, Type.BOOLEAN);
     XcodeVersionProperties xcodeVersionProperties;
     try {
       xcodeVersionProperties = resolveXcodeVersion(
-          requireDefinedVersions,
           appleOptions.xcodeVersion,
           availableVersions,
           defaultVersion);
@@ -166,46 +159,50 @@ public class XcodeConfig implements RuleConfiguredTargetFactory {
    * @param requireDefinedVersions whether the version config requires an explicitly defined version
    * @param xcodeVersionOverrideFlag the value of the {@code --xcode_version} command line flag
    * @param xcodeVersions the Xcode versions listed in the {@code xcode_config} rule
-   * @param defaultVersion the default Xcode version in the {@code xcode_config} rule. Can be null.
+   * @param defaultVersion the default Xcode version in the {@code xcode_config} rule.
    * @throws XcodeConfigException if the options given (or configuration targets) were
    *     malformed and thus the xcode version could not be determined
    */
   static XcodeVersionProperties resolveXcodeVersion(
-      boolean requireDefinedVersions,
       String xcodeVersionOverrideFlag,
       Iterable<XcodeVersionRuleData> xcodeVersions,
-      @Nullable XcodeVersionRuleData defaultVersion)
+      XcodeVersionRuleData defaultVersion)
       throws XcodeConfigException {
-    XcodeVersionRuleData xcodeVersion = resolveExplicitlyDefinedVersion(
-        requireDefinedVersions, xcodeVersions, defaultVersion, xcodeVersionOverrideFlag);
-
-    if (xcodeVersion != null) {
-      return xcodeVersion.getXcodeVersionProperties();
+    if (defaultVersion != null
+        && Iterables.isEmpty(Iterables.filter(
+              xcodeVersions,
+              ruleData -> ruleData.getLabel().equals(defaultVersion.getLabel())))) {
+      throw new XcodeConfigException(
+          String.format("default label '%s' must be contained in versions attribute",
+              defaultVersion.getLabel()));
     }
-    // TODO(b/64576392): Remove this fallback logic. An xcode_version target should be explicitly
-    // matched in all cases where --xcode_version is specified.
-    try {
-      DottedVersion dottedVersion = DottedVersion.fromString(xcodeVersionOverrideFlag);
-      return new XcodeVersionProperties(dottedVersion);
-    } catch (IllegalArgumentException e) {
-      // The --xcode_version flag is not a valid DottedVersion, so there is nothing to go on.
+    if (Iterables.isEmpty(xcodeVersions)) {
+      if (defaultVersion != null) {
+        throw new XcodeConfigException(
+            "default label must be contained in versions attribute");
+      }
+      return XcodeVersionProperties.unknownXcodeVersionProperties();
+    }
+    if (defaultVersion == null) {
+      throw new XcodeConfigException(
+          "if any versions are specified, a default version must be specified");
     }
-    return XcodeVersionProperties.unknownXcodeVersionProperties();
+
+    XcodeVersionRuleData xcodeVersion = resolveExplicitlyDefinedVersion(
+        xcodeVersions, defaultVersion, xcodeVersionOverrideFlag);
+
+    return xcodeVersion.getXcodeVersionProperties();
   }
 
   /**
    * Returns the {@link XcodeVersionRuleData} associated with the {@code xcode_version} target
    * explicitly defined in the {@code --xcode_version_config} build flag and selected by the {@code
    * --xcode_version} flag. If {@code --xcode_version} is unspecified, then this will return the
-   * default rule data as specified in the {@code --xcode_version_config} target. Returns null if
-   * either the {@code --xcode_version} did not match any {@code xcode_version} target, or if {@code
-   * --xcode_version} is unspecified and {@code --xcode_version_config} specified no default target.
+   * default rule data as specified in the {@code --xcode_version_config} target.
    */
-  @Nullable
   private static XcodeVersionRuleData resolveExplicitlyDefinedVersion(
-      boolean requireDefinedVersions,
       Iterable<XcodeVersionRuleData> xcodeVersionRules,
-      @Nullable XcodeVersionRuleData defaultVersion,
+      XcodeVersionRuleData defaultVersion,
       String versionOverrideFlag)
       throws XcodeConfigException {
 
@@ -218,18 +215,13 @@ public class XcodeConfig implements RuleConfiguredTargetFactory {
           aliasesToVersionMap.get(versionOverrideFlag);
       if (explicitVersion != null) {
         return explicitVersion;
+      } else {
+        throw new XcodeConfigException(
+            String.format("%s matches no alias in the config", versionOverrideFlag));
       }
-    } else if (defaultVersion != null) {
-      // No override specified. Use default.
-      return defaultVersion;
-    }
-
-    if (requireDefinedVersions) {
-      throw new XcodeConfigException(
-          "xcode version config required an explicitly defined version, but none was available");
     }
 
-    return null;
+    return defaultVersion;
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfigRule.java b/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfigRule.java
index 1b5a4adf40..b22e1aa6a6 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfigRule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfigRule.java
@@ -44,7 +44,8 @@ public class XcodeConfigRule implements RuleDefinition {
         /* <!-- #BLAZE_RULE(xcode_config).ATTRIBUTE(version) -->
         The default official version of xcode to use.
         The version specified by the provided <code>xcode_version</code> target is to be used if
-        no <code>xcode_version</code> build flag is specified.
+        no <code>xcode_version</code> build flag is specified. This is required if any
+        <code>versions</code> are set.
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(attr(DEFAULT_ATTR_NAME, LABEL)
             .allowedRuleClasses("xcode_version")
@@ -61,11 +62,9 @@ public class XcodeConfigRule implements RuleDefinition {
             .allowedFileTypes()
             .nonconfigurable("this rule determines configuration"))
         /* <!-- #BLAZE_RULE(xcode_config).ATTRIBUTE(version) -->
-        Whether to require the build's xcode version match one of the declared targets.
-        If true, this will raise an error if either the <code>xcode_version</code> flag value
-        or <code>default</code> attribute value do not match one of the versions declared
-        among <code>xcode_verison</code> targets.
+        Deprecated. This attribute has no effect.
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+        // TODO(b/64576392): Remove this attribute.
         .add(attr(REQUIRE_DEFINED_VERSIONS_ATTR_NAME, BOOLEAN)
             .value(false)
             .nonconfigurable("this rule determines configuration"))
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/MockObjcSupport.java b/src/test/java/com/google/devtools/build/lib/packages/util/MockObjcSupport.java
index 73f9e9b491..d42f6f720a 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/MockObjcSupport.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/MockObjcSupport.java
@@ -97,12 +97,26 @@ public final class MockObjcSupport {
         String.format(
             "  srcs = ['%s//objcproto:well_known_type.proto'],", TestConstants.TOOLS_REPOSITORY),
         ")",
-        "xcode_config(name = 'host_xcodes', default = ':version7_3_1')",
+        "xcode_config(name = 'host_xcodes',",
+        "  default = ':version7_3_1',",
+        "  versions = [':version7_3_1', 'version5_0', 'version7_3', 'version5_8'])",
         "xcode_version(",
         "  name = 'version7_3_1',",
         "  version = '" + DEFAULT_XCODE_VERSION + "',",
         "  default_ios_sdk_version = \"" + DEFAULT_IOS_SDK_VERSION + "\",",
         ")",
+        "xcode_version(",
+        "  name = 'version7_3',",
+        "  version = '7.3',",
+        ")",
+        "xcode_version(",
+        "  name = 'version5_0',",
+        "  version = '5.0',",
+        ")",
+        "xcode_version(",
+        "  name = 'version5_8',",
+        "  version = '5.8',",
+        ")",
         "objc_library(name = 'dummy_lib', srcs = ['objc_dummy.mm'])");
     // If the bazel tools repository is not in the workspace, also create a workspace tools/objc
     // package with a few lingering dependencies.
diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/IosDeviceTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/IosDeviceTest.java
index 273f51b497..a0a9f20c72 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/objc/IosDeviceTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/objc/IosDeviceTest.java
@@ -68,7 +68,7 @@ public class IosDeviceTest extends BuildViewTestCase {
     scratch.file("test/BUILD",
         "xcode_version(name = 'my_xcode', version = '15.2')",
         "ios_device(name = 'foo', type = 'IPHONE_6', xcode = ':my_xcode')");
-    useConfiguration("--xcode_version=2.1", "--ios_sdk_version=42.3");
+    useConfiguration("--xcode_version=7.3", "--ios_sdk_version=42.3");
 
     assertXcodeVersion("//test:foo", "15.2");
     assertIosVersion("//test:foo", XcodeVersionProperties.DEFAULT_IOS_SDK_VERSION);
@@ -79,7 +79,7 @@ public class IosDeviceTest extends BuildViewTestCase {
     scratch.file("test/BUILD",
         "xcode_version(name = 'my_xcode', version = '15.2', default_ios_sdk_version='17.8')",
         "ios_device(name = 'foo', type = 'IPHONE_6', xcode = ':my_xcode')");
-    useConfiguration("--xcode_version=2.1", "--ios_sdk_version=42.3");
+    useConfiguration("--xcode_version=7.3", "--ios_sdk_version=42.3");
 
     assertXcodeVersion("//test:foo", "15.2");
     assertIosVersion("//test:foo", "17.8");
@@ -90,7 +90,7 @@ public class IosDeviceTest extends BuildViewTestCase {
     scratch.file("test/BUILD",
         "xcode_version(name = 'my_xcode', version = '15.2', default_ios_sdk_version='17.8')",
         "ios_device(name = 'foo', type = 'IPHONE_6', ios_version='98.7', xcode = ':my_xcode')");
-    useConfiguration("--xcode_version=2.1", "--ios_sdk_version=42.3");
+    useConfiguration("--xcode_version=7.3", "--ios_sdk_version=42.3");
 
     assertXcodeVersion("//test:foo", "15.2");
     assertIosVersion("//test:foo", "98.7");
diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcBinaryTest.java
index 4775e2df30..4a20b8e777 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcBinaryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcBinaryTest.java
@@ -115,8 +115,7 @@ public class ObjcBinaryTest extends ObjcRuleTestCase {
    * Tests that bitcode is disabled for simulator builds even if enabled by flag.
    */
   public void testLinkActionsWithBitcode_simulator() throws Exception {
-    useConfiguration("--xcode_version=7.1", "--apple_bitcode=embedded",
-        "--ios_multi_cpus=x86_64");
+    useConfiguration("--apple_bitcode=embedded", "--ios_multi_cpus=x86_64");
     createBinaryTargetWriter("//objc:bin").setAndCreateFiles("srcs", "a.m").write();
 
     CommandAction linkAction = linkAction("//objc:bin");
@@ -128,8 +127,7 @@ public class ObjcBinaryTest extends ObjcRuleTestCase {
 
   @Test
   public void testLinkActionsWithNoBitcode() throws Exception {
-    useConfiguration("--xcode_version=7.1", "--apple_bitcode=none",
-        "--ios_multi_cpus=arm64");
+    useConfiguration("--apple_bitcode=none", "--ios_multi_cpus=arm64");
     createBinaryTargetWriter("//objc:bin").setAndCreateFiles("srcs", "a.m").write();
 
     CommandAction linkAction = linkAction("//objc:bin");
@@ -810,7 +808,7 @@ public class ObjcBinaryTest extends ObjcRuleTestCase {
 
   @Test
   public void testLinkActionsWithEmbeddedBitcode() throws Exception {
-    useConfiguration("--xcode_version=7.1", "--apple_bitcode=embedded", "--ios_multi_cpus=arm64");
+    useConfiguration("--apple_bitcode=embedded", "--ios_multi_cpus=arm64");
     createBinaryTargetWriter("//objc:bin").setAndCreateFiles("srcs", "a.m").write();
 
     CommandAction linkAction = linkAction("//objc:bin");
@@ -823,8 +821,7 @@ public class ObjcBinaryTest extends ObjcRuleTestCase {
 
   @Test
   public void testLinkActionsWithEmbeddedBitcodeMarkers() throws Exception {
-    useConfiguration(
-        "--xcode_version=7.1", "--apple_bitcode=embedded_markers", "--ios_multi_cpus=arm64");
+    useConfiguration("--apple_bitcode=embedded_markers", "--ios_multi_cpus=arm64");
     createBinaryTargetWriter("//objc:bin").setAndCreateFiles("srcs", "a.m").write();
 
     CommandAction linkAction = linkAction("//objc:bin");
diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java
index 08b1d0b191..26033f41a7 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java
@@ -600,7 +600,6 @@ public class ObjcLibraryTest extends ObjcRuleTestCase {
   @Test
   public void testCompilationActionsWithEmbeddedBitcode() throws Exception {
     useConfiguration(
-        "--xcode_version=7.1",
         "--ios_multi_cpus=arm64",
         "--apple_bitcode=embedded");
     createLibraryTargetWriter("//objc:lib")
@@ -616,7 +615,6 @@ public class ObjcLibraryTest extends ObjcRuleTestCase {
   @Test
   public void testCompilationActionsWithEmbeddedBitcodeMarkers() throws Exception {
     useConfiguration(
-        "--xcode_version=7.1",
         "--ios_multi_cpus=arm64",
         "--apple_bitcode=embedded_markers");
 
@@ -634,7 +632,6 @@ public class ObjcLibraryTest extends ObjcRuleTestCase {
   public void testCompilationActionsWithNoBitcode() throws Exception {
     useConfiguration(
         "--crosstool_top=" + MockObjcSupport.DEFAULT_OSX_CROSSTOOL,
-        "--xcode_version=7.1",
         "--ios_multi_cpus=arm64",
         "--apple_bitcode=none");
 
@@ -656,7 +653,6 @@ public class ObjcLibraryTest extends ObjcRuleTestCase {
   public void testCompilationActionsWithBitcode_simulator() throws Exception {
     useConfiguration(
         "--crosstool_top=" + MockObjcSupport.DEFAULT_OSX_CROSSTOOL,
-        "--xcode_version=7.1",
         "--ios_multi_cpus=x86_64",
         "--apple_bitcode=embedded");
 
diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcSkylarkTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcSkylarkTest.java
index 11d36332da..a5453e5b51 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcSkylarkTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcSkylarkTest.java
@@ -290,7 +290,7 @@ public class ObjcSkylarkTest extends ObjcRuleTestCase {
         "   name='my_target',",
         ")");
 
-    useConfiguration("--cpu=ios_i386", "--xcode_version=7.1");
+    useConfiguration("--cpu=ios_i386", "--xcode_version=7.3");
     ConfiguredTarget skylarkTarget = getConfiguredTarget("//examples/apple_skylark:my_target");
 
 
@@ -304,7 +304,7 @@ public class ObjcSkylarkTest extends ObjcRuleTestCase {
     assertThat(env).containsEntry("APPLE_SDK_PLATFORM", "iPhoneSimulator");
     assertThat(env).containsEntry("APPLE_SDK_VERSION_OVERRIDE", "8.4");
     assertThat(sdkVersion).isEqualTo("8.4");
-    assertThat(skylarkTarget.get("xcode_version")).isEqualTo("7.1");
+    assertThat(skylarkTarget.get("xcode_version")).isEqualTo("7.3");
     assertThat(skylarkTarget.get("single_arch_platform")).isEqualTo("IOS_SIMULATOR");
     assertThat(skylarkTarget.get("single_arch_cpu")).isEqualTo("i386");
     assertThat(skylarkTarget.get("platform_type")).isEqualTo("ios");