Closed Bug 842445 Opened 8 years ago Closed 8 years ago

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

Categories

(SeaMonkey :: Release Engineering, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ewong, Assigned: ewong)

References

Details

Attachments

(12 files, 5 obsolete files)

10.14 KB, patch
Callek
: review+
Details | Diff | Splinter Review
56.25 KB, patch
Callek
: review+
Details | Diff | Splinter Review
702 bytes, patch
ewong
: review+
Details | Diff | Splinter Review
1.25 KB, patch
ewong
: review+
Details | Diff | Splinter Review
1.07 KB, patch
ewong
: review+
Details | Diff | Splinter Review
818 bytes, patch
ewong
: review+
Details | Diff | Splinter Review
1017 bytes, patch
Callek
: review+
Details | Diff | Splinter Review
908 bytes, patch
Callek
: review+
Details | Diff | Splinter Review
4.58 KB, patch
ewong
: review+
Details | Diff | Splinter Review
1.33 KB, patch
ewong
: review+
Details | Diff | Splinter Review
2.11 KB, patch
Callek
: review+
Details | Diff | Splinter Review
1.91 KB, patch
Callek
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #715378 - Flags: review?(bugspam.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)
Attachment #715378 - Flags: review?(bugspam.Callek) → review-
Attachment #715378 - Attachment is obsolete: true
Attachment #715407 - Flags: review?(bugspam.Callek)
SeaMonkey-Aurora builds are now broken as well after yesterday's merge...  :-(
Blocks: 828317
Attachment #715407 - Attachment is obsolete: true
Attachment #715407 - Flags: review?(bugspam.Callek)
Attachment #717621 - Flags: review?(bugspam.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 ?
Attachment #717621 - Attachment is obsolete: true
Attachment #717621 - Flags: review?(bugspam.Callek)
Attachment #717700 - Flags: review?(bugspam.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'],
Attachment #717700 - Flags: review?(bugspam.Callek) → review+
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
Attachment #715379 - Flags: review?(bugspam.Callek) → review+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file [custom] pwd -W (obsolete) —
Landing this now...
Attachment #720335 - Flags: review?(ewong)
Attachment #720335 - Attachment is obsolete: true
Attachment #720335 - Attachment is patch: false
Attachment #720335 - Flags: review?(ewong)
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.
Attachment #720344 - Flags: review?(ewong)
Attachment #720332 - Flags: review?(ewong) → review+
Attachment #720336 - Flags: review?(ewong) → review+
Attachment #720344 - Flags: review?(ewong) → review+
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
Attachment #720357 - Flags: review?(ewong)
Attachment #720357 - Flags: review?(ewong) → review+
Attachment #720369 - Flags: review?(bugspam.Callek)
Attachment #720404 - Flags: review?(bugspam.Callek)
Status: REOPENED → ASSIGNED
Attachment #720369 - Flags: review?(bugspam.Callek) → review+
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
Attachment #720404 - Flags: review?(bugspam.Callek) → review+
So l10n failed due to not finding py2.7, this should fix that up.
Attachment #720432 - Flags: review?(ewong)
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
Attachment #720449 - Flags: review?(ewong)
Attachment #720432 - Flags: review?(ewong) → review+
Attachment #720449 - Flags: review?(ewong) → review+
Attachment #723303 - Flags: review?(bugspam.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
Attachment #723303 - Flags: review?(bugspam.Callek) → review+
Attachment #723303 - Attachment is obsolete: true
Attachment #723306 - Flags: review?(bugspam.Callek)
Attachment #723306 - Flags: review?(bugspam.Callek) → review+
Attachment #723334 - Flags: review?(bugspam.Callek)
Attachment #723334 - Flags: review?(bugspam.Callek) → review+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.