refactor mobile builds to use config.py + generateBranchObjects

RESOLVED FIXED

Status

defect
P2
normal
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: aki, Assigned: jhford)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mobile])

Attachments

(6 attachments, 10 obsolete attachments)

15.48 KB, patch
aki
: review+
bhearsum
: review+
Details | Diff | Splinter Review
69.17 KB, patch
jhford
: checked-in+
Details | Diff | Splinter Review
11.91 KB, patch
aki
: review+
Details | Diff | Splinter Review
967 bytes, patch
catlee
: review+
bear
: feedback+
Details | Diff | Splinter Review
893 bytes, patch
catlee
: review+
bear
: feedback+
Details | Diff | Splinter Review
6.38 KB, patch
aki
: review+
Details | Diff | Splinter Review
Reporter

Description

10 years ago
This will allow us to setup/split/configure the mobile builds like we do the desktop builds.  Q4 goal.
Reporter

Updated

10 years ago
Blocks: 492540
Reporter

Comment 1

10 years ago
This is a good/nice to have as far as having mobile configs with the rest of the builds is concerned.

However, as pm* are currently configured, this would move mobile-trunk and mobile-1.9.2 builds to pm01, which is probably of questionable benefit at best, and would most likely bring the entire thing down.

This is an internal-only goal.  Bumping to concentrate on Fennec 1.0 tasks, but keeping this in mind.  We may end up reconfiguring how we have pm* set up in between.
Component: Release Engineering → Release Engineering: Future
Reporter

Updated

10 years ago
Assignee: aki → nobody
Mass move of bugs from Release Engineering:Future -> Release Engineering. See
http://coop.deadsquid.com/2010/02/kiss-the-future-goodbye/ for more details.
Component: Release Engineering: Future → Release Engineering
Priority: -- → P3
Starting to work on this.
Assignee: nobody → jhford
Depends on: 560224
Priority: P3 → P2
setting up my own master on mobile-sandbox. did the following

#edit .bash_profile to remove PYTHON* variables
wget http://python.org/ftp/python/2.6.5/Python-2.6.5.tar.bz2
cd Python-2.6.5
./configure --prefix=/usr/local
make -j4
su -c 'make altinstall'

wget http://pypi.python.org/packages/source/v/virtualenv/virtualenv-1.4.8.tar.gz#md5=74ded4025a56e538c1c8df6b9825a8b8
tar zxf virtualenv-1.4.8.tar.gz
cd virtualenv-1.4.8
su -c 'python2.6 setup.py install'

cd /builds/buildbot
hg clone http://hg.mozilla.org/users/catlee_mozilla.com/buildbot
hg clone http://hg.mozilla.org/build/buildbotcustom
hg clone http://hg.mozilla.org/build/buildbot-configs
hg up -C -r buildbot-0.8.0 -R buildbotcustom
hg up -C -r buildbot-0.8.0 -R buildbot-configs

virtualenv -p python2.6 --no-site-packages virt-env
pushd virt-env/lib/python2.6/site-packages
ln -s ../../../../buildbotcustom
popd

source virt-env/bin/activate

wget http://tmrc.mit.edu/mirror/twisted/Twisted/10.0/Twisted-10.0.0.tar.bz2#md5=3b226af1a19b25e3b3e93cc6edf5e284
tar jxf Twisted-10.0.0.tar.bz2
cd Twisted-10.0.0
python setup.py install
cd ..

cd buildbot
python setup.py install
cd ..

easy_install pycrypto
easy_install pyopenssl
easy_install pyasn1

cd buildbot-configs
./setup-master.py schedule-master mozilla 1
./setup-master.py build-master mozilla 2

not sure how to link them together db-wise.  Asking in IRC
trying to use BBDB_URL="sqlite:////builds/buildbot/state.sqlite" as suggested by catlee in irc
Ok, they seem to both be accessing the same sqlite database with that BBDB_URL

$ lsof | grep sqlite
buildbot  16141    cltbld  mem       REG        8,1   189465     820898 /usr/local/lib/python2.6/lib-dynload/_sqlite3.so
buildbot  16141    cltbld  mem       REG        8,1   385180     785754 /usr/lib/libsqlite3.so.0.8.6
buildbot  16141    cltbld    8u      REG       8,17    47104     149474 /builds/buildbot/state.sqlite
buildbot  16141    cltbld   10u      REG       8,17    47104     149474 /builds/buildbot/state.sqlite
buildbot  16141    cltbld   19u      REG       8,17    47104     149474 /builds/buildbot/state.sqlite
buildbot  16141    cltbld   22u      REG       8,17    47104     149474 /builds/buildbot/state.sqlite
buildbot  16172    cltbld  mem       REG        8,1   189465     820898 /usr/local/lib/python2.6/lib-dynload/_sqlite3.so
buildbot  16172    cltbld  mem       REG        8,1   385180     785754 /usr/lib/libsqlite3.so.0.8.6
buildbot  16172    cltbld   10u      REG       8,17    47104     149474 /builds/buildbot/state.sqlite
buildbot  16172    cltbld   11u      REG       8,17    47104     149474 /builds/buildbot/state.sqlite
buildbot  16172    cltbld   12u      REG       8,17    47104     149474 /builds/buildbot/state.sqlite
buildbot  16172    cltbld   13u      REG       8,17    47104     149474 /builds/buildbot/state.sqlite
$ ps -ef | grep buildbot
cltbld   16141     1 17 11:18 ?        00:00:21 /builds/buildbot/virt-env/bin/python /builds/buildbot/virt-env/bin/buildbot start .
cltbld   16172     1  0 11:19 ?        00:00:00 /builds/buildbot/virt-env/bin/python /builds/buildbot/virt-env/bin/buildbot start .
root     16268 16221  0 11:20 pts/0    00:00:00 grep buildbot
I have also confirmed that a job from our scheduler master is picked up on our build master in this configuration
Reporter

Updated

