Support Xcode betas with xcode-locator #7371

keith · 2019-02-07T05:59:50Z

Previously if you had 2 builds of the same version of Xcode, for example
10.2.0 beta 1 and 10.2.0 beta 2, there was no way to disambiguate them.
This change appends the ProductBuildVersion of each Xcode version to
allow you to specify that if desired.

keith · 2019-02-07T06:01:27Z

Here is the example output in the previous case:

% bazel-bin/tools/osx/xcode-locator 2>/dev/null
{
        "10": "/Applications/Xcode-10.1.0.app/Contents/Developer",
        "10.2.0": "/Applications/Xcode-10.2.0b1.app/Contents/Developer",
        "10.1.0": "/Applications/Xcode-10.1.0.app/Contents/Developer",
        "10.1": "/Applications/Xcode-10.1.0.app/Contents/Developer",
        "10.2": "/Applications/Xcode-10.2.0b2.app/Contents/Developer",
}
% bazel-bin/tools/osx/xcode-locator -v 2>/dev/null
10.2.0:10.2,10.2.0:/Applications/Xcode-10.2.0b1.app/Contents/Developer
10.1.0:10.1.0,10.1,10:/Applications/Xcode-10.1.0.app/Contents/Developer
% bazel-bin/tools/osx/xcode-locator 10.2.0 2>/dev/null
/Applications/Xcode-10.2.0b1.app/Contents/Developer

As you can see there isn't a single version you can pass to make sure you get beta 1 or beta 2. I also assume this differs based on what the OS returns from LSCopyApplicationURLsForBundleIdentifier.

Here's the new output in this case:

% bazel-bin/tools/osx/xcode-locator 2>/dev/null
{
        "10": "/Applications/Xcode-10.1.0.app/Contents/Developer",
        "10.1": "/Applications/Xcode-10.1.0.app/Contents/Developer",
        "10.1.0": "/Applications/Xcode-10.1.0.app/Contents/Developer",
        "10.2.0.14490.95": "/Applications/Xcode-10.2.0b2.app/Contents/Developer",
        "10.2.0.14490": "/Applications/Xcode-10.2.0b2.app/Contents/Developer",
        "10.2.0": "/Applications/Xcode-10.2.0b2.app/Contents/Developer",
        "10.1.0.14460": "/Applications/Xcode-10.1.0.app/Contents/Developer",
        "10.1.0.14460.46": "/Applications/Xcode-10.1.0.app/Contents/Developer",
        "10.2.0.14490.87.2": "/Applications/Xcode-10.2.0b1.app/Contents/Developer",
        "10.2.0.14490.87": "/Applications/Xcode-10.2.0b1.app/Contents/Developer",
        "10.2.0.14490.95.3": "/Applications/Xcode-10.2.0b2.app/Contents/Developer",
        "10.2": "/Applications/Xcode-10.2.0b2.app/Contents/Developer",
}
% bazel-bin/tools/osx/xcode-locator -v 2>/dev/null
10.2.0:10.2.0.14490.87,10.2.0,10.2.0.14490.95.3,10.2,10.2.0.14490.87.2,10.2.0.14490.95,10.2.0.14490:/Applications/Xcode-10.2.0b2.app/Contents/Developer
10.1.0:10.1.0,10,10.1,10.1.0.14460,10.1.0.14460.46:/Applications/Xcode-10.1.0.app/Contents/Developer
% bazel-bin/tools/osx/xcode-locator 10.2.0 2>/dev/null
/Applications/Xcode-10.2.0b2.app/Contents/Developer
% bazel-bin/tools/osx/xcode-locator 10.2.0.14490.95.3 2>/dev/null
/Applications/Xcode-10.2.0b2.app/Contents/Developer
% bazel-bin/tools/osx/xcode-locator 10.2.0.14490.87 2>/dev/null
/Applications/Xcode-10.2.0b1.app/Contents/Developer

There are some downsides to this representation.

It's still ambiguous when using xcode-locator -v. This shouldn't be a big problem since this feature won't be used by everyone, and you can see the longer versions if you don't pass -v. The reason I didn't change the "real" version to include this new portion is because this error message includes these versions, which would likely surprise users

bazel/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfig.java

