805811 - bootstrap.py/mozboot should support MacPorts and/or Fink

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[jarmstrong]

I noticed an inconsistency in the build setup while reinstalling a mac this morning.


[**] The pre-requisite docs page mention using either homebrew or mac ports for package management:

https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions/Mac_OS_X_Prerequisites



[**] The bootstrap.py script will explicitly download and install homebrew:

Saving to: ‘bootstrap.py’
2012-10-26 10:39:39 (108 MB/s) - ‘bootstrap.py’ saved [4300/4300]

We will install the Homebrew package manager to install required packages.

You will be prompted to install Homebrew with its default settings. If you
would prefer to do this manually, hit CTRL+c, install Homebrew yourself, ensure
"brew" is in your $PATH, and relaunch bootstrap.



[**] homebrew will later complain about the mix of packgage managers in use.

bash-3.2# /usr/local/bin/brew install yasm mercurial gawk
Warning: It appears you have MacPorts or Fink installed.
Software installed with other package managers causes known problems for
Homebrew. If a formula fails to build, uninstall MacPorts/Fink and try again.
==> Downloading http://tortall.net/projects/yasm/releases/yasm-1.2.0.tar.gz



[**]  If a user already has a package manager installed bootstrap should pay attention to their preference and provide an override rather than quietly forcing them to make a switch.

If there is a clear benefit to using homebrew for package management enough to justify switching { similar to macports -vs- fink }.  Bootstrap should probably fail early citing the error above to reduce surprise along with updating web docs to no longer suggest macports as a viable option for building firefox after release [n].

Comment 1

[Gregory Szorc [:gps]]

mozboot/bootstrap.py is a best-effort to automate the build prerequisites. When I implemented mozboot initially, I supported Homebrew because that's what I run. I think user choice is good and that MacPorts and Fink should be supported at some point. I agree that mozboot should respect any existing package manager instead of forcing Homebrew.

Changing this bug to be about implementing support for MacPorts and/or Fink in mozboot.

To anyone coming along, the files in question are located in the main mozilla-central repository under python/mozboot. You are interested in osx.py.

Comment 2

[Zuhao(Joe) Chen]

Can I take this bug?

Comment 3

[Gregory Szorc [:gps]]

Go for it.

Comment 4

[Gregory Szorc [:gps]]

Any update here? If you need help, just ask.

Comment 5

[Zuhao(Joe) Chen]

(In reply to Gregory Szorc [:gps] from comment #4)
> Any update here? If you need help, just ask.

Sorry just got back from vacation. Will restart working on it right away. Hopefully I can get something tonight.

Comment 6

[Zuhao(Joe) Chen]

Hi Gregory,

I was looking at the http://mxr.mozilla.org/mozilla-central/source/python/mozboot/bin/bootstrap.py
For homebrew, it grabs a bootstrap script on github to install homebrew. For this bug, do you want me to write a script that install macports? Or you already have preferred one available somewhere?

Or just prompt the user "macports is not installed" if users choose macports as option but it's not installed.

Thanks,
Joe

Comment 7

[Gregory Szorc [:gps]]

If the user wants to use macports and it isn't installed, the bootstrapper should download the relevant installer from macports.org and launch it. The installer doesn't need to be completely automated (it's fine if the user has to click through, etc).

Comment 8

[Zuhao(Joe) Chen]

Attachment: First Patch for bug805811

Added couple functions to install macports and relevant packages.

I encounter a weird problem. When I try to add a try except clause to handle input error in install_system_packages() method. It allways return a syntax error. Then I dived a little bit deeper into it and found that it can't import Bootstrapper if I added that clause. Any idea why?

Thanks,
Joe

Comment 9

[Zuhao(Joe) Chen]

Is this bug still active?

Comment 10

[Gregory Szorc [:gps]]

Sorry for the delay on review. I've been extremely busy on other high-priority projects. I hope to get around to this next week. Feel free to keep poking me.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 705133 [details] [diff] [review]
First Patch for bug805811

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

This looks really good for a first version! Just a few changes to raise the code quality bar.

I'll also be away Thursday through Monday'ish. So, no need to rush :)

::: python/mozboot/mozboot/osx.py
@@ +83,5 @@
> +We are now installing all required packages via MacPorts. You will see a lot of
> +output as packages are built.
> +'''
> +
> +HOMEBREW_OLD_CLANG = '''

MACPORTS_OLD_CLANG?

Also, since this is essentially copied from HOMEBREW_OLD_CLANG, why don't you add a '%s' and feed in the appropriate packager manager when you use the string.

@@ +109,5 @@
>  
>      def install_system_packages(self):
>          self.ensure_xcode()
> +
> +        choice = int(raw_input(PKGMGR_CHOICE))

This could throw a ValueError for inputs like "foo". I'd add a method on the base class since this is likely to be used by other systems:

    def prompt_int(self, prompt, low, high):
        '''Prompts the user with prompt and requires an integer between log and high.'''

@@ +116,5 @@
> +            self.ensure_homebrew()
> +            self.ensure_homebrew_packages()
> +        else:
> +            self.ensure_macports()
> +            self.ensure_macports_packages()

Do you think it is worth searching the user's system for Homebrew and MacPorts before prompting? Presumably if they have one or the other installed they would prefer to just use that one. We could always do this as a follow-up bug (presumably the user knows which one she has installed!).

@@ +218,5 @@
> +        if self.which('port') is not None:
> +            return
> +
> +        print(MACPORTS_INSTALL[self.os_version])
> +        pkg = urllib2.urlopen(url=MACPORTS_INSTALL[self.os_version], timeout=20).read()

I know you are copying the Homebrew code, but is 20s sufficient for this file? This file appears to be ~500k. We should consider dial-up users here. How about 300s?

@@ +235,5 @@
> +                    ('mercurial','mercurial'),
> +                    ('yasm', 'yasm'),
> +                    ('libidl', 'libidl'),
> +                    ('autoconf213', 'autoconf213'),
> +                    ]