9 years ago
Whiteboard: [mobile]
Duplicate of this bug: 568570
Posted patch buildbot-configs v2 (obsolete) — Splinter Review
Notes:
-does not support try server's mail notifier
-missing no-merge builders
-missing l10n
-prioritization (builder's nextSlave key unset) -- don't know how
-has duplicate schedulers and change sources
-ready to start using bindmount for scratchbox


To Do:
-make mobile build factories understand that now baseWorkDir should be in the argv for subcommands on maemo not a full path.
  -i.e. do something like
         cmd_prefix=['%s/%s' % self.scratchbox_base', self.scratchbox_cmd')),
                                   '-p', '-k', '-d']
         in the factory __init__ then in the step __init__ do
         command=cmd_prefix + ['%s/%s' % (self.mount_point, self.base_workdir), 'pwd'],
-Get rid of MaemoBuildFactory - make using scratchbox a factory option
-fix maemo env{}'s
-schedulers for other branches on mobile-browser changes
  -i.e. a mobile-browser change should trigger a mozilla-central, mozilla-1.9.2, etc change
ns?
-be able to do a single mozilla repository on two mobile-browser repositories
-port mobile_config settings to new configs
-get ready for try
-call generateMobileBranchObjects from generateBranchObjects, not doing yet as this makes testing a lot simpler
Posted patch buildbotcustom v2 (obsolete) — Splinter Review
Forgot to mention

$ hg ident -R buildbot-configs/
4971ce1423ad+ (buildbot-0.8.0)
$ hg parent -R buildbot-configs/ --template={node}
4971ce1423ad7c276125f396d3cd9aae379672d6

$ hg ident -R buildbotcustom/
8f6a65260a29+ (buildbot-0.8.0)
$ hg parent -R buildbotcustom --template={node}
8f6a65260a298036c66a00f8d678ce9be74f1735
also, there is a hack right now that makes every branch use the mozilla-central mozconfig, that will be fixed in a later patch
Status: NEW → ASSIGNED
Posted patch buildbot-configs v3 (obsolete) — Splinter Review
\o/
==================================
-added a ScratchboxCommand class that makes scratchbox as invisible as possible
-created a new Maemo build factory
  -running tests on it now
-this patch should be ready for review soon

/o\
==================================
-needs try and l10n support
-need to fix mozconfigs
-need to fix environments
-need to port over production mobile data
Attachment #450271 - Attachment is obsolete: true
Posted patch buildbotcustom v3 (obsolete) — Splinter Review
Attachment #450272 - Attachment is obsolete: true
oh, the commented out generateBranchObjects calls are to make testing just mobile stuff easier.

/me goes to bed.
Comment on attachment 450335 [details] [diff] [review]
buildbotcustom v3

>+class ScratchboxCommand(ShellCommand):
>+
>+    def __init__(self,
>+                 workdir_xform=lambda x: x,
>+                 sbox_base='/builds/scratchbox',
>+                 sbox_args=['-k', '-p'],
>+                 sbox_command='login',
>+                 **kwargs):
>+        assert type(kwargs['command']) is str, 'Scratchbox only understands commands as strings'
>+        self.sbox_base = sbox_base
>+        self.sbox_args = sbox_args
>+        self.sbox_command = sbox_command
>+        self.workdir_xform = workdir_xform
>+
>+        ShellCommand.__init__(self, **kwargs)
>+        self.addFactoryArguments(sbox_command=self.sbox_command,
>+                                 sbox_base=self.sbox_base,
>+                                 sbox_args=sbox_args,
>+                                 workdir_xform=workdir_xform,
>+                                )
>+
>+    def start(self):
>+        properties = self.build.getProperties()
>+        warnings = []
>+        kwargs = properties.render(self.remote_kwargs)
>+        original_command = properties.render(self.command)
>+        kwargs['logfiles'] = self.logfiles
>+        if kwargs.has_key('usePTY') and kwargs['usePTY'] != 'slave-config':
>+            if self.slaveVersionIsOlderThan("svn", "2.7"):
>+                warnings.append("NOTE: slave does not allow master to override usePTY\n")
>+        kwargs['command'] = ['%s/%s'% (self.sbox_base, self.sbox_command)] \
>+                                + self.sbox_args + \
>+                            ['-d', self.workdir_xform(kwargs['workdir']), original_command]
>+
>+        cmd = RemoteShellCommand(**kwargs)
>+        self.setupEnvironment(cmd)
>+        self.checkForOldSlaveAndLogfiles()
>+
>+        self.startCommand(cmd, warnings)
>+
>+