Lines 184 to 189 in e2e2353

    
           throw new XcodeConfigException( 
        
               String.format( 
        
                   "--xcode_version=%1$s specified, but '%1$s' is not an available Xcode version. " 
        
                       + "available versions: [%2$s]. If you believe you have '%1$s' installed, try " 
        
                       + "running \"bazel shutdown\", and then re-run your command.", 
        
                   versionOverrideFlag, printableXcodeVersions(xcodeVersionRules)));

This version isn't user facing. I would have preferred to use the build number that Xcode shows in the "Welcome to Xcode" UI but there are some problems with that. First off that version isn't in the Info.plist, it's in Contents/version.plist, meaning we have to parse a second file to get it. The bigger problem is that DottedVersion doesn't like having letters in sections and crashes on certain patterns of that. I figured this option was better than working around those downsides because it will be a lesser used feature anyways.

sergiocampama · 2019-02-15T02:01:12Z

DottedVersion requires each component to follow the \d+[a-Z]*\d* regexp. What if when the component starts with a letter, you append a 0 in the beginning? I think that should be semver equivalent.

I like this change, it improves xcode locator's functionality quite a lot.

looping in @aragos and @c-parsons for input from the bazel POV.

aragos · 2019-02-15T14:42:38Z

Right, DottedVersion actually follows Apple's naming scheme - if you look at the directory names you have your Xcodes in they work with DottedVersion.

In general I'd be ok with this change but shorter/more descriptive version names would be nice. I don't think parsing a second file would be a problem (this is cached, right?) but @c-parsons may disagree. :)

keith · 2019-02-16T02:09:34Z

Do we know if it's possible for this version to ever start with a letter? I've been maintaining this since Xcode 8.3 and every version since then including betas start with a number https://github.com/keith/Xcode.app-strings/commits/master but the problem is actually the case where they end with a letter.

Here's the exception that I hit when I tried that originally:

Caused by: java.lang.IllegalArgumentException: Dotted version components must all be of the form \d+([a-z]+\d*)? but got 10.2.0.10p91b

(note I lowercased it locally because the regex is case sensitive)

It looks DottedVersion is very tied to there only being a single alpha group in each component, otherwise it looks like it would break sorting

aragos · 2019-02-19T14:32:54Z

I'd be fine with modifying DottedVersion to be more permissive if you feel like that's required to represent Apple's naming scheme. Both upper casing and more letters should be ok.

keith · 2019-02-19T23:41:47Z

I'm not sure that changing DottedVersion is viable. The implementation of it is very tied to its internal Components type that is very specific about the alpha requirements. I'm not sure we could reasonable support the current behavior and this new format.

keith · 2019-02-20T16:44:45Z

I talked to Sergio about this more offline and am taking a stab at updating DottedVersion

Previously if you had 2 builds of the same version of Xcode, for example 10.2.0 beta 1 and 10.2.0 beta 2, there was no way to disambiguate them. This change appends the `ProductBuildVersion` of each Xcode version to allow you to specify that if desired.

keith · 2019-02-20T16:57:15Z

tools/osx/xcode_locator.m

-        if ([versionArg rangeOfCharacterFromSet:versSet.invertedSet].length
-            != 0) {
-          versionArg = nil;
-        }


In theory we could add all uppercase and lowercase letters here, but I don't think this validation added a lot of value anyways, so it's probably fine to remove it entirely

keith · 2019-02-20T16:59:46Z

@aragos I updated DottedVersion to support this use case so this is ready for another pass!

aragos

Thanks for the changes! Looks good to me but it would be great to also get an approval from @sergiocampama.

sergiocampama

Looks good to me.

keith · 2019-02-20T21:42:29Z

Awesome thanks!

*** Reason for rollback *** Partial rollback because we need to migrate some folks before we can get the new functionality live. *** Original change description *** Support Xcode betas with xcode-locator Previously if you had 2 builds of the same version of Xcode, for example 10.2.0 beta 1 and 10.2.0 beta 2, there was no way to disambiguate them. This change appends the `ProductBuildVersion` of each Xcode version to allow you to specify that if desired. Closes #7371. PiperOrigin-RevId: 234900481

