1275437 - Have MozBoot support msys2

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Nathan Hakkakzadeh [:Nat]]

Currently, Windows developers have to use MozillaBuild to get a working build environment. But, MozillaBuild sets up an msys 1.x environment, which has is pretty limited compared to msys2, which is complete with pacman, a package manger. Because msys2 comes with pacman, we should be able to use MozBoot to make any msys2 environment be able to build Firefox.

Comment 1

[Gregory Szorc [:gps]]

Perfect is the enemy of done here. I'll r+ an initial version that adds basic Windows support to bootstrap, even if it can't produce a full build. (e.g. because of virtualenv bugs).

We should put Windows support in mozboot behind an environment variable until we can actually produce builds with it.

Comment 2

[Gregory Szorc [:gps]]

Also, supporting the installer (NSIS) is also a non-requirement for the first version.

Comment 3

[Nathan Hakkakzadeh [:Nat]]

Attachment: MozReview Request: Bug 1275437 - Added placeholder for Windows bootstrapper. r?gps

Created a WindowsBootstrapper class that raises a NotImplementedError when initialized.

Bootstrapper now detects if the system is being run on Windows, and if it is dispatches to the WindowsBootstrapper.

Review commit: https://reviewboard.mozilla.org/r/54978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54978/

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8756141 [details]
MozReview Request: Bug 1275437 - Added placeholder for Windows bootstrapper. r?gps

https://reviewboard.mozilla.org/r/54978/#review51834

Looks good!

Are you going to keep adding features/commits to this bug or do you want me to land this? Keep in mind you need to keep opening new bugs when things land. It's how we track things.

Comment 5

[Nathan Hakkakzadeh [:Nat]]

https://reviewboard.mozilla.org/r/54978/#review51834

I'm going to keep adding features/commits to this bug. Although, I'd like to keep sending smaller commits to get reviewed. Is that alright?

Comment 6

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/54978/#review51834

That's perfectly alright.

Comment 7

[Nathan Hakkakzadeh [:Nat]]

Attachment: MozReview Request: Bug 1275437 - Added methods for interacting with pacman. r?gps

These new convenience methods let the bootstrapper update the local package list, upgrade all installed packages, and install new packages.

Review commit: https://reviewboard.mozilla.org/r/55198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55198/

Comment 8

[Nathan Hakkakzadeh [:Nat]]

Attachment: Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

Windows bootstrapper checks if pacman is installed before continuing.

Added a convenience method similar to BaseBootstrapper.which that works with the mingw version of python in msys2.

Review commit: https://reviewboard.mozilla.org/r/55226/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55226/

Comment 9

[Nathan Hakkakzadeh [:Nat]]

Attachment: MozReview Request: Bug 1275437 - Bootstrapper now recognizes msys version of Python. r?gps

Before, the bootstrapper would only know it was being run on Windows if it was run with the official Windows version of Python or the mingw version of Python.

Review commit: https://reviewboard.mozilla.org/r/55228/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55228/

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8756464 [details]
MozReview Request: Bug 1275437 - Added methods for interacting with pacman. r?gps

https://reviewboard.mozilla.org/r/55198/#review51928

Looks mostly good!

::: python/mozboot/mozboot/windows.py:21
(Diff revision 1)
> +
> +    def run(self, command):
> +        subprocess.check_call(command, stdin=sys.stdin)
> +
> +    def pacman_update(self):
> +        command = ['pacman', '-S', '--refresh']

Let's use `--sync` instead of `-S` to make this easier to read for people not familiar with pacman.

::: python/mozboot/mozboot/windows.py:25
(Diff revision 1)
> +    def pacman_update(self):
> +        command = ['pacman', '-S', '--refresh']
> +        self.run(command)
> +
> +    def pacman_upgrade(self):
> +        command = ['pacman', '-Syu']

Please expand these arguments too.

Comment 11

[Nathan Hakkakzadeh [:Nat]]