Just on a drive-by here, why do you need to override start()? You should just be able to build up command in __init__ and pass that to ShellCommand's constructor.
(In reply to comment #15)
> (From update of attachment 450335 [details] [diff] [review])
> >+class ScratchboxCommand(ShellCommand):
> >+
> >+    def __init__(self,
> >+                 workdir_xform=lambda x: x,
> >+                 sbox_base='/builds/scratchbox',
> >+                 sbox_args=['-k', '-p'],
> >+                 sbox_command='login',
> >+                 **kwargs):
> >+        assert type(kwargs['command']) is str, 'Scratchbox only understands commands as strings'
> >+        self.sbox_base = sbox_base
> >+        self.sbox_args = sbox_args
> >+        self.sbox_command = sbox_command
> >+        self.workdir_xform = workdir_xform
> >+
> >+        ShellCommand.__init__(self, **kwargs)
> >+        self.addFactoryArguments(sbox_command=self.sbox_command,
> >+                                 sbox_base=self.sbox_base,
> >+                                 sbox_args=sbox_args,
> >+                                 workdir_xform=workdir_xform,
> >+                                )
> >+
> >+    def start(self):
> >+        properties = self.build.getProperties()
> >+        warnings = []
> >+        kwargs = properties.render(self.remote_kwargs)
> >+        original_command = properties.render(self.command)
> >+        kwargs['logfiles'] = self.logfiles
> >+        if kwargs.has_key('usePTY') and kwargs['usePTY'] != 'slave-config':
> >+            if self.slaveVersionIsOlderThan("svn", "2.7"):
> >+                warnings.append("NOTE: slave does not allow master to override usePTY\n")
> >+        kwargs['command'] = ['%s/%s'% (self.sbox_base, self.sbox_command)] \
> >+                                + self.sbox_args + \
> >+                            ['-d', self.workdir_xform(kwargs['workdir']), original_command]
> >+
> >+        cmd = RemoteShellCommand(**kwargs)
> >+        self.setupEnvironment(cmd)
> >+        self.checkForOldSlaveAndLogfiles()
> >+
> >+        self.startCommand(cmd, warnings)
> >+
> >+
> 
> Just on a drive-by here, why do you need to override start()? You should just
> be able to build up command in __init__ and pass that to ShellCommand's
> constructor.

i tried doing that and what resulted was:

argv == ['/builds/scratchbox/moz-scratchbox', '-d', '/builds/slave/.../build', ['/builds/scratchbox/moz-scratchbox', '-d', '/builds/slave/.../build', 
echo]].
(In reply to comment #16)
> > Just on a drive-by here, why do you need to override start()? You should just
> > be able to build up command in __init__ and pass that to ShellCommand's
> > constructor.
> 
> i tried doing that and what resulted was:
> 
> argv == ['/builds/scratchbox/moz-scratchbox', '-d', '/builds/slave/.../build',
> ['/builds/scratchbox/moz-scratchbox', '-d', '/builds/slave/.../build', 
> echo]].

Any chance you've got that code around still?
no, i don't, but it was something along the lines of:

class:
  def __init__(self, command='pwd', workdir='.', **kwargs):
    command = ['/.../mozscratchbox', '-d', workdir, command]
    ShellCommand.__init__(self, command=command, workdir=workdir)
Comment on attachment 450335 [details] [diff] [review]
buildbotcustom v3

I would greatly prefer the ScratchboxCommand work and generateBranchObjects work to be separated. The ScratchboxCommand stuff belongs in a different bug IMHO.

I think you've missed part of the original point of this bug, too. I've always thought of this as having one function, which is based a branch's configuration, that returns *all* of the branches objects. Even if it's just having generateBranchObjects call the function you've already written, and combine the objects, I think it's a big improvement.

One specific thing:
+def generateMobileBranchObjects(config, name, branches=[],
+             other_branch_builders=[]):

Don't use lists as defaults unless you copy them immediately. Generally, we use None as the default in cases like this, eg: http://hg.mozilla.org/build/buildbotcustom/file/tip/process/factory.py#l904


I only skimmed over factory.py but it seemed O.K. Other than the above, I don't have an issue with anything high level. I reserve the right to nit in a real review, though ;).
Posted patch buildbot-configs (obsolete) — Splinter Review
This patch is ready to land for production.  It will require that the localconfig files have the appropriate tinderbox tree set for their mobile branches.

Bear, can you please look at this patch and tell me if the configuration settings are correct as I cannot complete a test build on android.
Attachment #450333 - Attachment is obsolete: true
Attachment #451417 - Flags: review?(bhearsum)
Attachment #451417 - Flags: review?(aki)
Attachment #451417 - Flags: feedback?(bear)
Posted patch buildbotcustom (obsolete) — Splinter Review
This is the accompanying patch.

I just realized that I did not remove things replaced by this config change from mobile_config.py .  That is a fairly small change but I can attach either a new buildbot-configs patch or a supplementary patch
Attachment #450335 - Attachment is obsolete: true
Attachment #451418 - Flags: review?(bhearsum)
Attachment #451418 - Flags: review?(aki)

Updated

9 years ago
Attachment #451417 - Flags: feedback?(bear) → feedback+
Reporter

Comment 22

9 years ago
Comment on attachment 451417 [details] [diff] [review]
buildbot-configs

>+    'mobile_tinderbox_tree': 'MobileTest', #XXX Remove this line

Looks like this needs changing, or at least overriding below? Or do you want to bring these live and report elsewhere?

If we're reporting elsewhere, are we uploading elsewhere as well?

>+    'mobile_platforms': {
>+        #Platforms disabled for development, move *-i686 mozconfigs to *
>+        'maemo4': {},
>+        'maemo5-gtk': {},
>+        'maemo5-qt': {},
>+        'android-r7': {},
>+        'linux': {},
>+        'win32': {},
>+        'macosx': {},
>+    },

I'm not sure about this.
I do see the benefit of not having to munge the -i686 out for it to become
a valid platform name.
However, I do find it useful to specify that this is a not-official platform
for the mobile build, especially in release builds.

>+    'maemo4':{
>+        'base_name': 'Maemo 4 %(branch)s %(mobile_branch)s',

I approve of putting the "4" in the name.
This will require some scraping tweaks + tbpl coordination.

>+        'package_globlist': ['dist/*.tar.bz2', 'dist/*.zip'],

No debs?  We also require deb_name.txt for Maemo, or all repacks will bork.

>+    'maemo5-qt':{
<snip>
>+        'debs': False,

Turn 'em on! :)

>+                      'mobile_platforms': {},
>+                      #'mobile_platforms': {'maemo4': {}, 'maemo5-gtk': {},
>+                      #                     'maemo5-qt': {}, 'linux': {},
>+                      #                     'win32': {}, 'macosx': {},
>+                      #                    },
>                      },
>     'tracemonkey': {},
>-    'places': {},
>+    'places': {
>+                'mobile_platforms': {},
>+    },
>     'electrolysis': {},
>-    'addonsmgr': {},
>+    'addonsmgr': {
>+                'mobile_platforms': {},
>+    },
> }

Hm, so all project branches except places and addonsmgr? Effectively turning it on for tracemonkey and electrolysis. Ok.

I bet other people will want you to remove the commented-out section above.

>+#BRANCHES['mozilla-central']['mobile_platforms']['maemo4']['env']['MOZ_SYMBOLS_EXTRA_BUILDID'] = 'mozilla-central-maemo4'
>+#BRANCHES['mozilla-central']['mobile_platforms']['maemo5-gtk']['env']['MOZ_SYMBOLS_EXTRA_BUILDID'] = 'mozilla-central-maemo5-gtk'
>+#BRANCHES['mozilla-central']['mobile_platforms']['maemo5-qt']['env']['MOZ_SYMBOLS_EXTRA_BUILDID'] = 'mozilla-central-maemo5-qt'
>+#BRANCHES['mozilla-central']['mobile_platforms']['linux']['env']['MOZ_SYMBOLS_EXTRA_BUILDID'] = 'mozilla-central'
>+#BRANCHES['mozilla-central']['mobile_platforms']['win32']['env']['MOZ_SYMBOLS_EXTRA_BUILDID'] = 'mozilla-central'
>+#BRANCHES['mozilla-central']['mobile_platforms']['macosx']['env']['MOZ_SYMBOLS_EXTRA_BUILDID'] = 'mozilla-central'

Are you just commenting these out until builds are enabled on m-c?
Reporter

Comment 23

9 years ago
Comment on attachment 451418 [details] [diff] [review]
buildbotcustom

+    for platform in config.get('mobile_platforms', {}).keys():
+        render = {'branch': name,
+                  'mobile_branch': mobile_repo_name,
+                  'platform': platform,
+        }
+        pf=config['mobile_platforms'][platform]
+        base_name = pf.get('base_name') % render

I don't think you use render['platform'] anywhere.  Could be useful in
the future though.

+        factory_kwargs={
<snip>
+            'baseUploadDir': '%s-%s' % (name, mobile_repo_name),

Hmmmmm.
[latest-]mozilla-central-mobile-browser ?
I think I'd much rather
 'baseUploadDir': pf.get('base_upload_dir', [latest-]mozilla-central-%(platform)s )

Do you disagree?

a) putting the moz branch in the upload dir name, and b) putting the platform in the upload dir name, have been wants of mine for a while now.

Not sure if you're appending the platform later at this point. But my vote would be to remove the -mobile_repo_name from here.

c) this will require blog posts and newsgroup posts to notify everyone that binaries are changing upload locations.  When we update m-c and m-1.9.2 we'll have to update the deb repo scripts.