…ing on the old version. *** Original change description *** Support Xcode betas with xcode-locator Previously if you had 2 builds of the same version of Xcode, for example 10.2.0 beta 1 and 10.2.0 beta 2, there was no way to disambiguate them. This change appends the `ProductBuildVersion` of each Xcode version to allow you to specify that if desired. Closes #7371. PiperOrigin-RevId: 235823433

DavidGoldman · 2019-03-06T22:22:43Z

It's worth noting that even with this change Bazel may not select the proper Xcode beta if you don't specify the --xcode_version flag via the command line and instead rely on the active xcode-select'ed Xcode version: due to the version scanning here which does not check the build version. It's probably best way that code to use xcode-select -p directly.

keith · 2019-03-07T02:09:13Z

+1 that's a good callout, my expectation was generally just that if you're not setting that you're not super worried about what Xcode version people are using in general, so it would probably be ok

Homebrew's CI is breaking on 10.12 (Sierra), for Bazel 0.24+ due to the signature used for NSDictionary:initWithContentsOfURL introduced in the fix for #7371. This signature with an error parameter is only available on 10.13+ (High Sierra). Here's Apple's documentation: https://developer.apple.com/documentation/foundation/nsdictionary/1416069-initwithcontentsofurl?language=objc The Homebrew issue: Homebrew/homebrew-core#38651 Closes #8068. PiperOrigin-RevId: 244357959

googlebot added the cla: yes label Feb 7, 2019

keith force-pushed the ks/xcode-betas branch from 7a1061e to 3d61f58 Compare February 7, 2019 06:49

jin added the z-team-Apple Deprecated. Send to rules_apple, or label team-Rules-CPP + platform:apple label Feb 19, 2019

jin assigned aragos Feb 19, 2019

keith force-pushed the ks/xcode-betas branch from 3d61f58 to 4bb3618 Compare February 20, 2019 16:52

keith requested a review from lberki as a code owner February 20, 2019 16:52

keith added 2 commits February 20, 2019 08:55

Update DottedVersion to support multiple letters

e944070

keith force-pushed the ks/xcode-betas branch from 4bb3618 to e944070 Compare February 20, 2019 16:56

keith commented Feb 20, 2019

View reviewed changes

aragos approved these changes Feb 20, 2019

View reviewed changes

aragos requested review from sergiocampama and removed request for lberki February 20, 2019 18:04

sergiocampama approved these changes Feb 20, 2019

View reviewed changes

bazel-io closed this in ab72246 Feb 20, 2019

keith deleted the ks/xcode-betas branch February 20, 2019 21:42

sayrer mentioned this pull request Apr 16, 2019

Conditionally use deprecated signature for initWithContentsOfURL #8068

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Xcode betas with xcode-locator #7371

Support Xcode betas with xcode-locator #7371

keith commented Feb 7, 2019 •

edited

Loading

keith commented Feb 7, 2019 •

edited

Loading

sergiocampama commented Feb 15, 2019

aragos commented Feb 15, 2019

keith commented Feb 16, 2019

aragos commented Feb 19, 2019

keith commented Feb 19, 2019

keith commented Feb 20, 2019

keith Feb 20, 2019

keith commented Feb 20, 2019

aragos left a comment

sergiocampama left a comment

keith commented Feb 20, 2019

DavidGoldman commented Mar 6, 2019

keith commented Mar 7, 2019

Support Xcode betas with xcode-locator #7371

Support Xcode betas with xcode-locator #7371

Conversation

keith commented Feb 7, 2019 • edited Loading

keith commented Feb 7, 2019 • edited Loading

sergiocampama commented Feb 15, 2019

aragos commented Feb 15, 2019

keith commented Feb 16, 2019

aragos commented Feb 19, 2019

keith commented Feb 19, 2019

keith commented Feb 20, 2019

keith Feb 20, 2019

Choose a reason for hiding this comment

keith commented Feb 20, 2019

aragos left a comment

Choose a reason for hiding this comment

sergiocampama left a comment

Choose a reason for hiding this comment

keith commented Feb 20, 2019

DavidGoldman commented Mar 6, 2019

keith commented Mar 7, 2019

keith commented Feb 7, 2019 •

edited

Loading

keith commented Feb 7, 2019 •

edited

Loading