These are all identical so I'm not sure we need the 2-tuple. A simple list should do.

@@ +236,5 @@
> +                    ('yasm', 'yasm'),
> +                    ('libidl', 'libidl'),
> +                    ('autoconf213', 'autoconf213'),
> +                    ]
> +        

Nit: leading whitespace.

@@ +247,5 @@
> +            if not printed:
> +                print(MACPORTS_PACKAGES)
> +                printed = True
> +
> +            subprocess.check_call([port, '-v', 'install', package])

Another way to write this would be:

# Convert to set for performance reasons.
installed = set(self.check_output([port, 'list']).split())

required = [...]

missing = [package for package in required if package not in installed]

if missing:
     # print and call install.

Comment 12

[Zuhao(Joe) Chen]

Attachment: New patch for this bug

I kinda restructure it a little bit. Mainly added the package manager selection procedure into it.

Comment 13

[Zuhao(Joe) Chen]

Attachment: Fixed a minor error in last patch

Fixed a minor error in last patch.

Originally I wanted has a list of dict to contain package managers so that indexing them is a little bit more robust but I think I'm making the problem more complicated since there are probably only three package managers(Homebrew, MacPorts or Fink). I don't like the code I write here in ensure_pkg_mgr method but I couldn't think of a nicer way...sigh...Any suggestion on this?

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 712780 [details] [diff] [review]
Fixed a minor error in last patch

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

Looking very good! Just a few small nits to correct. The next patch will likely get r+.

General nit: Comments should be full sentences. Start with a capital letter and end with a period. This is a general Mozilla style convention (it annoyed me when I started contributing as well). It apparently makes a huge different to non-English speakers (they can feed comments into translators and they come out somewhat decently unlike incomplete sentences, which are harder for translators to deal with).

Along the same vein, please expand "pkg mgr" to "package manager" everywhere. Again, foreign speakers may not get abbreviations.

::: python/mozboot/mozboot/base.py
@@ +81,5 @@
> +                raise Exception("Please enter a valid option!")
> +        except ValueError:
> +            raise Exception("Please enter a valid option!")
> +        finally:
> +            return choice
\ No newline at end of file

Let's put a while loop in here and reprompt until we get an allowed value. This should cut down on complexity at the callers.

::: python/mozboot/mozboot/osx.py
@@ +21,5 @@
>  HOMEBREW_AUTOCONF213 = 'https://raw.github.com/Homebrew/homebrew-versions/master/autoconf213.rb'
>  
> +MACPORTS_URL = {"10.8": 'https://distfiles.macports.org/MacPorts/MacPorts-2.1.2-10.8-MountainLion.pkg',
> +                "10.7": 'https://distfiles.macports.org/MacPorts/MacPorts-2.1.2-10.7-Lion.pkg',
> +                "10.6": 'https://distfiles.macports.org/MacPorts/MacPorts-2.1.2-10.6-SnowLeopard.pkg',}

