842445 - Enable SeaMonkey win builders to build using pymake instead of make.

Status: RESOLVED FIXED

(SeaMonkey :: Release Engineering, defect)

Description

[Edmund Wong (:ewong)]

Eventually, SeaMonkey's Win builders will need to move to using pymake
instead of make as the default building with m-c proper is pymake.

Comment 1

[Edmund Wong (:ewong)]

Attachment: Buildbotcustom changes to enable pymake for builders.

Comment 2

[Edmund Wong (:ewong)]

Attachment: Buildbot config changes to enable pymake for Win builders.

Comment 3

[Justin Wood (:Callek)]

Comment on attachment 715378 [details] [diff] [review]
Buildbotcustom changes to enable pymake for builders.

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

Besides my comments below you also need to modify misc.py to pass the enable_pymake flag for any of this to actually work. ala http://mxr.mozilla.org/build/search?string=pymake&find=misc.py&findi=&filter=^[^\0]*%24&hitlimit=&tree=build

::: process/factory.py
@@ +1442,5 @@
>          env['MINIDUMP_STACKWALK'] = getPlatformMinidumpPath(self.platform)
>          env['MINIDUMP_SAVE_PATH'] = WithProperties('%(basedir:-)s/minidumps')
>          self.addStep(unittest_steps.MozillaCheck,
>           test_name="check",
> +         makeCMD=self.make_cmd

To do this you need to adjust: http://mxr.mozilla.org/build/source/buildbotcustom/steps/unittest.py#409 (and similar code around there)

@@ +2031,5 @@
>               extract_fn = parse_make_upload,
>               haltOnFailure=True,
>               description=["upload"],
>               timeout=40*60, # 40 minutes
>               log_eval_func=lambda c,s: regex_log_evaluator(c, s, upload_errors),

current line # 2200 needs the tweak as well (CC*BuildFactory)

@@ +3443,5 @@
>               haltOnFailure=True,
>              ))
>              prettyEnv['WIN32_INSTALLER_IN'] = WithProperties('%(win32_installer_in)s')
>          self.addStep(ShellCommand(
>           name='repack_installers_pretty',

orig file line 3443 needs adjusting:
         command=['sh', '-c',
                  WithProperties('make installers-%(locale)s LOCALE_MERGEDIR=$PWD/merged')],

same with orig file line 3634 |         command=['make', 'wget-en-US'],|

@@ +3690,1 @@
>                       'fi'],

this makes me cringe, but it works.

@@ +3724,5 @@
>              ))
>          self.addStep(ShellCommand(
>           name='repack_installers',
>           description=['repack', 'installers'],
>           command=['sh','-c',

line 3720 (orig file) also needs a change

@@ +4102,5 @@
>              ))
>  
>          self.addStep(ShellCommand(
>           name='repack_installers',
>           description=['repack', 'installers'],

line 4099 as well

@@ +5987,5 @@
> +             description='kill python',
> +             descriptionDone="killed python",
> +             command="pskill -t python.exe",
> +             workdir="D:\\Utilities"
> +            ))

don't add this kill_python. MoCo have abandoned use of pskill, and I'm unsure if that could cause buildbot itself to die with this command (I recognize we kill make above, which can stay for now)

Comment 4

[Edmund Wong (:ewong)]

Attachment: Buildbotcustom changes to enable pymake for builders. (v2)

Comment 5

[rsx11m]

SeaMonkey-Aurora builds are now broken as well after yesterday's merge...  :-(

Comment 6

[Edmund Wong (:ewong)]

Attachment: Buildbotcustom changes to enable pymake for SeaMonkey win builders. (v3)

Comment 7

[Justin Wood (:Callek)]

Comment on attachment 717621 [details] [diff] [review]
Buildbotcustom changes to enable pymake for SeaMonkey win builders. (v3)

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

Other than the misc.py issues and my nit in factory.py this *looks* pretty good. I'll need to do a deeper review later (with a dump of the master) to be certain here though, reading this line-by-line is brain numbing ;-)

Thanks though!

::: misc.py
@@ -993,1 @@
>          buildSpace = pf.get('build_space', config['default_build_space'])

Changes to this file need to also get applied in generateCCBranchObjects

Other than that this part looks good.

::: process/factory.py
@@ +420,5 @@
>  
> +        if self.enable_pymake:
> +          self.make_cmd = ['python', WithProperties("%(basedir)s/build/build/pymake/make.py")]
> +        else:
> +          self.make_cmd = ['make']

Nit: for consistency with (current) upstream buildbotcustom code about this, can you name it self.makeCmd ?

Comment 8

[Edmund Wong (:ewong)]

Attachment: Buildbotcustom changes to enable pymake on SeaMonkey builders.(v4)

Comment 9

[Justin Wood (:Callek)]

Comment on attachment 717700 [details] [diff] [review]
Buildbotcustom changes to enable pymake on SeaMonkey builders.(v4)

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

r+ with the nits I mention (I'll be landing very soon)

I should note the l10n pymake switch has a *slight* chance of being broken, because MoCo hasn't done that before now. However they *are* doing that in Bug 784848, so might as well leave it here for now, and if we're broken from here we can port the rest of that bug over.

::: process/factory.py
@@ +1442,5 @@
>          env['MINIDUMP_STACKWALK'] = getPlatformMinidumpPath(self.platform)
>          env['MINIDUMP_SAVE_PATH'] = WithProperties('%(basedir:-)s/minidumps')
>          self.addStep(unittest_steps.MozillaCheck,
>           test_name="check",
> +         makeCMD=self.makeCmd

missing trailing comma

@@ +2378,5 @@
>          if extraArgs is not None:
>              partialMarCommand.extend(extraArgs)
>          self.addStep(ShellCommand(
>              name='make_partial_mar',
> +            description=self.makeCmd + ['partial', 'mar'],

nit: no need to modify description here. (I reverted)

@@ +3690,1 @@
>                       'fi'],

similar to the reason below, we can't actually do this :(

My solution:

        if self.makeCmd[0] == 'make':
           makeCmd = 'make '
        else:
           # pymake - we can't use self.makeCmd due to embedded WithProperties
           makeCmd = 'python %(basedir)s/build/build/pymake/make.py '
        # now build the command
        command = 'if [ ! -e dist/host/bin/mbsdiff ]; then ' +\
                  makeCmd + 'tier_base;' + makeCmd + 'tier_nspr; '+\
                  makeCmd + '-C config;' +\
                  makeCmd + '-C modules/libmar;' +\
                  makeCmd + '-C modules/libbz2;' +\
                  makeCmd + '-C other-licenses/bsdiff;' +\
                  'fi'
..
   command=['sh', '-c', WithProperties(command)]

@@ +3725,5 @@
>          self.addStep(ShellCommand(
>           name='repack_installers',
>           description=['repack', 'installers'],
>           command=['sh','-c',
> +                  WithProperties(' '.join(self.makeCmd)+' installers-%(locale)s LOCALE_MERGEDIR=$PWD/merged')],

File "/builds/buildbot/master01/lib/python2.6/site-packages/buildbotcustom/process/factory.py", line 3729, in d
oRepack
    WithProperties(' '.join(self.makeCmd)+' installers-%(locale)s LOCALE_MERGEDIR=$PWD/merged')],
TypeError: sequence item 1: expected string, instance found
make: *** [checkconfig] Error 1

... The issue is because self.makeCmd is not strictly a string, part of it is a WithProperties.

My solution:

command=self.makeCmd + [WithProperties('installers-%(locale)s'), 'LOCALE_MERGEDIR=$PWD/merged'],

@@ +4104,5 @@
>          self.addStep(ShellCommand(
>           name='repack_installers',
>           description=['repack', 'installers'],
>           command=['sh','-c',
> +                  WithProperties(' '.join(self.makeCmd)+' installers-%(locale)s LOCALE_MERGEDIR=$PWD/merged')],

here too:
command=self.makeCmd + [WithProperties('installers-%(locale)s'), 'LOCALE_MERGEDIR=$PWD/merged'],

Comment 10

[Justin Wood (:Callek)]

Comment on attachment 715379 [details] [diff] [review]
Buildbot config changes to enable pymake for Win builders.

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

This looks good, I'm going to push pymake disabling for beta/release for now, so we can be sure (for now) that we won't break those branches. We'll make pymake enabled on next beta cycle.

I'll attach and r? you that patch though

Comment 11

[Justin Wood (:Callek)]

Attachment: [config] disable pymake on beta/release

Comment 12

[Justin Wood (:Callek)]

Pushed:

Configs: http://hg.mozilla.org/build/buildbot-configs/rev/9a07b92395ec
         http://hg.mozilla.org/build/buildbot-configs/rev/32e44e7ebb3a
Custom:  http://hg.mozilla.org/build/buildbotcustom/rev/34e0682cf4a8
           (with my nits applied)

Did a reconfig!

Comment 13

[Justin Wood (:Callek)]

Booo :(

python: can't open file '/e/builds/slave/c-cen-t-w32/build/build/pymake/make.py': [Errno 2] No such file or directory


Lets try to fix this.

Comment 14

[Justin Wood (:Callek)]

Attachment: [custom] pwd -W

Landing this now...

Comment 15

[Justin Wood (:Callek)]

Attachment: [custom] pwd -W (real patch)

Comment 16

[Justin Wood (:Callek)]

Attachment: [custom] force py2.7

Yet another needed fix -- pymake needs py2.6 or greater, buildbotve on these systems have py2.5

We can't swap the PATH entry since we need a py with win32api for some buildbot-code that uses python. So this makes us force py2.7.

Pushing and reconfig again.

Comment 17

[Justin Wood (:Callek)]

Attachment: [tools] extend the hack in tools retry.py

Sooo, while we got a green build, it wasn't able to upload.

This is due to retry.py failing. After a bit of manual expirimentation on the host I found out retry.py does magic (hacky) on windows to find the "real" exe, but skips that magic when it sees a . in the arg0. So to work around that I add another hack here to make it do that still for py2.7

Comment 18

[Edmund Wong (:ewong)]

Attachment: makeCMD -> makeCmd

Comment 19

[Edmund Wong (:ewong)]

Attachment: Make package-compare step requires modified env.

Comment 20

[Justin Wood (:Callek)]

Comment on attachment 720404 [details] [diff] [review]
Make package-compare step requires modified env.

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

This should indeed fix this one

Comment 21

[Justin Wood (:Callek)]

Attachment: [custom] l10n needs a slew of self.env

So l10n failed due to not finding py2.7, this should fix that up.

Comment 22

[Justin Wood (:Callek)]

Attachment: [custom] last for this bug - disable pymake for l10n

I guess we might as well wait for the MoCo bug that enables pymake on l10n, then finish porting that before turning it on here, we are broken in a few difficult ways right now

Comment 23

[Edmund Wong (:ewong)]

Attachment: Revert locale_merge changes. (v1)

Comment 24

[Justin Wood (:Callek)]

Comment on attachment 723303 [details] [diff] [review]
Revert locale_merge changes. (v1)

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

r+ if you fix the first hunk :-)

::: process/factory.py
@@ +3458,5 @@
>              prettyEnv['WIN32_INSTALLER_IN'] = WithProperties('%(win32_installer_in)s')
>          self.addStep(ShellCommand(
>           name='repack_installers_pretty',
>           description=['repack', 'installers', 'pretty'],
> +         command=WithProperties('make installers-%(locale)s LOCALE_MERGEDIR=$PWD/merged')],

you missed the sh -c part here

Comment 25

[Edmund Wong (:ewong)]

Attachment: Revert locale_merge changes. (v2)

Comment 26

[Edmund Wong (:ewong)]

Attachment: Fixed the remaining locale_merge