798528 - python/mozboot/bin/bootstrap.py doesn't detect installed xcode version

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With Xcode 3.5.1 installed on 10.7 the bootstrap script doesn't detect it and always wants to install it via the App Store.

$ python python/mozboot/bin/bootstrap.py 

Xcode is required to build Firefox. Please complete the install of Xcode
through the App Store.

Once the install has finished, please relaunch this script.

Comment 1

[Gregory Szorc [:gps]]

I'm pretty sure mozboot is doing the right thing! We now require Xcode 4 to build (I know we no longer support Clang on older Xcode but we may have broken other Xcode 3.x pieces too).

Currently, if an older Xcode is detected on 10.7+ (which could happen if you upgrade your OS), we don't display the "your Xcode needs to be upgraded message." So, I think this is just a messaging bug.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks like this is the cause if Xcode 4.2 is installed. I have updated to 4.5.1 and now it also gets installed under /Applications/Xcode.app. Shouldn't we check both locations for version>=7?

Comment 3

[Gregory Szorc [:gps]]

Xcode < N (I think 4) is always installed to /Developer/Applications/Xcode.app. Xcode >= N is always installed to /Applications/Xcode.app. Since we require Xcode > N to build the tree, it should only be sufficient to check for /Applications/Xcode.app.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Print an upgrade message when old Xcode is present, v1

Pretty trivial patch.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Well, as pointed out I had 4.2 and it was installed in /Developers. So the assumption is probably not right. Could it be that if you install all the developer tools also xcode will end-up in /Developers/Applications/Xcode.app? Because that's what I did initially.

IMO we should really check for the version of xcode and not make guesses on the location. 

Something I found at the following website I really like:
http://www.garron.me/bits/what-xcode-version-am-i-running.html

$ xcodebuild -version
Xcode 4.5.1
Build version 4G1004

Comment 6

[Gregory Szorc [:gps]]

So, I think Xcode 4.3 is when the transition from /Developers to /Applications was made. AFAIK there is no way to install Xcode 4.3+ into /Developers (unless you have a very funky setup).

On OS X 10.7+, we insist you have Xcode 4.3+ (preferably the latest released version) because we require a Clang shipping in that version or newer.

On OS X < 10.7, Xcode 4.2 is the latest version available to you and its Clang cannot compile m-c. So, on these machines we install Clang via Homebrew.

On Xcode 4.3+, we don't perform the supplemental Clang install and rely on Xcode's default.

In the future, we will likely have the bootstrapper pull down the same Clang build that we use on the official build machines. Until then, we are left with silly Xcode version checks (which we don't really do right now, which I agree is not optimal). Patches are welcome.

As far as the location of Xcode, I've never seen Xcode 4.3+ installed anywhere but /Applications. Where is your version of Xcode 4.5.1 installed?

Comment 7

[Nomis101]

There is also the user choice. I want to have all my developer stuff in one place, so I still put every Xcode version into /Developer. You also can download every Xcode version as dmg from the Apple Developer side instead of using the AppStore.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I fully agree to Nomix101 statement. It's an users choice where to place the app bundle. /Applications/ is not the must go. That said, xcodebuild is probably the best solution here. You can easily strip out the version information.

Comment 9

[Gregory Szorc [:gps]]

I'm not advocating against user choice: I didn't realize it was possible to install newer Xcode anywhere but /Applications.

Anyway, it's obvious the Xcode detection logic needs updated to account for non-default install locations.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 668630 [details] [diff] [review]
Print an upgrade message when old Xcode is present, v1

Review of attachment 668630 [details] [diff] [review]:
-----------------------------------------------------------------

The path is certainly not indicative of the version in use. You should test the actual version of Xcode.

Comment 11

[Gregory Szorc [:gps]]

Putting this back in the general bug pool.

Comment 12

[Gregory Szorc [:gps]]

To potential contributors:

Relevant code is in python/mozboot/mozboot/osx.py in mozilla-central. You'll need to beef up the OS X detection per the previous comments.

Comment 13

[Bharat Shetty Barkur]

Hi, 

Bharat here.

I have understood the requirements and I would like to take up this bug.

- Bharat

Comment 14

[Jeff Hammel]

Thanks, Bharat!  You are now assigned :)