+                'locales': pf.get('locales', config.get('locales')),

Locales needs to be data driven at runtime, not reconfig time, hence the
current ugliness.  Not Your Problem at this point.

+        if revision:
+            rev_list = ['--rev', revision]
+        else:
+            rev_list = []
         self.addStep(ShellCommand,
             name='hg_update',
-            command=['hg', 'update', '-C', '-r', revision],
+            command=['hg', 'update', '-C'] + rev_list,

Hm, do you want to do this, or set revision to "default" by default?


One thing that I noticed is that we're regressing bug 548054 here.
Could you add a branch-specific email-on-breakage setting?  It won't matter
until m-c and m-1.9.2 kick over, but it'll regress at that point.


Other than that, these patches do look pretty readable and cool, assuming they work :)
(In reply to comment #22)
> (From update of attachment 451417 [details] [diff] [review])
> >+    'mobile_tinderbox_tree': 'MobileTest', #XXX Remove this line
> Looks like this needs changing, or at least overriding below? Or do you want to
> bring these live and report elsewhere?
> 

It is a safe fallback, but should be removed to make an attempt to configure a production mobile branch reporting to mobile-test fail.  I don't have a strong prefernce, but i think removing it is better.  If there are no objections I will remove this in the next patch

> If we're reporting elsewhere, are we uploading elsewhere as well?
> 
> >+        'linux': {},
> >+        'win32': {},
> >+        'macosx': {},
> I'm not sure about this.
> I do see the benefit of not having to munge the -i686 out for it to become
> a valid platform name.
> However, I do find it useful to specify that this is a not-official platform
> for the mobile build, especially in release builds.

If we are going to add anything, i'd vote for linux-mobile as desktop linux is also on i686.

> 
> >+    'maemo4':{
> >+        'base_name': 'Maemo 4 %(branch)s %(mobile_branch)s',
> 
> I approve of putting the "4" in the name.
> This will require some scraping tweaks + tbpl coordination.
 
Yep, we can file bugs for this :)

> >+        'package_globlist': ['dist/*.tar.bz2', 'dist/*.zip'],
> 
> No debs?  We also require deb_name.txt for Maemo, or all repacks will bork.
 
will add in.

> >+    'maemo5-qt':{
> <snip>
> >+        'debs': False,
> 
> Turn 'em on! :)

agreed

> >+                      'mobile_platforms': {},
> >+                      #'mobile_platforms': {'maemo4': {}, 'maemo5-gtk': {},
> >+                      #                     'maemo5-qt': {}, 'linux': {},
> >+                      #                     'win32': {}, 'macosx': {},
> >+                      #                    },
> >                      },
> >     'tracemonkey': {},
> >-    'places': {},
> >+    'places': {
> >+                'mobile_platforms': {},
> >+    },
> >     'electrolysis': {},
> >-    'addonsmgr': {},
> >+    'addonsmgr': {
> >+                'mobile_platforms': {},
> >+    },
> > }
> 
> Hm, so all project branches except places and addonsmgr? Effectively turning it
> on for tracemonkey and electrolysis. Ok.
> 
> I bet other people will want you to remove the commented-out section above.
> 
> >+#BRANCHES['mozilla-central']['mobile_platforms']['maemo4']['env']['MOZ_SYMBOLS_EXTRA_BUILDID'] = 'mozilla-central-maemo4'
> >+#BRANCHES['mozilla-central']['mobile_platforms']['maemo5-gtk']['env']['MOZ_SYMBOLS_EXTRA_BUILDID'] = 'mozilla-central-maemo5-gtk'
> >+#BRANCHES['mozilla-central']['mobile_platforms']['maemo5-qt']['env']['MOZ_SYMBOLS_EXTRA_BUILDID'] = 'mozilla-central-maemo5-qt'
> >+#BRANCHES['mozilla-central']['mobile_platforms']['linux']['env']['MOZ_SYMBOLS_EXTRA_BUILDID'] = 'mozilla-central'
> >+#BRANCHES['mozilla-central']['mobile_platforms']['win32']['env']['MOZ_SYMBOLS_EXTRA_BUILDID'] = 'mozilla-central'
> >+#BRANCHES['mozilla-central']['mobile_platforms']['macosx']['env']['MOZ_SYMBOLS_EXTRA_BUILDID'] = 'mozilla-central'
> 
> Are you just commenting these out until builds are enabled on m-c?

