Have MozBoot support msys2

RESOLVED FIXED in Firefox 49

Status

--
enhancement
RESOLVED FIXED
3 years ago
7 months ago

People

(Reporter: Nat, Assigned: Nat)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Unspecified
Windows
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(6 attachments, 2 obsolete attachments)

58 bytes, text/x-review-board-request
gps
: review+
Details
58 bytes, text/x-review-board-request
gps
: review+
Details
58 bytes, text/x-review-board-request
gps
: review+
Details
58 bytes, text/x-review-board-request
gps
: review+
Details
58 bytes, text/x-review-board-request
gps
: review+
Details
58 bytes, text/x-review-board-request
gps
: review+
Details
(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1100925
(Assignee)

Updated

3 years ago
Assignee: nobody → nhakkakzadeh
Status: NEW → ASSIGNED
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.
Also, supporting the installer (NSIS) is also a non-requirement for the first version.
(Assignee)

Comment 3

3 years ago
Created attachment 8756141 [details]
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/
Attachment #8756141 - Flags: review?(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.
Attachment #8756141 - Flags: review?(gps) → review+
(Assignee)

Comment 5

3 years ago
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?
(Assignee)

Comment 7

3 years ago
Created attachment 8756464 [details]
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/
Attachment #8756464 - Flags: review?(gps)
(Assignee)

Comment 8

3 years ago
Created attachment 8756531 [details]
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/
Attachment #8756531 - Flags: review?(gps)
(Assignee)

Comment 9

3 years ago
Created attachment 8756538 [details]
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/
Attachment #8756538 - Flags: review?(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.
Attachment #8756464 - Flags: review?(gps)
(Assignee)

Comment 11

3 years ago
Created 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

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/
Attachment #8756554 - Flags: review?(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.
Attachment #8756531 - Flags: review?(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.
Attachment #8756538 - Flags: review?(gps)
(Assignee)

Comment 14

3 years ago
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/
(Assignee)

Comment 15

3 years ago
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.
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.
(Assignee)

Comment 17

3 years ago
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/
Attachment #8756464 - Flags: review?(gps)
Attachment #8756531 - Flags: review?(gps)
(Assignee)

Comment 18

3 years ago
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/
(Assignee)

Comment 19

3 years ago
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/
(Assignee)

Comment 20

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8756538 - Attachment is obsolete: true
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`)
Attachment #8756554 - Flags: review?(gps)
(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.
(Assignee)

Comment 25

3 years ago
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/
Attachment #8756554 - Flags: review?(gps)
(Assignee)

Comment 26

3 years ago
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 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"
Attachment #8756141 - Flags: review+
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?
Attachment #8756531 - Flags: review?(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?
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==';'
(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.
(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.
(Assignee)

Comment 33

3 years ago
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/
Attachment #8756141 - Flags: review?(gps)
Attachment #8756531 - Flags: review?(gps)
(Assignee)

Comment 34

3 years ago
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/
(Assignee)

Comment 35

3 years ago
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/
(Assignee)

Comment 36

3 years ago
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/
(Assignee)

Comment 37

3 years ago
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/
(Assignee)

Comment 38

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8756554 - Attachment is obsolete: true
Attachment #8756554 - Flags: review?(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.
Attachment #8756141 - Flags: review?(gps) → review+
Comment on attachment 8756464 [details]
MozReview Request: Bug 1275437 - Added methods for interacting with pacman. r?gps

https://reviewboard.mozilla.org/r/55198/#review52026
Attachment #8756464 - Flags: review?(gps) → review+
(Assignee)

Comment 41

3 years ago
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 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.
Attachment #8756531 - Flags: review?(gps)
(Assignee)

Comment 43

3 years ago
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/
Attachment #8756531 - Flags: review?(gps)
(Assignee)

Comment 44

3 years ago
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'.
(Assignee)

Comment 45

3 years ago
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/
(Assignee)

Comment 46

3 years ago
Created attachment 8756949 [details]
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/
Attachment #8756949 - Flags: review?(gps)
(Assignee)

Comment 47

3 years ago
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/
(Assignee)

Comment 48

3 years ago
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/
(Assignee)

Comment 49

3 years ago
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/
(Assignee)

Comment 50

3 years ago
Created attachment 8757017 [details]
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/
Attachment #8757017 - Flags: review?(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.
Attachment #8756531 - Flags: review?(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.
Attachment #8756949 - Flags: review?(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.
Attachment #8757017 - Flags: review?(gps)
(Assignee)

Comment 54

3 years ago
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.
(Assignee)

Comment 55

3 years ago
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/
Attachment #8757017 - Flags: review?(gps)
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.
Attachment #8757017 - Flags: review?(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.
(Assignee)

Comment 58

3 years ago
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/
Attachment #8756531 - Flags: review?(gps)
Attachment #8756949 - Flags: review?(gps)
Attachment #8757017 - Flags: review?(gps)
(Assignee)

Comment 59

3 years ago
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/
(Assignee)

Comment 60

3 years ago
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/
(Assignee)

Comment 61

3 years ago
Created attachment 8758496 [details]
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/
Attachment #8758496 - Flags: review?(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).
Attachment #8756949 - Flags: review?(gps) → review+
Comment on attachment 8756949 [details]
Bug 1275437 - Added installing of Mercurial to Windows boostrapper.

https://reviewboard.mozilla.org/r/55478/#review53438
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
Attachment #8757017 - Flags: review?(gps) → review+
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.
Attachment #8758496 - Flags: review?(gps) → review+
Attachment #8756531 - Flags: review?(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.
(Assignee)

Comment 67

3 years ago
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?
(Assignee)

Comment 68

3 years ago
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/
Attachment #8756531 - Flags: review?(gps)
(Assignee)

Comment 69

3 years ago
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/
(Assignee)

Comment 70

3 years ago
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/
(Assignee)

Comment 71

3 years ago
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/
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.
(Assignee)

Comment 73

3 years ago
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?
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.
(Assignee)

Comment 75

3 years ago
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...
(Assignee)

Comment 76

3 years ago
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/
Attachment #8756531 - Attachment description: MozReview Request: Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman. r?gps → Bug 1275437 - Windows bootstrapper installs necessary system and browser packages that come from pacman.
Attachment #8756949 - Attachment description: MozReview Request: Bug 1275437 - Added installing of Mercurial to Windows boostrapper. → Bug 1275437 - Added installing of Mercurial to Windows boostrapper.
Attachment #8757017 - Attachment description: MozReview Request: Bug 1275437 - Fixed some offending version checks that were inhereted from the BaseBootstrapper r?gps → Bug 1275437 - Fixed some offending version checks that were inhereted from the BaseBootstrapper
Attachment #8758496 - Attachment description: MozReview Request: Bug 1275437 - Moved installing of Python into an install_python method in the windows bootstrapper; r?gps → Bug 1275437 - Moved installing of Python into an install_python method in the windows bootstrapper;
(Assignee)

Comment 77

3 years ago
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/
(Assignee)

Comment 78

3 years ago
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/
(Assignee)

Comment 79

3 years ago
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 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
Attachment #8756531 - Flags: review?(gps) → review+

Comment 81

3 years ago
Pushed by gszorc@mozilla.com:
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

Updated

a year ago
Product: Core → Firefox Build System

Updated

7 months ago
Duplicate of this bug: 1331219
You need to log in before you can comment on or make changes to this bug.