I noticed 2.1.3 was released a few days ago. You might want to update to that.

It's too bad MacPorts doesn't contain a "latest stable" package. It's going to be annoying having to change this every few months :/

@@ +117,5 @@
> +        choice = self.ensure_pkg_mgr()
> +        if choice == 1:
> +            self.ensure_homebrew_packages()
> +        elif choice == 2:
> +            self.ensure_macports_packages()

Let's return strings. Plain integers aren't very readable and are prone to changing.

@@ +222,5 @@
> +        for missing in missings:
> +            subprocess.check_call([port, '-v', 'install', missing])
> +
> +        if self.os_version < StrictVersion('10.7') and 'llvm' not in installed:
> +            print(HOMEBREW_OLD_CLANG)

Wasn't HOMEBREW_OLD_CLANG renamed?

@@ +226,5 @@
> +            print(HOMEBREW_OLD_CLANG)
> +
> +            subprocess.check_call([port, '-v', 'install', 'llvm'])
> +
> +    def ensure_pkg_mgr(self):

Let's spell it out: ensure_package_manager.

@@ +229,5 @@
> +
> +    def ensure_pkg_mgr(self):
> +        '''
> +        Search package mgr in sys.path, if none is found, prompt the user to install one.
> +        If only one is found, use that one. If both are found, prompt the user to choice

I think you meant "choose" instead of "choice."

@@ +239,5 @@
> +        for name, cmd in PKG_MGR.iteritems():
> +            if self.which(cmd) is not None:
> +                installed.append(name)
> +
> +        if len(installed) == 0:

if not installed:

Empty lists in Python evaluate as falsey.

@@ +250,5 @@
> +            elif choice == 2:
> +                self.install_macports()
> +                return 2
> +            else:
> +                raise Exception("ERROR! Invalid choice!")

Instead of raising on invalid input here, I'd just have prompt_int() keep reprompting. Maybe have it error after say 3 failed attempts. Or, have it go in an infinite loop if you are feeling lazy :)

Comment 15

[Zuhao(Joe) Chen]

Attachment: Rework after Gregory's feedback

Added max try attempts to prompt_int method in base class.

Simplified ensure_package_manager method.

Comment 16

[Zuhao(Joe) Chen]

Attachment: Corrected duplicate variable name issue.

I just found a duplicate global variable name in the patch and I rename one of the variable.

Comment 17

[Gregory Szorc [:gps]]

I committed this with some minor changes:

https://hg.mozilla.org/mozilla-central/rev/dd821b465e97

Thank you for the patch!

Comment 18

[Zuhao(Joe) Chen]

(In reply to Gregory Szorc [:gps] from comment #17)
> I committed this with some minor changes:
> 
> https://hg.mozilla.org/mozilla-central/rev/dd821b465e97
> 
> Thank you for the patch!

Great! I saw the changes there and I'm sorry for the code quality of mine. I should have done it better...I really appreciate your feedback during review process.

Just wondering if there is larger project regarding automation tool I could participate in since I really want to contribute more because I found this really helpful for me to learn to produce code with higher quality. Is there any long term project that I may participate in? Thanks in advance!

Comment 19

[Gregory Szorc [:gps]]

(In reply to Zuhao(Joe) Chen from comment #18)
> Just wondering if there is larger project regarding automation tool I could
> participate in since I really want to contribute more because I found this
> really helpful for me to learn to produce code with higher quality. Is there
> any long term project that I may participate in? Thanks in advance!

Definitely. We even have a full team named "Automation and Tools" (commonly known as the A-Team)! I think https://wiki.mozilla.org/Auto-tools/New_Contributor is where you want to start. Jeff Hammel (CCd) can also give you more pointers.

Comment 20

[Jeff Hammel]

(In reply to Gregory Szorc [:gps] from comment #19)
> (In reply to Zuhao(Joe) Chen from comment #18)
> > Just wondering if there is larger project regarding automation tool I could
> > participate in since I really want to contribute more because I found this
> > really helpful for me to learn to produce code with higher quality. Is there
> > any long term project that I may participate in? Thanks in advance!
> 
> Definitely. We even have a full team named "Automation and Tools" (commonly
> known as the A-Team)! I think
> https://wiki.mozilla.org/Auto-tools/New_Contributor is where you want to
> start. Jeff Hammel (CCd) can also give you more pointers.

Yep, that is a good place to start.  Feel free to hit me up (jhammel) in the #ateam channel and I'm happy to talk more.