Yes, I would really prefer to leave this code there and commented out.

(In reply to comment #23)
> (From update of attachment 451418 [details] [diff] [review])
> +    for platform in config.get('mobile_platforms', {}).keys():
> +        render = {'branch': name,
> +                  'mobile_branch': mobile_repo_name,
> +                  'platform': platform,
> +        }
> +        pf=config['mobile_platforms'][platform]
> +        base_name = pf.get('base_name') % render
> 
> I don't think you use render['platform'] anywhere.  Could be useful in
> the future though.

that was because I was intending to have the mozconfig have the platform in it.  The reason that isn't in this patch is because I didn't want to have to rename the mozconfigs.

> 
> +        factory_kwargs={
> <snip>
> +            'baseUploadDir': '%s-%s' % (name, mobile_repo_name),
> 
> Hmmmmm.
> [latest-]mozilla-central-mobile-browser ?
> I think I'd much rather
>  'baseUploadDir': pf.get('base_upload_dir',
> [latest-]mozilla-central-%(platform)s )
> 
> Do you disagree?
> 
> a) putting the moz branch in the upload dir name, and b) putting the platform
> in the upload dir name, have been wants of mine for a while now.
> 
> Not sure if you're appending the platform later at this point. But my vote
> would be to remove the -mobile_repo_name from here.
> 
> c) this will require blog posts and newsgroup posts to notify everyone that
> binaries are changing upload locations.  When we update m-c and m-1.9.2 we'll
> have to update the deb repo scripts.

I would personally prefer to have both branches in the directory name as it is 100% clear which exact repositories it was built with at a moments glance.  I would support adding the platform into it.  If it is a significant roadblock to landing I am fine with leaving it out.  Lets ask what the mobile team's preference is as it is easy to change on our side.

> 
> +                'locales': pf.get('locales', config.get('locales')),
> 
> Locales needs to be data driven at runtime, not reconfig time, hence the
> current ugliness.  Not Your Problem at this point.

Should I remove this?

> 
> +        if revision:
> +            rev_list = ['--rev', revision]
> +        else:
> +            rev_list = []
>          self.addStep(ShellCommand,
>              name='hg_update',
> -            command=['hg', 'update', '-C', '-r', revision],
> +            command=['hg', 'update', '-C'] + rev_list,
> 
> Hm, do you want to do this, or set revision to "default" by default?

I have no strong feelings one way or the other.  The only case that I can think of that makes me not want to do this is in-repo branches.  We really should clobber if we are changing branches between runs though.

> One thing that I noticed is that we're regressing bug 548054 here.
> Could you add a branch-specific email-on-breakage setting?  It won't matter
> until m-c and m-1.9.2 kick over, but it'll regress at that point.

I can add that
 
> Other than that, these patches do look pretty readable and cool, assuming they
> work :)

cool :D
Based on Aki's comment it sounds like there is a lot of behaviour changes in these patches. If you the two of you are OK with that I won't object, but if we have to back this out for any reason it's going to suck to have those as ride-alongs.
If we change the latest-* directories, we need to symlink on ftp
Also, it was decided that [latest-]mozbranch-platform would be the directory naming (e.g. latest-mozilla-central-maemo5-gtk)
Posted patch Buildbot-configs (obsolete) — Splinter Review
This patch addresses Aki's concerns.  The Tinderbox trees are set properly.  I have changed the mobile desktop build platforms from 'platform' to 'platform-mobile', though the mozconfigs will still look in 'platform-i686' to minimize the scope of this patch.  Globlists for maemo now have the debs and deb_name.txt files included. QT debs are turned on and will cause builds to be red until uploading debs works.

-PM02 will no longer do any android
-PM02 will no longer do any project branches
-buildbot-080 will do all android builds
-buildbot-080 will do all project branches
-mobile desktop dep builds no longer clobber
-android deb builds no longer clobber
Attachment #451417 - Attachment is obsolete: true
Attachment #451685 - Flags: review?(bhearsum)
Attachment #451685 - Flags: review?(aki)
Attachment #451685 - Flags: feedback?(bear)
Attachment #451417 - Flags: review?(bhearsum)
Attachment #451417 - Flags: review?(aki)
Posted patch Buildbotcustom (obsolete) — Splinter Review
This also has what should be needed in buildbotcustom for try server to work.
Attachment #451418 - Attachment is obsolete: true
Attachment #451686 - Flags: review?(bhearsum)
Attachment #451686 - Flags: review?(aki)
Attachment #451418 - Flags: review?(bhearsum)
Attachment #451418 - Flags: review?(aki)

Updated

9 years ago
Attachment #451685 - Flags: feedback?(bear) → feedback+
Reporter

Comment 30

9 years ago
Comment on attachment 451685 [details] [diff] [review]
Buildbot-configs

>+    'mobile_objdir': 'obj-fennec',

We need to figure out if this is used for anything -- we have 'objdir' hardcoded in the mozconfigs currently.  Searching in this patch, though, I don't see 'platform_objdir' used anywhere, so it's probably ok outside of having extraneous config settings.

>+        'linux-mobile': {},
>+        'win32-mobile': {},
>+        'macosx-mobile': {},

Hm.
linux-mobile makes sense for making sure we differentiate from linux Firefox desktop, but these are Fennec desktop builds, specifically not for mobile devices when you look at it from that angle.

Are these names internal only, or do they get exposed somewhere?
What's your rationale for changing these?

I could see the need to differentiate between our current Fennec linux desktop builds and future potential Fennec linux builds on an ARM chip, so we should probably have something intel-specific here. -intel (or -intal32), -x86, or keeping -i686 make sense to me.

>+        'package_globlist': ['dist/*.apk'],