Attachment: MozReview Request: Bug 1275437 - Changed flags passed to pacman in Windows bootstrapper to be the --long-form instead of the single character flag. r?gps

This will make it easier to read for people who are not familiar with pacman.

Review commit: https://reviewboard.mozilla.org/r/55238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55238/

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

https://reviewboard.mozilla.org/r/55226/#review51930

::: python/mozboot/mozboot/windows.py:45
(Diff revision 1)
> +        '''Returns true if the program `name` exists on current system, false otherwise. Necessary because BaseBootstrapper.which
> +        does not work when the mingw version of python is run from a UNIX-like shell because os.pathsep gives a Windows path 
> +        seperator, not a UNIX path seperator.

0_o

What exactly is the problem with path normalization?

::: python/mozboot/mozboot/windows.py:62
(Diff revision 1)
> +        raise NotImplementedError('Bootstrap support is not yet available for Android on Windows. '
> +                                  'For now, use MozillaBuild to set up a build environment on Windows.')

I /think/ we don't support building Fennec (Android) on Windows. But you may want to double check in #mobile. If we don't, this error message should be changed.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8756538 [details]
MozReview Request: Bug 1275437 - Bootstrapper now recognizes msys version of Python. r?gps

https://reviewboard.mozilla.org/r/55228/#review51936

You may want to fold this commit into an earlier one.

::: python/mozboot/mozboot/bootstrap.py:145
(Diff revision 1)
>              args['flavor'] = platform.system()
>  
> -        elif sys.platform.startswith('win32'):
> +        elif sys.platform.startswith('win32') or sys.platform.starswith('msys'):
> -            # TODO: Check to see if platform is actually msys2 or not.
>              cls = WindowsBootstrapper
> -
> +        

Trailing whitespace here.

Comment 14

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756554 [details]
MozReview Request: Bug 1275437 - Changed flags passed to pacman in Windows bootstrapper to be the --long-form instead of the single character flag. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55238/diff/1-2/

Comment 15

[Nathan Hakkakzadeh [:Nat]]

https://reviewboard.mozilla.org/r/55226/#review51930

> 0_o
> 
> What exactly is the problem with path normalization?

When you're using a MinGW or native Windows version of Python, os.pathsep returns ';' because that's the path seperator for Win32. But, if you've called that verison of Python from a msys2 environment, your PATH environment variable with be seperated by ':'. So the Python which implementation in BaseBootstrapper won't work because it isn't splitting on the right character.

Comment 16

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/55226/#review51930

> When you're using a MinGW or native Windows version of Python, os.pathsep returns ';' because that's the path seperator for Win32. But, if you've called that verison of Python from a msys2 environment, your PATH environment variable with be seperated by ':'. So the Python which implementation in BaseBootstrapper won't work because it isn't splitting on the right character.

Yuck. I have a feeling this will break random things in the build system. But that's not a problem that needs solved today.

Comment 17

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756141 [details]
MozReview Request: Bug 1275437 - Added placeholder for Windows bootstrapper. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54978/diff/1-2/

Comment 18

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756464 [details]
MozReview Request: Bug 1275437 - Added methods for interacting with pacman. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55198/diff/1-2/

Comment 19

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55226/diff/1-2/

Comment 20

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756554 [details]
MozReview Request: Bug 1275437 - Changed flags passed to pacman in Windows bootstrapper to be the --long-form instead of the single character flag. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55238/diff/2-3/

Comment 21

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/54978/#review51950

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8756554 [details]
MozReview Request: Bug 1275437 - Changed flags passed to pacman in Windows bootstrapper to be the --long-form instead of the single character flag. r?gps

https://reviewboard.mozilla.org/r/55238/#review51956

Please fold this commit into the one that introduced these functions. (Use `hg histedit`)

Comment 23

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/55238/#review51958

Comment 24

[Mike Hommey [:glandium]]

(In reply to Nathan Hakkakzadeh [:Nat] from comment #15)
> https://reviewboard.mozilla.org/r/55226/#review51930
> 
> > 0_o
> > 
> > What exactly is the problem with path normalization?
> 
> When you're using a MinGW or native Windows version of Python, os.pathsep
> returns ';' because that's the path seperator for Win32. But, if you've
> called that verison of Python from a msys2 environment, your PATH
> environment variable with be seperated by ':'. So the Python which
> implementation in BaseBootstrapper won't work because it isn't splitting on
> the right character.

That doesn't sound right. Something in the stack is supposed to change the PATH to use ; as a separator when invoking Win32 native executables.

Comment 25

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55226/diff/2-3/

Comment 26

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756554 [details]
MozReview Request: Bug 1275437 - Changed flags passed to pacman in Windows bootstrapper to be the --long-form instead of the single character flag. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55238/diff/3-4/

Comment 27

[Gregory Szorc [:gps]]

Comment on attachment 8756141 [details]
MozReview Request: Bug 1275437 - Added placeholder for Windows bootstrapper. r?gps

https://reviewboard.mozilla.org/r/54978/#review51970

::: python/mozboot/mozboot/bootstrap.py:143
(Diff revision 2)
>                  sys.platform.startswith('freebsd'):
>              cls = FreeBSDBootstrapper
>              args['version'] = platform.release()
>              args['flavor'] = platform.system()
>  
> +        elif sys.platform.startswith('win32') or sys.platform.starswith('msys'):

Typo here: "starswith" != "startswith"

Comment 28

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/55226/#review51930

> Yuck. I have a feeling this will break random things in the build system. But that's not a problem that needs solved today.

Could this be a difference between msys2's "python2" package and its "mingw-w64-x86_64-python2" package?

Comment 29

[Gregory Szorc [:gps]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

https://reviewboard.mozilla.org/r/55226/#review51976

::: python/mozboot/mozboot/windows.py:24
(Diff revision 3)
> +        'mingw-w64-x86_64-toolchain', # We need gcc so pip can compile Mercurial
> +        'mingw-w64-i686-toolchain'

If this is the only reason we need the toolchain, I'm tempted to fix this by uploading a MinGW wheel for Mercurial to PyPI (I maintain the Mercurial Windows wheels). Do you know if this is the only reason we need these (rather large) packages?

Comment 30

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/55226/#review51930

> Could this be a difference between msys2's "python2" package and its "mingw-w64-x86_64-python2" package?

From msys2, /usr/bin/python2 has os.pathsep==':' and /mingw64/bin/python2 has os.pathsep==';'

Comment 31

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #30)
> https://reviewboard.mozilla.org/r/55226/#review51930
> 
> > Could this be a difference between msys2's "python2" package and its "mingw-w64-x86_64-python2" package?
> 
> From msys2, /usr/bin/python2 has os.pathsep==':' and /mingw64/bin/python2
> has os.pathsep==';'

... which is expected, since /usr/bin/python2 is the msys python, where os.path is posixpath. /mingw64/bin/python2 is the win32 python, where os.path is ntpath.

(In reply to Gregory Szorc [:gps] from comment #29)
> Comment on attachment 8756531 [details]
> MozReview Request: Bug 1275437 - Windows bootstrapper installs necessary
> system and browser packages that come from pacman. r?gps
> 
> https://reviewboard.mozilla.org/r/55226/#review51976
> 
> ::: python/mozboot/mozboot/windows.py:24
> (Diff revision 3)
> > +        'mingw-w64-x86_64-toolchain', # We need gcc so pip can compile Mercurial
> > +        'mingw-w64-i686-toolchain'
> 
> If this is the only reason we need the toolchain, I'm tempted to fix this by
> uploading a MinGW wheel for Mercurial to PyPI (I maintain the Mercurial
> Windows wheels). Do you know if this is the only reason we need these
> (rather large) packages?

msys2 has a mercurial package, too.

Comment 32

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #31)
> (In reply to Gregory Szorc [:gps] from comment #30)
> > https://reviewboard.mozilla.org/r/55226/#review51930
> > 
> > > Could this be a difference between msys2's "python2" package and its "mingw-w64-x86_64-python2" package?
> > 
> > From msys2, /usr/bin/python2 has os.pathsep==':' and /mingw64/bin/python2
> > has os.pathsep==';'
> 
> ... which is expected, since /usr/bin/python2 is the msys python, where
> os.path is posixpath. /mingw64/bin/python2 is the win32 python, where
> os.path is ntpath.

But the point I'm making in 24 is that os.environ['PATH'] should have been munged by the msys->win32 transition and use ; when coming from msys and using win32 python.

Comment 33

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756141 [details]
MozReview Request: Bug 1275437 - Added placeholder for Windows bootstrapper. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54978/diff/2-3/

Comment 34

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756464 [details]
MozReview Request: Bug 1275437 - Added methods for interacting with pacman. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55198/diff/2-3/

Comment 35

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55226/diff/3-4/

Comment 36

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756554 [details]
MozReview Request: Bug 1275437 - Changed flags passed to pacman in Windows bootstrapper to be the --long-form instead of the single character flag. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55238/diff/4-5/

Comment 37

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756464 [details]
MozReview Request: Bug 1275437 - Added methods for interacting with pacman. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55198/diff/3-4/

Comment 38

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55226/diff/4-5/

Comment 39

[Gregory Szorc [:gps]]

Comment on attachment 8756141 [details]
MozReview Request: Bug 1275437 - Added placeholder for Windows bootstrapper. r?gps

https://reviewboard.mozilla.org/r/54978/#review52024

::: python/mozboot/mozboot/bootstrap.py:143
(Diff revision 3)
>                  sys.platform.startswith('freebsd'):
>              cls = FreeBSDBootstrapper
>              args['version'] = platform.release()
>              args['flavor'] = platform.system()
>  
> +        elif sys.platform.startswith('win32') or sys.platform.startswith('msys'):

Pro tip: you can specify a tuple to startswith() and endswith() to match multiple things.

Comment 40

[Gregory Szorc [:gps]]

Comment on attachment 8756464 [details]
MozReview Request: Bug 1275437 - Added methods for interacting with pacman. r?gps

https://reviewboard.mozilla.org/r/55198/#review52026

Comment 41

[Nathan Hakkakzadeh [:Nat]]

https://reviewboard.mozilla.org/r/55226/#review51930

> From msys2, /usr/bin/python2 has os.pathsep==':' and /mingw64/bin/python2 has os.pathsep==';'

Looking at it now, I think I was wrong about why it isn't working with BaseBootstrapper.which. Will update here when I find the real problem...

Comment 42

[Gregory Szorc [:gps]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

https://reviewboard.mozilla.org/r/55226/#review52032

I'd still like us to get to the bottom of the Python path separator foo and the mingw toolchain requirements before granting review on this. But otherwise this is good.

::: python/mozboot/mozboot/windows.py:61
(Diff revision 5)
> +    def install_mobile_android_packages(self):
> +        raise NotImplementedError('We do not support building Android on Windows. Sorry!')
> +
> +    def install_mobile_android_artifact_mode_packages(self):
> +        raise NotImplementedError('We do not support building Android on Windows. Sorry!')

As a follow-up, you may want to change the production selection menu at the beginning of bootstrap to not provide the option to setup Android.

Comment 43

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55226/diff/5-6/

Comment 44

[Nathan Hakkakzadeh [:Nat]]

https://reviewboard.mozilla.org/r/55226/#review51930

> Looking at it now, I think I was wrong about why it isn't working with BaseBootstrapper.which. Will update here when I find the real problem...

Figured it out. I was passing 'pacman' to which, not 'pacman.exe'.

Comment 45

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55226/diff/6-7/

Comment 46

[Nathan Hakkakzadeh [:Nat]]

Attachment: Bug 1275437 - Added installing of Mercurial to Windows boostrapper.

Added convenience method for installing from pip.

Had install_system_packages method of the windows bootstrapper install mercurial as part of the bootstrapping process.

Review commit: https://reviewboard.mozilla.org/r/55478/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55478/

Comment 47

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756141 [details]
MozReview Request: Bug 1275437 - Added placeholder for Windows bootstrapper. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54978/diff/3-4/

Comment 48

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756464 [details]
MozReview Request: Bug 1275437 - Added methods for interacting with pacman. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55198/diff/4-5/

Comment 49

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55226/diff/7-8/

Comment 50

[Nathan Hakkakzadeh [:Nat]]

Attachment: Bug 1275437 - Fixed some offending version checks that were inhereted from the BaseBootstrapper

Overrode BaseBootstrapper.which to append '.exe' to any which checks since (hopefully) anything the bootstrapper looks for, must be a windows executable.

Overrode BaseBootstrapper._hgplain_env to replace any unicode entries with str entries to avoid a bug in the MinGW version of Python.

Review commit: https://reviewboard.mozilla.org/r/55526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55526/

Comment 51

[Gregory Szorc [:gps]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

https://reviewboard.mozilla.org/r/55226/#review53222

::: python/mozboot/mozboot/windows.py:63
(Diff revision 8)
> +    def install_mobile_android_artifact_mode_packages(self):
> +        raise NotImplementedError('We do not support building Android on Windows. Sorry!')
> +
> +    def _update_package_manager(self):
> +        self.pacman_update()
> +            

Nit: trailing whitespace here.

Comment 52

[Gregory Szorc [:gps]]

Comment on attachment 8756949 [details]
Bug 1275437 - Added installing of Mercurial to Windows boostrapper.

https://reviewboard.mozilla.org/r/55478/#review53224

Mercurial should be installed/updated via a "upgrade_mercurial" method. See base.py for the logic.

Comment 53

[Gregory Szorc [:gps]]

Comment on attachment 8757017 [details]
Bug 1275437 - Fixed some offending version checks that were inhereted from the BaseBootstrapper

https://reviewboard.mozilla.org/r/55526/#review53230

::: python/mozboot/mozboot/windows.py:54
(Diff revision 1)
> +        '''The base.py file uses unicode literals which subprocess.Popen does not like on the MinGW version of Python.
> +        This removes the offending entry and replaces it with an str one.
> +        '''
> +        env = BaseBootstrapper._hgplain_env(self)
> +        del env['HGPLAIN']
> +        env[str('HGPLAIN')] = str('1')
> +        return env

Can you just change the implementation in base.py to use str keys and values in the dict instead? This feels like the appropriate solution.

Comment 54

[Nathan Hakkakzadeh [:Nat]]

https://reviewboard.mozilla.org/r/55526/#review53230

> Can you just change the implementation in base.py to use str keys and values in the dict instead? This feels like the appropriate solution.

Of course. I was just wary of accidentally messing up the other bootstrappers.

Comment 55

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8757017 [details]
Bug 1275437 - Fixed some offending version checks that were inhereted from the BaseBootstrapper

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55526/diff/1-2/

Comment 56

[(not currently active) Ted Mielczarek]

So the goal for this bug would be to change the build setup process from:
1) Install MozillaBuild
2) Build

to:
1) Install MSYS2
2) bootstrap
3) build?

I think that's fine, I just want to clarify. If we want to make MSYS2 our officially-supported build environment we'll probably also need to move the MSVC detection logic from the MozillaBuild batch scripts into configure. (I feel like there's a bug filed on this but I can't find it. glandium said he wanted to do this after he got the toolchain bits into moz.configure.) It might also be nice to build some sort of wrapper installer that installed MSYS2 and ran bootstrap in one fell swoop, but that might be less important.

Comment 57

[Gregory Szorc [:gps]]

Comment on attachment 8757017 [details]
Bug 1275437 - Fixed some offending version checks that were inhereted from the BaseBootstrapper

https://reviewboard.mozilla.org/r/55526/#review53344

::: python/mozboot/mozboot/base.py:302
(Diff revision 2)
>  
>          HGPLAIN prevents Mercurial from applying locale variations to the output
>          making it suitable for use in scripts.
>          """
>          env = os.environ.copy()
> -        env['HGPLAIN'] = '1'
> +        env[str('HGPLAIN')] = str('1')

Use b'' literals to force the str/bytes type. You almost certainly never want to use the str() or unicode() functions.

Comment 58

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55226/diff/8-9/

Comment 59

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756949 [details]
Bug 1275437 - Added installing of Mercurial to Windows boostrapper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55478/diff/1-2/

Comment 60

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8757017 [details]
Bug 1275437 - Fixed some offending version checks that were inhereted from the BaseBootstrapper

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55526/diff/2-3/

Comment 61

[Nathan Hakkakzadeh [:Nat]]

Attachment: Bug 1275437 - Moved installing of Python into an install_python method in the windows bootstrapper;

The correct version of Python will get installed from the install_python method instead of with the system packages.

This is more in-line with how a bootstrapper *should* extend from the base bootstrapper.

Review commit: https://reviewboard.mozilla.org/r/56760/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56760/

Comment 62

[Gregory Szorc [:gps]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #56)
> So the goal for this bug would be to change the build setup process from:
> 1) Install MozillaBuild
> 2) Build
> 
> to:
> 1) Install MSYS2
> 2) bootstrap
> 3) build?

Well, we'd ideally have some VBScript or some kind of one-liner or installer to streamline installing msys2 and invoking our bootstrapper. That will be a follow-up. We're likely looking at landing *some* Windows support in bootstrap behind an environment variable. Then we can follow up with the robustness/streamline improvements.

> I think that's fine, I just want to clarify. If we want to make MSYS2 our
> officially-supported build environment we'll probably also need to move the
> MSVC detection logic from the MozillaBuild batch scripts into configure. (I
> feel like there's a bug filed on this but I can't find it. glandium said he
> wanted to do this after he got the toolchain bits into moz.configure.) It
> might also be nice to build some sort of wrapper installer that installed
> MSYS2 and ran bootstrap in one fell swoop, but that might be less important.

Yes, we should definitely move the MSVC toolchain logic into configure. I'm not sure why we insist on running vcvarsall.bat for their dev environment. We can detect that stuff in configure. It really isn't that much work to reimplement the logic in vcvarsall.bat (we've practically done it for the in-tree mozconfigs, albeit with slightly different paths from the tooltool archives in order to avoid spaces).

Comment 63

[Gregory Szorc [:gps]]

Comment on attachment 8756949 [details]
Bug 1275437 - Added installing of Mercurial to Windows boostrapper.

https://reviewboard.mozilla.org/r/55478/#review53438

Comment 64

[Gregory Szorc [:gps]]

Comment on attachment 8757017 [details]
Bug 1275437 - Fixed some offending version checks that were inhereted from the BaseBootstrapper

https://reviewboard.mozilla.org/r/55526/#review53440

Comment 65

[Gregory Szorc [:gps]]

Comment on attachment 8758496 [details]
Bug 1275437 - Moved installing of Python into an install_python method in the windows bootstrapper;

https://reviewboard.mozilla.org/r/56760/#review53444

Bonus points if you fold this into the commit that introduced the package.

Comment 66

[Gregory Szorc [:gps]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

https://reviewboard.mozilla.org/r/55226/#review53446

::: python/mozboot/mozboot/windows.py:26
(Diff revision 9)
> +        'mingw-w64-x86_64-toolchain', # We need gcc so pip can compile Mercurial
> +        'mingw-w64-i686-toolchain'

Please change the comment to a "TODO" that says something about removing once we install Mercurial from a wheel.

::: python/mozboot/mozboot/windows.py:64
(Diff revision 9)
> +    def install_mobile_android_artifact_mode_packages(self):
> +        raise NotImplementedError('We do not support building Android on Windows. Sorry!')
> +
> +    def _update_package_manager(self):
> +        self.pacman_update()
> +            

Trailing whitespace.

Comment 67

[Nathan Hakkakzadeh [:Nat]]

https://reviewboard.mozilla.org/r/55226/#review53446

> Trailing whitespace.

I don't understand why this whitespace is a problem... Its right before a method definition, isn't that good?

Comment 68

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55226/diff/9-10/

Comment 69

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756949 [details]
Bug 1275437 - Added installing of Mercurial to Windows boostrapper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55478/diff/2-3/

Comment 70

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8757017 [details]
Bug 1275437 - Fixed some offending version checks that were inhereted from the BaseBootstrapper

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55526/diff/3-4/

Comment 71

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8758496 [details]
Bug 1275437 - Moved installing of Python into an install_python method in the windows bootstrapper;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56760/diff/1-2/

Comment 72

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/55226/#review53446

> I don't understand why this whitespace is a problem... Its right before a method definition, isn't that good?

People generally don't like trailing whitespace because it adds no value, makes diffs harder to read, etc. That's why it is being highlighted as a giant red blob.

You should configure your editor to strip trailing whitespace on save.

Comment 73

[Nathan Hakkakzadeh [:Nat]]

https://reviewboard.mozilla.org/r/55226/#review53446

> People generally don't like trailing whitespace because it adds no value, makes diffs harder to read, etc. That's why it is being highlighted as a giant red blob.
> 
> You should configure your editor to strip trailing whitespace on save.

But how is the whitespace marked on line 64 different from line 67 or line 71 or line 75 or line 80?

Comment 74

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/55226/#review53446

> But how is the whitespace marked on line 64 different from line 67 or line 71 or line 75 or line 80?

Line 64 has spaces on it. Lines 71, 75, etc are empty.

Comment 75

[Nathan Hakkakzadeh [:Nat]]

https://reviewboard.mozilla.org/r/55226/#review53446

> Line 64 has spaces on it. Lines 71, 75, etc are empty.

Oh... I thought you were talking about the newline. Fixing...

Comment 76

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55226/diff/10-11/

Comment 77

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8756949 [details]
Bug 1275437 - Added installing of Mercurial to Windows boostrapper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55478/diff/3-4/

Comment 78

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8757017 [details]
Bug 1275437 - Fixed some offending version checks that were inhereted from the BaseBootstrapper

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55526/diff/4-5/

Comment 79

[Nathan Hakkakzadeh [:Nat]]

Comment on attachment 8758496 [details]
Bug 1275437 - Moved installing of Python into an install_python method in the windows bootstrapper;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56760/diff/2-3/

Comment 80

[Gregory Szorc [:gps]]

Comment on attachment 8756531 [details]
Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.

https://reviewboard.mozilla.org/r/55226/#review54414

Comment 81

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e2ff34312b
Added placeholder for Windows bootstrapper. r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/b12e09309b96
Added methods for interacting with pacman. r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c18f9787879
Windows bootstrapper installs necessary system and browser packages that come from pacman. r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf30f2236629
Added installing of Mercurial to Windows boostrapper. r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/3952e910a66a
Fixed some offending version checks that were inhereted from the BaseBootstrapper r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/b86c7fc1f4bd
Moved installing of Python into an install_python method in the windows bootstrapper; r=gps

Comment 82

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f4e2ff34312b
https://hg.mozilla.org/mozilla-central/rev/b12e09309b96
https://hg.mozilla.org/mozilla-central/rev/3c18f9787879
https://hg.mozilla.org/mozilla-central/rev/cf30f2236629
https://hg.mozilla.org/mozilla-central/rev/3952e910a66a
https://hg.mozilla.org/mozilla-central/rev/b86c7fc1f4bd