Comment 15

[Bharat Shetty Barkur]

Attachment: Patch for beefing up the osx detection logic using xcodebuild

I have a draft diff for this bug. Please review. If the logic is not right or can be improved or is missing anything, please comment.

Also, I need help on testing this patch for all the cases.

Thanks,
- B

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 671161 [details] [diff] [review]
Patch for beefing up the osx detection logic using xcodebuild

Thank you Bharat! Just some thoughts before Greg comes to a review...

>+    def check_if_xcode_installed(self):
>+	return subprocess.call(["type", "xcodebuild1"], 

I assume the '1' was added by accident here?

>+    def get_xcode_version():
>+	ver_str = subprocess.check_output(["xcodebuild", "-version"])
>+	ver = ver_str.split("\n")[0]
>+	ver_num_str = ver.split(" ")[1]
>+	main_ver_num = ver_num_str.split(".")[0]
>+	sub_ver = ver_num_str.split(".")[1]
>+	return main_ver_num, sub_ver

A regular expression should be simpler to do here. 

>+    def print_xcode_latest_msg(self):
>+        print(XCODE_REQUIRED)
>+                	
>+	subprocess.check_call(['open', XCODE_APP_STORE])
>+       	
>+	print('Once the install has finished, please relaunch this script.')
>+	sys.exit(1)

I can't follow the code here. Is it a wrong indentation?

>+	if (self.check_if_xcode_installed() == False):
>+	    if self.os_version >= 7:
>+		self.print_xcode_latest_msg()
>+        else:
>+	    xcode_ver, xcode_sub_ver = get_xcode_version()
>+	    
>+	    if self.os_version >= 7:
>+		if ((xcode_ver <= 4) and (xcode_sub_ver < 3)):
>+		    print(XCODE_UPGRADE_REQUIRED)		
>+        	    sys.exit(1)

Wouldn't it be enough to say that we need XCode in version >x.y?

Comment 17

[Bharat Shetty Barkur]

(In reply to Henrik Skupin (:whimboo) from comment #16)
> Comment on attachment 671161 [details] [diff] [review]
> Patch for beefing up the osx detection logic using xcodebuild
> 
> Thank you Bharat! Just some thoughts before Greg comes to a review...
Thanks Henrik for fast review :)

> 
> >+    def check_if_xcode_installed(self):
> >+	return subprocess.call(["type", "xcodebuild1"], 
> 
> I assume the '1' was added by accident here?
Yes, sorry. Actually, I was trying a test to see what happens when a command doesn't exist on OS X. So was using xcodebuild1 instead of xcodebuild. Will be careful in future.

> >+    def get_xcode_version():
> >+	ver_str = subprocess.check_output(["xcodebuild", "-version"])
> >+	ver = ver_str.split("\n")[0]
> >+	ver_num_str = ver.split(" ")[1]
> >+	main_ver_num = ver_num_str.split(".")[0]
> >+	sub_ver = ver_num_str.split(".")[1]
> >+	return main_ver_num, sub_ver
> 
> A regular expression should be simpler to do here. 
Will look around on how to achieve this in regex.

> 
> >+    def print_xcode_latest_msg(self):
> >+        print(XCODE_REQUIRED)
> >+                	
> >+	subprocess.check_call(['open', XCODE_APP_STORE])
> >+       	
> >+	print('Once the install has finished, please relaunch this script.')
> >+	sys.exit(1)
> 
> I can't follow the code here. Is it a wrong indentation?
I think there is some problem with my vim settings on OS X. All those 4 lines were supposed to be inside print_xcode_latest_msg function. Also, please guide me on the coding style for these. For eg: after the print in the above, should there be newline before subprocess.check_call etc.

> 
> >+	if (self.check_if_xcode_installed() == False):
> >+	    if self.os_version >= 7:
> >+		self.print_xcode_latest_msg()
> >+        else:
> >+	    xcode_ver, xcode_sub_ver = get_xcode_version()
> >+	    
> >+	    if self.os_version >= 7:
> >+		if ((xcode_ver <= 4) and (xcode_sub_ver < 3)):
> >+		    print(XCODE_UPGRADE_REQUIRED)		
> >+        	    sys.exit(1)
> 
> Wouldn't it be enough to say that we need XCode in version >x.y?
Let us see what others say about this logic.

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 671161 [details] [diff] [review]
Patch for beefing up the osx detection logic using xcodebuild

Review of attachment 671161 [details] [diff] [review]:
-----------------------------------------------------------------

Overall it looks very good. I think the 2nd version will be very close. You are already ahead of the curve compared to most first-time contributors to Mozilla!

One general nit: there's a bunch of trailing whitespace. Please remove it.

::: python/mozboot/mozboot/osx.py
@@ +105,5 @@
>          self.ensure_homebrew_packages()
>  
> +    def check_if_xcode_installed(self):
> +	return subprocess.call(["type", "xcodebuild1"], 
> +		stdout=subprocess.PIPE, stderr=subprocess.PIPE) == 0	

Do you think checking for the existence of /usr/bin/xcodebuild is sufficient? Or, could this binary be installed somewhere else?

What about specifying the full path to "type" e.g. "/usr/bin/type"? This binary is built-in and I'm pretty sure the version in /usr/bin should always be sufficient.

@@ +108,5 @@
> +	return subprocess.call(["type", "xcodebuild1"], 
> +		stdout=subprocess.PIPE, stderr=subprocess.PIPE) == 0	
> +	
> +    def get_xcode_version():
> +	ver_str = subprocess.check_output(["xcodebuild", "-version"])

subprocess.check_output is only available in Python 2.7+ and mozboot currently targets 2.6+ because that's what older versions of OS X ship with. Fortunately, the base class (BaseBootstrapper) implements check_output. So, you should just call self.check_output(...).

@@ +113,5 @@
> +	ver = ver_str.split("\n")[0]
> +	ver_num_str = ver.split(" ")[1]
> +	main_ver_num = ver_num_str.split(".")[0]
> +	sub_ver = ver_num_str.split(".")[1]
> +	return main_ver_num, sub_ver

I too think a regular expression would be best. How about:

  output = subprcess.check_output(...)

  match = re.search('^Xcode (\d+)\.(\d+)\.(\d+)', output, re.MULTILINE)

  if not match:
     # Should only happen if the output wasn't the expected format.

  return int(match.group(1)), int(match.group(2)), int(match.group(3))

@@ +127,2 @@
>      def ensure_xcode(self):
> +	if (self.check_if_xcode_installed() == False):

Nit: Never compare directly to True or False. if implies boolean conversion. Also, parenthesis aren't needed in conditional expressions in Python. e.g. "if self.check_if_xcode_installed:"

@@ +133,5 @@
> +	    
> +	    if self.os_version >= 7:
> +		if ((xcode_ver <= 4) and (xcode_sub_ver < 3)):
> +		    print(XCODE_UPGRADE_REQUIRED)		
> +        	    sys.exit(1)

We should probably open the App Store like we do in print_xcode_latest_msg().

What about self.os_version < 7? Perhaps the review tool is just swallowing the code from the context display. I really wish Bugzilla integrated with our source control system so you could expand context...

Comment 19

[Jeff Hammel]

+    def get_xcode_version():
+	ver_str = subprocess.check_output(["xcodebuild", "-version"])
+	ver = ver_str.split("\n")[0]
+	ver_num_str = ver.split(" ")[1]
+	main_ver_num = ver_num_str.split(".")[0]
+	sub_ver = ver_num_str.split(".")[1]
+	return main_ver_num, sub_ver

We probably *should* document what the version is supposed to look like in a docstring to this function. 

(::sigh:: you people and your regexes :P)

Comment 20

[Mike Hoye [:mhoye]]

Can you confirm that you're still working on this bug?

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

That was not intentional, playing with kanbanzilla app made a whiteboard tag on my behalf. Sorry for bugspam. Mike - the activity on this bug prompted Bharat to email me and let me know they are not working on this bug:

"Please transfer the ownership of this bug to someone else, as I have been busy with other projects. I did mail to others earlier, but, for some reasons it was never transferred."

Comment 22

[Gregory Szorc [:gps]]

Nobody has complained about broken bootstrap support on older OS X's. I'm not sure if this is still a legit bug or if it got fixed as part of other improvements to the bootstrapper.

I'm marking this WORKSFORME. Please reopen if the issue persists and you want to work on it.