Pretty sure this will break; Bear is using embedding/android/*.apk until 'make package' is written for Android builds.

>+        'base_name': 'Linux Mobile Desktop %(branch)s %(mobile_branch)s',

Let's keep this simple and take mobile_branch outta the names, to match the upload dir.

>+    #empty mobile_platforms for central and 192 is temporary until l10n is fixed
>+    'mozilla-central': { 'mobile_platforms': {
>+                                      'android-r7': {},
>+                                             },
>+                       },

Odd indenting here.

>+#BRANCHES['mozilla-central']['mobile_platforms']['android-r7']['env']['MOZ_SYMBOLS_EXTRA_BUILDID'] = 'mozilla-central'

This should probably be uncommented, but we don't have buildsymbols for Android yet.

>+++ b/mozilla2/mobile_config.py

It's good that we have a patch for these.
This will remove them from the buildbot-0.8.0 branch of buildbot-configs only, correct?
We will need to merge this portion of the patch into default at some point, but since our testing hasn't had a lot of fully green builds (aiui), I'd like to see builds go green before we disable the currently-working builds.

Also, with this patch we're turning on Android builds on project branches, correct?  This may burn build cycles and require builds to be hidden on tinderbox waterfalls.
Attachment #451685 - Flags: review?(aki) → review-
Reporter

Comment 31

9 years ago
(In reply to comment #24)
> > One thing that I noticed is that we're regressing bug 548054 here.
> > Could you add a branch-specific email-on-breakage setting?  It won't matter
> > until m-c and m-1.9.2 kick over, but it'll regress at that point.
> 
> I can add that

Missing afaict :)
Going to guess this will be an branch-level email_on_failure (or something)
(In reply to comment #30)
> (From update of attachment 451685 [details] [diff] [review])
> >+    'mobile_objdir': 'obj-fennec',
> 
> We need to figure out if this is used for anything -- we have 'objdir'
> hardcoded in the mozconfigs currently.  Searching in this patch, though, I
> don't see 'platform_objdir' used anywhere, so it's probably ok outside of
> having extraneous config settings.

it is an extraneous config setting as far as i can tell.
 
> >+        'linux-mobile': {},
> >+        'win32-mobile': {},
> >+        'macosx-mobile': {},
> 
> Hm.
> linux-mobile makes sense for making sure we differentiate from linux Firefox
> desktop, but these are Fennec desktop builds, specifically not for mobile
> devices when you look at it from that angle.
> 
> Are these names internal only, or do they get exposed somewhere?
> What's your rationale for changing these?
> 
> I could see the need to differentiate between our current Fennec linux desktop
> builds and future potential Fennec linux builds on an ARM chip, so we should
> probably have something intel-specific here. -intel (or -intal32), -x86, or
> keeping -i686 make sense to me.

They are external for the upload directory.  Currently, we have linux-i686 which could be Firefox or Fennec.  I don't mind adding -i686 but I would argue that just 'linux-i686' is not specific enough and will be confusing once we start dashboarding.  Most arm boards are going to require specific enough builds that I think we should just make the platform the name.  An example of this is Maemo, which we call maemo4, maemo5-gtk and maemo5-qt instead of 'linux-arm-maemo4'.  We recently moved away from 'linux-arm-gnueabi' to 'maemo4' and I think this was a good choice that I'd like to see continue.

One issue with not putting in cpu arch here would be 64bit mobile desktop builds.  As desktop builds are in support of mobile development and I don't believe there are any 64-bit mobile phones out there, i doubt we hit this issue for a *very* long time.

> 
> >+        'package_globlist': ['dist/*.apk'],
> 
> Pretty sure this will break; Bear is using embedding/android/*.apk until 'make
> package' is written for Android builds.
> 
you are correct (http://hg.mozilla.org/build/buildbot-configs/file/fb8eae47487d/mozilla2/mobile_master.py#l288).  I was using the factory defaults.


> >+        'base_name': 'Linux Mobile Desktop %(branch)s %(mobile_branch)s',
> 
> Let's keep this simple and take mobile_branch outta the names, to match the
> upload dir.

sure

> 
> >+    #empty mobile_platforms for central and 192 is temporary until l10n is fixed
> >+    'mozilla-central': { 'mobile_platforms': {
> >+                                      'android-r7': {},
> >+                                             },
> >+                       },
> 
> Odd indenting here.

yes, trying to match the indenting used elsewhere, but it looks weird with 1 key per dict.

> It's good that we have a patch for these.
> This will remove them from the buildbot-0.8.0 branch of buildbot-configs only,
> correct?

yes

> We will need to merge this portion of the patch into default at some point, but
> since our testing hasn't had a lot of fully green builds (aiui), I'd like to
> see builds go green before we disable the currently-working builds.
> 
> Also, with this patch we're turning on Android builds on project branches,
> correct?  This may burn build cycles and require builds to be hidden on
> tinderbox waterfalls.

Sure, we can do that.  Alternatively we could just keep them disabled until we are asked to enable them.

It wouldn't be difficult to transfer the mobile_config.py changes the default branch.

(In reply to comment #31)
> (In reply to comment #24)
> > > One thing that I noticed is that we're regressing bug 548054 here.
> > > Could you add a branch-specific email-on-breakage setting?  It won't matter
> > > until m-c and m-1.9.2 kick over, but it'll regress at that point.
> > 
> > I can add that
> 
> Missing afaict :)
> Going to guess this will be an branch-level email_on_failure (or something)

oops, forgot to do that.  It feels like something that we should have for all builders on that branch.  I can either add it in for this patch or do it as a follow up.  I'd prefer to make it a follow up.
Reporter

Comment 33

9 years ago
(In reply to comment #32)
> They are external for the upload directory.  Currently, we have linux-i686
> which could be Firefox or Fennec.

Except it's under pub/mobile.
If you see issues with try uploads, that may be something, but linux-mobile doesn't sit well with me since it's a non-mobile build of Fennec.

> I don't mind adding -i686 but I would argue
> that just 'linux-i686' is not specific enough and will be confusing once we
> start dashboarding.

Let's group Fennec in dashboards and/or have a product column, making this moot.

> Most arm boards are going to require specific enough
> builds that I think we should just make the platform the name.  An example of
> this is Maemo, which we call maemo4, maemo5-gtk and maemo5-qt instead of
> 'linux-arm-maemo4'.

There have been rumblings of installing vanilla ubuntu on an arm board.
My vote says linux-x86 is probably the way to go if we change it at all.

> One issue with not putting in cpu arch here would be 64bit mobile desktop
> builds.

Wait, that's a reason *to* put cpu arch in here; those would become linux-x64.


> > >+    'mozilla-central': { 'mobile_platforms': {
> > >+                                      'android-r7': {},
> > >+                                             },
> > >+                       },
> > 
> > Odd indenting here.
> 
> yes, trying to match the indenting used elsewhere, but it looks weird with 1
> key per dict.

Pretty sure the indenting elsewhere was to line up with the { after 'platforms':, but since this is 'mobile-platforms': it doesn't line up anymore.

> Sure, we can do that.  Alternatively we could just keep them disabled until we
> are asked to enable them.

Ok... but, it appears android project branch builds are *en*abled with this patch, no?

> (In reply to comment #31)
> > (In reply to comment #24)
> > > > One thing that I noticed is that we're regressing bug 548054 here.
> > > > Could you add a branch-specific email-on-breakage setting?  It won't matter
> > > > until m-c and m-1.9.2 kick over, but it'll regress at that point.
> > > 
> > > I can add that
> > 
> > Missing afaict :)
> > Going to guess this will be an branch-level email_on_failure (or something)
> 
> oops, forgot to do that.  It feels like something that we should have for all
> builders on that branch.  I can either add it in for this patch or do it as a
> follow up.  I'd prefer to make it a follow up.

All builders on that branch, except the specified bug is mobile-specific.  I suppose you can have platform-level email settings.
(In reply to comment #33)
> (In reply to comment #32)
> > They are external for the upload directory.  Currently, we have linux-i686
> > which could be Firefox or Fennec.
> 
> Except it's under pub/mobile.
> If you see issues with try uploads, that may be something, but linux-mobile
> doesn't sit well with me since it's a non-mobile build of Fennec.
> 

yes, looking at my patch for try support, it will be an issue for try server uploads.

> > Most arm boards are going to require specific enough
> > builds that I think we should just make the platform the name.  An example of
> > this is Maemo, which we call maemo4, maemo5-gtk and maemo5-qt instead of
> > 'linux-arm-maemo4'.
> 
> There have been rumblings of installing vanilla ubuntu on an arm board.
> My vote says linux-x86 is probably the way to go if we change it at all.
> 
> > One issue with not putting in cpu arch here would be 64bit mobile desktop
> > builds.
> 
> Wait, that's a reason *to* put cpu arch in here; those would become linux-x64.

Why don't we stop picking colours for our bikeshed and use %(branch)s-mobile-%(platform)s .  Both of our concerns are addressed in a way that neither is incompatible with the other.

> 
> 
> > > >+    'mozilla-central': { 'mobile_platforms': {
> > > >+                                      'android-r7': {},
> > > >+                                             },
> > > >+                       },
> > > 
> > > Odd indenting here.
> > 
> > yes, trying to match the indenting used elsewhere, but it looks weird with 1
> > key per dict.
> 
> Pretty sure the indenting elsewhere was to line up with the { after
> 'platforms':, but since this is 'mobile-platforms': it doesn't line up anymore.

are we going to block on this?  as mobile_platforms and platforms are the same level of nesting, they are indented the same amount.  I really don't care what indentation is used as long as it is functionally equivalent to what is in this patch.

> 
> > Sure, we can do that.  Alternatively we could just keep them disabled until we
> > are asked to enable them.
> 
> Ok... but, it appears android project branch builds are *en*abled with this
> patch, no?

Right, they are enabled because I cut the patch before this issue was brought up ;)

> 
> > (In reply to comment #31)
> > > (In reply to comment #24)
> > > > > One thing that I noticed is that we're regressing bug 548054 here.
> > > > > Could you add a branch-specific email-on-breakage setting?  It won't matter
> > > > > until m-c and m-1.9.2 kick over, but it'll regress at that point.
> > > > 
> > > > I can add that
> > > 
> > > Missing afaict :)
> > > Going to guess this will be an branch-level email_on_failure (or something)
> > 
> > oops, forgot to do that.  It feels like something that we should have for all
> > builders on that branch.  I can either add it in for this patch or do it as a
> > follow up.  I'd prefer to make it a follow up.
> 
> All builders on that branch, except the specified bug is mobile-specific.  I
> suppose you can have platform-level email settings.

Added -- setting is not in config.py, rather it is in production_config.py as it is only used on production.

New patch coming up in a few.
Posted patch buildbot-configs-v6 (obsolete) — Splinter Review
using platform == 'linux' instead of platform == 'linux*'

We are going to maintain the seperation of the platform 'linux' from product 'firefox/fennec' by having a seperate list of mobile_platforms and having builds upload to a different location on ftp.

I have disabled android on project branches for this patch.
Attachment #451685 - Attachment is obsolete: true
Attachment #451788 - Flags: review?(bhearsum)
Attachment #451788 - Flags: review?(aki)
Attachment #451685 - Flags: review?(bhearsum)
add the mail notifier for mobile build failures.
Attachment #451686 - Attachment is obsolete: true
Attachment #451789 - Flags: review?(bhearsum)
Attachment #451789 - Flags: review?(aki)
Attachment #451686 - Flags: review?(bhearsum)
Attachment #451686 - Flags: review?(aki)
Reporter

Comment 37

9 years ago
Comment on attachment 451789 [details] [diff] [review]
buildbotcustom-v6

The "from" should probably be set from config as well, but I'm not going to block on that.
Attachment #451789 - Flags: review?(aki) → review+
Reporter

Comment 38

9 years ago
Comment on attachment 451788 [details] [diff] [review]
buildbot-configs-v6

Good job. I think you've addressed all my concerns here.

I think mozilla-1.9.2 needs a mobile_build_failure_emails set. That won't affect anything with this patch, but we might forget it when we enable mozilla-1.9.2 mobile builds. r=me with that nit.

When this lands, we need to watch these builds carefully, as well as the scrape/tbpl fun.
Attachment #451788 - Flags: review?(aki) → review+
Comment on attachment 451789 [details] [diff] [review]
buildbotcustom-v6

I don't think I have the domain knowledge to do a line by line review, but my high level concerns have been addressed and aki is happy with the details. r+.
Attachment #451789 - Flags: review?(bhearsum) → review+
Attachment #451788 - Flags: review?(bhearsum) → review+
This is the same patch as v6 brought up to date with recent changes.  Carrying forward the 2 r+'s
Attachment #451788 - Attachment is obsolete: true
Attachment #453195 - Flags: checked-in?
Posted patch patch for PM02 (obsolete) — Splinter Review
this patch is for the default branch.  It needs to be landed and PM02 reconfiged in addition to buildbot-configs-v6 minus the bitrot.
Attachment #453208 - Attachment is patch: true
Attachment #453208 - Attachment mime type: application/octet-stream → text/plain
Posted patch pm02 patchSplinter Review
Attachment #453208 - Attachment is obsolete: true
Attachment #453214 - Flags: review?(aki)
Reporter

Comment 43

9 years ago
Comment on attachment 453214 [details] [diff] [review]
pm02 patch

r=me, with a nit:
please use |if mobile_nightly_factory is not None:| in both to be consistent.
Attachment #453214 - Flags: review?(aki) → review+
(In reply to comment #44)
> For landing:
> 1) hg -R buildbot-configs import
> http://hg.mozilla.org/users/jford_mozilla.com/buildbot-configs/rev/e50ec8efd934
> 2) hg -R buildbot-configs import
> http://hg.mozilla.org/users/jford_mozilla.com/buildbot-configs/rev/db3245ded838
  3) hg -R buildbotcustom import http://hg.mozilla.org/users/jford_mozilla.com/buildbotcustom/rev/97a27935816e
> 
> Once these configs are going green again, we need to do for pm02
> 1) hg -R buildbot-configs import
> http://hg.mozilla.org/users/jford_mozilla.com/buildbotcustom/rev/97a27935816e
> 2) hg -R buildbot-configs import
> http://hg.mozilla.org/users/jford_mozilla.com/buildbot-configs/rev/c53185b1f441
bustage fix.  Moving config setting to config.py
Attachment #453728 - Flags: review?(catlee)
Attachment #453728 - Flags: feedback?(bear)
part 2
Attachment #453729 - Flags: review?(catlee)
Attachment #453729 - Flags: feedback?(bear)

Updated

9 years ago
Attachment #453728 - Flags: feedback?(bear) → feedback+

Updated

9 years ago
Attachment #453729 - Flags: feedback?(bear) → feedback+
Comment on attachment 453728 [details] [diff] [review]
buildbot-configs follow up patch

>diff --git a/mozilla/config.py b/mozilla/config.py
>--- a/mozilla/config.py
>+++ b/mozilla/config.py
>@@ -201,16 +201,17 @@ MOBILE_PLATFORM_VARS = {
>         'builds_before_reboot': localconfig.BUILDS_BEFORE_REBOOT,
>         'build_space': 6,
>         'upload_symbols': True,
>         'slaves': SLAVES['linux'],
>         'platform_objdir': MOBILE_OBJDIR,
>         'enable_ccache': True,
>         'env': {
>             'JAVA_HOME': '/tools/jdk6',
>+            'PATH': '/tools/jdk6/bin:/opt/local/bin:/tools/python/bin:/tools/buildbot/bin:/usr/kerberos/bin:/usr/local/bin:/bin:/usr/bin:/home/'

this line should be 

>+            'PATH': '/tools/jdk6/bin:/opt/local/bin:/tools/python/bin:/tools/buildbot/bin:/usr/kerberos/bin:/usr/local/bin:/bin:/usr/bin:/home/cltbld/bin'
>             'SYMBOL_SERVER_HOST': localconfig.SYMBOL_SERVER_HOST,
>             'SYMBOL_SERVER_USER': 'ffxbld',
>             'SYMBOL_SERVER_PATH': SYMBOL_SERVER_PATH,
>             'SYMBOL_SERVER_SSH_KEY': "/home/cltbld/.ssh/ffxbld_dsa",
>             'MOZ_OBJDIR': MOBILE_OBJDIR,
>             'CCACHE_DIR': '/builds/slave/ccache',
>             'CCACHE_UMASK': '002',
>         },
Attachment #453728 - Flags: review?(catlee) → review+
Attachment #453729 - Flags: review?(catlee) → review+
Attachment #453195 - Flags: checked-in? → checked-in+
Noticed there are lots of builds which shouldn't exist on buildbot 0.8.0 masters:
* all mobile builds for 1.9.1
Linux Mobile Desktop mozilla-1.9.1 nightly
Linux Mobile Desktop mozilla-1.9.1 build
Android R7 mozilla-1.9.1 nightly
Android R7 mozilla-1.9.1 build
OS X 10.5.2 Mobile Desktop mozilla-1.9.1 nightly
Maemo 5 GTK mozilla-1.9.1 nightly
Maemo 5 GTK mozilla-1.9.1 build
Maemo 5 QT mozilla-1.9.1 nightly
Maemo 5 QT mozilla-1.9.1 build
WINNT 5.2 Mobile Desktop mozilla-1.9.1 nightly
Maemo 4 mozilla-1.9.1 nightly
Maemo 4 mozilla-1.9.1 build
* mobile nightlies for twig branches, places where we don't do Firefox nightlies
Linux Mobile Desktop cedar nightly
Android R7 cedar nightly
OS X 10.5.2 Mobile Desktop cedar nightly
Maemo 5 GTK cedar nightly
Maemo 5 QT cedar nightly
WINNT 5.2 Mobile Desktop cedar nightly
Maemo 4 cedar nightly

Please check for others.
Reporter

Updated

9 years ago
Attachment #453944 - Flags: review?(aki) → review+
I don't beleive there is anything left to do in this bug so I am going to close it.  If there are issues with the configuration values or configuration logic please file a new bug.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Doesn't seem worth creating a new bug for this - the places branch was missed in the attachment 453944 [details] [diff] [review] since it has enable_nightly set to False, it also needs enable_mobile_nightly set to false.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
this bug has been closed for nearly a year, i think this should be tracked in a new bug.  filed 642853 to track this.
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.